nacx commented on this pull request.
Many thanks for this huge effort!
I've commented mostly style things. Excellent job!
> + }
+
+ @Provides
+ protected Predicate<Supplier<Provisionable>>
provideResourceAvailablePredicate(final AzureComputeApi api,
+
@Named(OPERATION_TIMEOUT) Integer operationTimeout, PollPeriod pollPeriod) {
+ return retry(new ResourceInStatusPredicate("Succeeded"),
operationTimeout, pollPeriod.pollInitialPeriod,
+ pollPeriod.pollMaxPeriod);
+ }
+
+ @Provides
+ @Named("STORAGE")
+ protected Predicate<URI> provideStorageAccountAvailablePredicate(final
AzureComputeApi api,
+
@Named(OPERATION_TIMEOUT) Integer operationTimeout, PollPeriod pollPeriod) {
+ return retry(new ActionDonePredicate(api), operationTimeout,
pollPeriod.pollInitialPeriod,
+ pollPeriod.pollMaxPeriod);
+ }
I'd say this predicate is no longer used so we can remove it (not mandatory for
this PR, just as a reminder for us)
> + }
+
+ @Provides
+ @Named(VAULT_DELETE_STATUS)
+ protected VaultPredicates.DeletedVaultStatusPredicateFactory
provideDeletedVaultStatusPredicateFactory(final AzureComputeApi api,
+
@Named(OPERATION_TIMEOUT) Integer operationTimeout,
+
final PollPeriod pollPeriod) {
+ return new VaultPredicates.DeletedVaultStatusPredicateFactory(api,
operationTimeout.longValue(), pollPeriod.pollInitialPeriod,
pollPeriod.pollMaxPeriod);
+ }
+
+ public static class VaultPredicates {
+ public static class DeletedVaultStatusPredicateFactory {
+ private final AzureComputeApi api;
+ private final long operationTimeout;
+ private final long initialPeriod;
+ private final long maxPeriod;
I know the existing VirtualMachine predicate uses this pattern, but I think it
should be better to let the factory create predicates that don't retry, just
simple predicates, and let the `@Provides` method decorate it with the `retry`
logic. This way we have an injectable, blocking predicate that retries, but we
also can instantiate a simple predicate just to check the status of the
resource. Does it make sense to you?
The same change could be applied to the existing VM predicate, to align it with
all others.
> +
+ DeletedVaultStatusPredicateFactory(final AzureComputeApi api,
final long operationTimeout, final long initialPeriod, final long maxPeriod) {
+ this.api = checkNotNull(api, "api cannot be null");
+ this.operationTimeout = operationTimeout;
+ this.initialPeriod = initialPeriod;
+ this.maxPeriod = maxPeriod;
+ }
+
+ public Predicate<String> create(final String resourceGroup, final
boolean shouldBePresent) {
+ checkNotNull(resourceGroup, "resourceGroup cannot be null");
+ return retry(new Predicate<String>() {
+ @Override
+ public boolean apply(final String name) {
+ checkNotNull(name, "name cannot be null");
+ boolean present = false;
+ checkNotNull(name, "name cannot be null");
Duplicate check.
> + DeletedVaultStatusPredicateFactory(final AzureComputeApi api,
> final long operationTimeout, final long initialPeriod, final long maxPeriod) {
+ this.api = checkNotNull(api, "api cannot be null");
+ this.operationTimeout = operationTimeout;
+ this.initialPeriod = initialPeriod;
+ this.maxPeriod = maxPeriod;
+ }
+
+ public Predicate<String> create(final String resourceGroup, final
boolean shouldBePresent) {
+ checkNotNull(resourceGroup, "resourceGroup cannot be null");
+ return retry(new Predicate<String>() {
+ @Override
+ public boolean apply(final String name) {
+ checkNotNull(name, "name cannot be null");
+ boolean present = false;
+ checkNotNull(name, "name cannot be null");
+ List<Vault.DeletedVault> vaults =
api.getVaultApi(resourceGroup).listDeletedVaults();
More idiomatic:
```java
return shouldBePresent == Iterables.any(vaults, new
Predicate<Vault.DeletedVault>() {
@Override public boolean apply(Vault.DeletedVault input) {
return input.name().equals(name);
}
});
```
> + }
+
+ public Predicate<String> create(final String resourceGroup, final
URI vaultUri, final boolean shouldBePresent) {
+ checkNotNull(resourceGroup, "resourceGroup cannot be null");
+ checkNotNull(vaultUri, "vaultUri cannot be null");
+ return retry(new Predicate<String>() {
+ @Override
+ public boolean apply(final String name) {
+ boolean present = false;
+ checkNotNull(name, "name cannot be null");
+ DeletedKeyBundle key =
api.getVaultApi(resourceGroup).getDeletedKey(vaultUri, name);
+ if (shouldBePresent) {
+ return key != null;
+ } else {
+ return key == null;
+ }
Just?
```java
return shouldBePresent == (key != null);
```
> +
+ public Predicate<String> create(final String resourceGroup, final
URI vaultUri, final boolean isRecovered) {
+ checkNotNull(resourceGroup, "resourceGroup cannot be null");
+ checkNotNull(vaultUri, "vaultUri cannot be null");
+ return retry(new Predicate<String>() {
+ @Override
+ public boolean apply(final String name) {
+ checkNotNull(name, "name cannot be null");
+ boolean result = false;
+ KeyBundle key =
api.getVaultApi(resourceGroup).getKey(vaultUri, name);
+ if (key != null) {
+ if (isRecovered) {
+ result = true;
+ } else {
+ result =
key.attributes().recoveryLevel().contains("Recoverable");
+ }
[minor style] Prefer inline conditionals.
> + }
+
+ public Predicate<String> create(final String resourceGroup, final
URI vaultUri, final boolean shouldBePresent) {
+ checkNotNull(resourceGroup, "resourceGroup cannot be null");
+ checkNotNull(vaultUri, "vaultUri cannot be null");
+ return retry(new Predicate<String>() {
+ @Override
+ public boolean apply(final String name) {
+ boolean present = false;
+ checkNotNull(name, "name cannot be null");
+ DeletedSecretBundle secret =
api.getVaultApi(resourceGroup).getDeletedSecret(vaultUri, name);
+ if (shouldBePresent) {
+ return secret != null;
+ } else {
+ return secret == null;
+ }
```java
return shouldBePresent == (secret != null);
```
> @@ -31,10 +31,10 @@
@AutoValue
public abstract static class Permissions {
- @Nullable public abstract List<String> certificates();
- @Nullable public abstract List<String> keys();
- @Nullable public abstract List<String> secrets();
- @Nullable public abstract List<String> storage();
+ public abstract List<String> certificates();
+ public abstract List<String> keys();
+ public abstract List<String> secrets();
+ public abstract List<String> storage();
[minor] leading space.
>
@RequestFilters({ OAuthFilter.class, ApiVersionFilter.class })
@Consumes(MediaType.APPLICATION_JSON)
public interface VaultApi {
+ static class PrependSlashOrEmptyString implements Function<Object, String> {
+ public String apply(Object from) {
+ if((from == null) || (from.toString().length() == 0)) {
[minor style] space after `if`.
> @@ -411,11 +426,12 @@ CertificateOperation createCertificate(@EndpointParam
> URI vaultBaseUrl,
@Named("certificate:get")
@GET
- @Path("/certificates/{certificateName}")
+
@Path("/certificates/{certificateName}{certificateVersion}{certificateVersion}")
Duplicate version variable.
> public void forceVaultRemoval() {
// see if the vault has been deleted or not
Vault vault = api().getVault(vaultName);
if (vault != null) {
if ((vault.properties().enableSoftDelete() != null) &&
vault.properties().enableSoftDelete()) {
api().deleteVault(vaultName);
- waitForOperation("vault", "delete", vaultName);
+ Object[] args = { resourceGroupName, vaultName, true };
Unused args?
> @@ -199,107 +119,13 @@ public void forceVaultRemoval() {
DeletedVault deletedVault = api().getDeletedVault(LOCATION, vaultName);
if (deletedVault != null) {
api().purgeVault(LOCATION, vaultName);
- waitForOperation("vault", "purge", vaultName);
+ Object[] args = { resourceGroupName, vaultName, true };
Unused args?
> @@ -394,8 +218,11 @@ public void testUpdateVaultToSoftDelete() {
@Test(dependsOnMethods = {"testPurgeDeletedKey", "testPurgeDeletedSecret"})
public void testDelete() {
- api().deleteVault(vaultName);
- assertTrue(waitForOperation("vault", "delete", vaultName));
+ boolean result = api().deleteVault(vaultName);
+ assertTrue(result);
+ Object[] args = { resourceGroupName, vaultName, true };
Unused args?
> @@ -414,7 +241,10 @@ public void testListDeleted() {
@Test(dependsOnMethods = {"testGetDeleted", "testListDeleted"})
public void testPurgeDeletedVault() {
api().purgeVault(LOCATION, vaultName);
- assertTrue(waitForOperation("vault", "purge", vaultName));
+ Object[] args = { resourceGroupName, vaultName, false };
Unused args?
--
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-84682266
