Added enableSoftDelete, DeletedVault, and Purge to method group

[tiffanyachen]

No description provided.

[anuchandy]

Changing return type of an existing property is a breaking change. Key-vault Fluent Mgmt lib is GA-ed, so we might want to wait for next major version upgrade of fluent lib for this. Can we convert to it string like inner.tenantId.toString() in impl and return.

[anuchandy]

should we use string as param type instead of UUID [for tenantId, applicationId]? to be consistent with the property getter return type.

[anuchandy]

To be consistent with naming pattern of other existing properties returning bool (e.g. enabledForDeployment, enabledForDiskEncryption, enabledForTemplateDeployment), should we rename these new getters, like purgeProtectionEnabled -> enabledForPurgeProtection, softDeleteEnabled -> enabledForSoftDelete

[tiffanyachen]

@schaabs and I discussed this a bit and decided to go with this pattern to make it consistent with our other languages.

[milismsft]

@tiffanyachen in this case you need to be consistent with the rest of the Java SDK naming patterns for this service and not other languages. There's a good reason and lots of review discussions before we chose the naming we use today. So unless you want to change all the other method names and introduce your own new naming pattern, then work around the backward compatibility breaks resulted from these change, then my recommendation is to stay with @anuchandy feedback.

[schaabs]

@anuchandy @milismsft In the case of the first three fields, enableForDeployment, enabledForDiskEncryption, and enabledForTemplateDeployment, they are named this way because the user is enabling the vault to be accesed FOR arm deployments, disk encryption, and template deployment. In the case of softDeleteEnabled and purgeProtectionEnabled the user is enabling the features soft delete and purge protection on the vault.

[milismsft]

@schaabs I don't understand how is soft delete any different then the others... It looks like it is required for instance when setting up SQL TDE (see customer report Azure/azure-libraries-for-net#315).
If "soft delete" and "purge protection" are simple vault properties it might be worth removing the whole "enabled" thing from the name, i.e "bool softDelete()" and "bool purgeProtection()".

[milismsft]

Also the "with" methods could be renamed as "withSoftDelete()" and "withPurgeProtection()" IF these properties are indeed vault only properties.

[anuchandy]

Is it possible to disable soft delete once enabled? if so we may need withSoftDeleteDisabled, withPurgeProtectionDisabled

[tiffanyachen]

SoftDelete and PurgeProtection can't be disabled.

[anuchandy]

We should not remove SupportsDeletingById support, this is a breaking change and keyVault lib is GA-ed.

[anuchandy]

Indentation should be 4 space.

[anuchandy]

You can run mvn checkstyle:check from the project root to catch coding style (e.g. indentation) violations.

[anuchandy]

We can use utility method to simplify this, like

if (inner().properties() == null) {
   return false;
}
return Utils.toPrimitiveBoolean(inner().properties().enableSoftDelete());

[anuchandy]

Seems we are making two api calls, we could have something like

DeletedVaultInner deletedVault = inner().getDeleted(vaultName, location);
if (deletedVault == null) {
    return null;
} else {
   return new DeletedVaultImpl(deletedVault);
}

[anuchandy]

To avoid sleeping while running in mock mode we can use SdkContext.sleep(20000) here.

[anuchandy]

This is correct.

Just make sure we are not using the name wrapPage, we could move converter variable def to public PagedList<DeletedVault> listDeleted() method.

[anuchandy]

Async:

    public Observable<DeletedVault> getDeletedAsync(String vaultName, String location) {
        VaultsInner client = this.inner();
        return client.getDeletedAsync(vaultName, location)
        .map(new Func1<DeletedVaultInner, DeletedVault>() {
            @Override
            public DeletedVault call(DeletedVaultInner inner) {
                return new DeletedVaultImpl(inner);
            }
        });
    }

[anuchandy]

Async

    public Completable purgeDeletedAsync(String vaultName, String location) {
        VaultsInner client = this.inner();
        return client.purgeDeletedAsync(vaultName, location).toCompletable();
    }

[anuchandy]

