nacx commented on this pull request.
Thanks @jmspring! Huge effort!
I'm adding some general comments after a first review, but I still have to go
in deeper detail through the entire PR again:
* In the model classes, enforce immutable lists.
* Also try to use non-null collections and enforce presence where possible
(`tags` are an example of field where null is OK, but in most cases I'd say it
should be safe to enforce presence in collections).
* Use the `Date` type for fields that represent dates.
* In the API methods, remove all 404 fallbacks from PUT/POST/PATCH methods.
* Consider removing the TrueIf2xx class if jclouds already provides that
behavior by default.
Regarding the KeyVaultFilter. It looks like a clone of the
`ClientCredentialsSecretFlow` filter. Instead of overriding it to cover the MWS
use case, couldn't we configure the tests to use "localhost" for the vaultURL
so the default OAuthFilter can be used? What are the limitations that made you
change this?
Thanks for all the effort!
> @@ -411,7 +411,7 @@ private OSProfile createOsProfile(String computerName,
> Template template) {
private List<NetworkInterfaceCard> createNetworkInterfaceCards(final String
nodeName, final String location,
AzureTemplateOptions options) {
- // Prefer a sorted list of NICs with the ones with public IPs first, to
+ // Prefer a sorted listVaults of NICs with the ones with public IPs
first, to
typo?
> + }
+ }
+
+ @AutoValue
+ public abstract static class SubjectAlternativeNames {
+ public abstract List<String> dnsNames();
+
+ public abstract List<String> emails();
+
+ public abstract List<String> upns();
+
+ @SerializedNames({"dns_names", "emails", "upns"})
+ public static SubjectAlternativeNames create(final List<String> dnsNames,
+ final List<String> emails,
+ final List<String> upns) {
+ return new AutoValue_Certificate_SubjectAlternativeNames(dnsNames,
emails, upns);
Enforce immutability
> +
+ @Nullable
+ public abstract SubjectAlternativeNames subjectAltNames();
+
+ @Nullable
+ public abstract String subject();
+
+ public abstract int validityMonths();
+
+ @SerializedNames({"ekus", "key_usage", "sans", "subject",
"validity_months"})
+ public static X509CertificateProperties create(final List<String>
enhancedKeyUsage,
+ final List<String>
keyUsage,
+ final
SubjectAlternativeNames subjectAltNames,
+ final String subject,
+ final int validityMonths)
{
+ return new
AutoValue_Certificate_X509CertificateProperties(enhancedKeyUsage, keyUsage,
subjectAltNames, subject, validityMonths);
Enforce immutability in all lists when present. Is there a reason to keep these
lists null, or could we better default to empty lists, to avoid the
null-collection anti-pattern?
> + @SerializedNames({"id", "provider"})
+ public static CertificateIssuer create(final String id,
+ final String provider) {
+ return new AutoValue_Certificate_CertificateIssuer(id, provider);
+ }
+ }
+
+ @AutoValue
+ public abstract static class IssuerAttributes {
+ @Nullable
+ public abstract String created();
+
+ public abstract boolean enabled();
+
+ @Nullable
+ public abstract String updated();
Can we use a `Date` type for the created and updated fields?
> + public abstract String id();
+
+ @SerializedNames({"contacts", "id"})
+ public static Contacts create(final List<Contact> contacts,
+ final String id) {
+ return new AutoValue_Certificate_Contacts(contacts, id);
+ }
+ }
+
+ @AutoValue
+ public abstract static class DeletedCertificateBundle {
+ @Nullable
+ public abstract CertificateAttributes attributes();
+
+ @Nullable
+ public abstract String bytes();
[minor] The method name suggests a return type of `byte[]`, but let's keep it
as-is if a string really makes more sense given the content of the field.
> + public static Contacts create(final List<Contact> contacts,
+ final String id) {
+ return new AutoValue_Certificate_Contacts(contacts, id);
+ }
+ }
+
+ @AutoValue
+ public abstract static class DeletedCertificateBundle {
+ @Nullable
+ public abstract CertificateAttributes attributes();
+
+ @Nullable
+ public abstract String bytes();
+
+ @Nullable
+ public abstract String deletedDate();
Change type to `Date`?
> + thumbprint
+ );
+ }
+ }
+
+ @Nullable
+ public abstract CertificateAttributes attributes();
+
+ @Nullable
+ public abstract String id();
+
+ @Nullable
+ public abstract Map<String, String> tags();
+
+ @Nullable
+ public abstract String x5t();
Why not "thumbprint" as in other classes?
> + }
+
+ @AutoValue
+ public abstract static class KeyAttributes {
+ @Nullable
+ public abstract Boolean enabled();
+ @Nullable
+ public abstract String created();
+ @Nullable
+ public abstract String expires();
+ @Nullable
+ public abstract String notBefore();
+ @Nullable
+ public abstract String recoveryLevel();
+ @Nullable
+ public abstract String updated();
Use `Date` types where possible.
> +public interface VaultApi {
+ // Vault operations
+ @Named("vault:list")
+ @SelectJson("value")
+ @GET
+ @Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults")
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ List<Vault> listVaults();
+
+ @Named("vault:create_or_update")
+ @PUT
+ @MapBinder(BindToJsonPayload.class)
+ @Fallback(NullOnNotFoundOr404.class)
+
@Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults/{vaultName}")
+ Vault createOrUpdateVault(@PathParam("vaultName") String vaultName,
@PayloadParam("location") String location,
+ @PayloadParam("properties") VaultProperties properties);
Add a `tags` parameter after the "location" one, for consistency with other
APIs.
> +
+@RequestFilters({ OAuthFilter.class, ApiVersionFilter.class })
+@Consumes(MediaType.APPLICATION_JSON)
+public interface VaultApi {
+ // Vault operations
+ @Named("vault:list")
+ @SelectJson("value")
+ @GET
+ @Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults")
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ List<Vault> listVaults();
+
+ @Named("vault:create_or_update")
+ @PUT
+ @MapBinder(BindToJsonPayload.class)
+ @Fallback(NullOnNotFoundOr404.class)
Do not use 404 fallbacks in write (PUT/POST) operations. An unexisting path
should be very unexpected in this case and throw an exception (as opposed to
get/delete, where you might expect the resource to not exist and null is fine
as a return value).
> + @Path("/providers/Microsoft.KeyVault/deletedVaults")
+ @GET
+ @SelectJson("value")
+ @Fallback(EmptyListOnNotFoundOr404.class)
+ List<DeletedVault> listDeletedVaults();
+
+ @Named("vault:get_deleted")
+ @GET
+
@Path("/providers/Microsoft.KeyVault/locations/{location}/deletedVaults/{vaultName}")
+ @Fallback(NullOnNotFoundOr404.class)
+ DeletedVault getDeletedVault(@PathParam("location") String location,
@PathParam("vaultName") String vaultName);
+
+ @Named("vault:purge")
+ @POST
+ @ResponseParser(TrueOn20X.class)
+ @Fallback(FalseOnNotFoundOr404.class)
Remove fallback
> +import com.google.common.base.Supplier;
+import com.google.common.cache.CacheBuilder;
+import com.google.common.cache.CacheLoader;
+import com.google.common.cache.LoadingCache;
+
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static org.jclouds.Constants.PROPERTY_SESSION_INTERVAL;
+import static org.jclouds.oauth.v2.config.OAuthProperties.RESOURCE;
+
+/**
+ * Allow users to customize the api versions for each method call.
+ * <p>
+ * In Azure ARM, each method may have its own api version. This filter allows
to
+ * configure the versions of each method, so there is no need to change the
code
+ * when Azure deprecates old versions.
+ */
Wrong javadoc
> + @MapBinder(BindToJsonPayload.class)
+ @Fallback(NullOnNotFoundOr404.class)
+
@Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults/{vaultName}")
+ Vault createOrUpdateVault(@PathParam("vaultName") String vaultName,
@PayloadParam("location") String location,
+ @PayloadParam("properties") VaultProperties properties);
+
+ @Named("vault:get")
+
@Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults/{vaultName}")
+ @GET
+ @Fallback(NullOnNotFoundOr404.class)
+ Vault getVault(@PathParam("vaultName") String vaultName);
+
+ @Named("vault:delete")
+
@Path("/resourcegroups/{resourcegroup}/providers/Microsoft.KeyVault/vaults/{vaultName}")
+ @DELETE
+ @ResponseParser(TrueOn20X.class)
Is this needed? Doesn't jclouds return true for 2xx by default when the return
type is a boolean? If so, let's remove tis parser class.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds-labs/pull/416#pullrequestreview-74423778