Async:

    public Observable<DeletedVault> listDeletedAsync() {
        VaultsInner client = this.inner();
        return client.listDeletedAsync()
                .flatMap(new Func1<Page<DeletedVaultInner>, Observable<Page<DeletedVaultInner>>>() {
                    @Override
                    public Observable<Page<DeletedVaultInner>> call(Page<DeletedVaultInner> page) {
                        return listDeletedNextInnerPageAsync(page.nextPageLink());
                    }
                })
                .flatMapIterable(new Func1<Page<DeletedVaultInner>, Iterable<DeletedVaultInner>>() {
                    @Override
                    public Iterable<DeletedVaultInner> call(Page<DeletedVaultInner> page) {
                        return page.items();
                    }
                })
                .map(new Func1<DeletedVaultInner, DeletedVault>() {
                    @Override
                    public DeletedVault call(DeletedVaultInner inner) {
                        return new DeletedVaultImpl(inner);
                    }
                });
    }

    private Observable<Page<DeletedVaultInner>> listDeletedNextInnerPageAsync(String nextLink) {
        if (nextLink == null) {
            Observable.empty();
        }
        VaultsInner client = this.inner();
        return client.listDeletedNextAsync(nextLink)
                .flatMap(new Func1<Page<DeletedVaultInner>, Observable<Page<DeletedVaultInner>>>() {
                    @Override
                    public Observable<Page<DeletedVaultInner>> call(Page<DeletedVaultInner> page) {
                        return Observable.just(page).concatWith(listDeletedNextInnerPageAsync(page.nextPageLink()));
                    }
                });
    }

[anuchandy]

@tiffanyachen - please note that in fluent lib the only async option we expose is RX based (Observable), we will not expose Future based model to customer. The idea is keep the fluent lib simple by exposing one way to do async, and if customers needed they can convert Observable to future using well know operators.

The only additional work we need to do here is expose async and sync for resource specific actions. In case of Vault they are listing deleted vaults, purge a vault and check name availability.

[anuchandy]

We should not expose any of the createOrUpdate variants, the reason is - we have a standard pattern to create [vaults().define(..)..createAsync()] and update [vault.update()..applyAsync()] any resource. We've already exposed them for vaults.

The guideline is - (1) in fluent lib underlying creareOrUpdate should not be exposed as it is, it has to be wrapped in standard fluent patterns.

(2) In fluent layer we will not expose Future based async option, only async option available will be Rx based. Customer can convert Observable<Vault> returned by define.createAsync() and update()..applyAsync() to future using well-known rx operators.

[tiffanyachen]

What about async methods that take a callback and are exposed by VaultsInner?

[anuchandy]

Idea is we expose only one way of doing async programming which is rx. If customer want they can convert rx observable to call back in their application layer. like:

ServiceFuture.fromBody(apiCallthatReturnsRxType(params), callback)

[anuchandy]

Same comment for update as well, it is already supported via standard fluent update()..apply() pattern.

General guideline is - In fluent layer we will not expose Future based async option, only async option available will be Rx based. Customer can convert Observable returned by update()..applyAsync() to future using well-known rx operators.

[anuchandy]

Delete sync and async are already exposed via standard fluent methods Vaults().deleteByResourceGroup(), vaults.deletebyId(), so these three methods does not have to be exposed like this.

General guideline is - In fluent layer we will not expose Future based async option, only async option available will be Rx based. Customer can convert Completable returned by delete to future using well-known rx operators.

[anuchandy]

Vaults already implements SupportsGettingByResourceGroup

azure-libraries-for-java/azure-mgmt-resources/src/main/java/com/microsoft/azure/management/resources/fluentcore/arm/collection/SupportsGettingByResourceGroup.java

Line 23 in f955388

public interface SupportsGettingByResourceGroup<T> {

hence this explicit methods are not needed

General guideline is - In fluent layer we will not expose Future based async option, only async option available will be Rx based. Customer can convert Observable returned by SupportsGettingByResourceGroup.GetByResourceGrouoAsync to future using well-known rx operators.

[anuchandy]

Vaults already implements SupportsListByResourceGroup

azure-libraries-for-java/azure-mgmt-resources/src/main/java/com/microsoft/azure/management/resources/fluentcore/arm/collection/SupportsListingByResourceGroup.java

Line 23 in f955388

public interface SupportsListingByResourceGroup<T> {

hence this explicit methods are not needed

[anuchandy]

create, update and delete of access policy is already exposed as a child operation on keyVault

azure-libraries-for-java/azure-mgmt-keyvault/src/test/java/com/microsoft/azure/management/keyvault/VaultTests.java

Line 46 in f955388

.defineAccessPolicy()

hence no need to expose them like this

[milismsft]

@tiffanyachen I have a similar change that only addresses enabling soft delete and purge protection at milismsft@b52185b. Would you be able to incorporate it into your changes?

[milismsft]

Can you please remove this comment and change the overall description of the method to something like "The result of checking for the Key Vault name availability."?

[milismsft]

I would expect to see here some methods such as isAvailable(), unavailabilityReason() and unavailabilityMessage()...

[tiffanyachen]

Added in latest commit

[tiffanyachen]

@milismsft See @schaabs comment's above regarding property names; ManageSqlServerKeysWithAzureKeyVaultKey.java's changes we can add.

[milismsft]

@tiffanyachen In Fluent we try to avoid exposing the users to the *Inner classes (or any other class that is part of the "implementation" folder except for the *Manager). In this particular case we can "promote" the DeletedVaultProperties properties as methods of this interface rather than having the user navigate through two levels of indirection in order to reach them (one being the .inner()). Would you be able to add the corresponding methods please? The exact mapping is:
location() -> inner().properties().location()
deleteDate() ->inner().properties().deleteDate()
scheduledPurgeDate() -> inner().properties().scheduledPurgeDate()
tags() -> inner().properties().tags()

[milismsft]

@tiffanyachen Could CreateMode.RECOVER be used with an existing Key Vault resource that has not been deleted? What impact other settings prior to reaching this method during the create flow will have when this value is set?

[tiffanyachen]

This can only be used for recovering a deleted vault with soft delete enabled; in any other case it'll result in an error.

[milismsft]

@tiffanyachen If this is the case then we will have to rework a bit the "Create()" flow. What's the maximum properties in the JSON body that can be passed to the create op when CreateMode.RECOVER is specified?
Basically we will need to think of a way to shortcut the current create flow for the user to be able to use this setting. I'm thinking it might be worth having a separate set of methods as part of Vaults.java something like recoverDeletedVault(resourceGroupName, vaultName) or recoverSoftDeletedVault() (plus the Async one), instead of expecting the user to specify the CreateMode via the define()/create() flow... Because I don't see the later how it will ever succeed when we ask the user for SKU and other stuff before it can reach to withCreateMode().

[milismsft]

@tiffanyachen the cast is not needed here (if DeletedVaultImpl class implements DeletedVault interface).

[milismsft]

@tiffanyachen the Travis build is failing because of check-style errors; you can try to reproduce them locally by executing "mvn checkstyle:check".

[milismsft]

@tiffanyachen Same here, we can remove this method as well.

[milismsft]

@tiffanyachen Let's remove this method since we don't need to expose it in the Fluent implementation.

[milismsft]

Same as above.

[milismsft]

... and this one :-)

[milismsft]

... and this one.

[milismsft]

please remove, not needed for Fluent.

[milismsft]

please remove, not needed for Fluent.

[milismsft]

please remove, not needed for Fluent.

[milismsft]

please remove, not needed for Fluent.

[milismsft]

please remove, not needed for Fluent.

[milismsft]

@tiffanyachen similar comments I made about the DeletedVault interface; we should "promote" the properties from VaultAccessPolicyPropertiesInner class as methods of this interface. Also the interface should extend HasId(), HasName() and expose type() and location().

[anuchandy]

@tiffanyachen, @schaabs PR is good, just approved it. You can ignore the CI failures (which are not related to key vault), when we merge kv2018 back to master, we will make sure its green.