Allow user to set keyvault secret client option

[zhiyuanliang-ms]

Why this PR?

#274

Visible change

New API
AzureAppConfigurationKeyVaultOptions.ConfigureClientOptions

usage:

IConfiguration config = new ConfigurationBuilder()
    .AddAzureAppConfiguration(options =>
    {
        options.ConfigureKeyVault(kv =>
        {
            kv.SetCredential(new DefaultAzureCredential());
            kv.ConfigureClientOptions(sco => sco.Retry.MaxAttempts = 3;
        });
    })
    .Build();

[zhiyuanliang-ms]

https://github.com/Azure/AppConfiguration-DotnetProvider/blob/main/src/Microsoft.Extensions.Configuration.AzureAppConfiguration/AzureAppConfigurationOptions.cs#L362

We have ConfigureClientOptions and other ConfigureXXX methods for AzureAppConfigurationOptions. However, this naming convention wasn't followed by AzureAppConfigurationKeyVaultOptions. We have SetXXX methods for AzureAppConfigurationKeyVaultOptions. I think SetClientOptions is more appropriate here. What do you think?

@jimmyca15 @amerjusupovic @avanigupta @zhenlan

[amerjusupovic]

I think SetClientOptions is more appropriate here. What do you think?

I think part of the reason all the methods in AzureAppConfigurationKeyVaultOptions start with Set is that the methods aren't really configuring anything, it's just passing in a secret resolver, credential, or refresh interval. For example, AzureAppConfigurationRefreshOptions.SetRefreshInterval follows that pattern. However, I think the other Configure methods in the main AzureAppConfigurationOptions involve actually configuring an options class, which is what we're doing in the proposed SetClientOptions.

I think that means AzureAppConfigurationKeyVaultOptions.ConfigureClientOptions would make more sense to me. That's just how I see it right now though, any other thoughts?

[zhenlan]

It's not just the naming. The benefit of ConfigureClientOptions is that users can make necessary client option changes directly inline (#106). They don't have to create a new SecretClientOptions object in advance and replace the current one.

public AzureAppConfigurationOptions ConfigureClientOptions(Action<ConfigurationClientOptions> configure)

[jimmyca15]

It's not just the naming. The benefit of ConfigureClientOptions is that users can make necessary client option changes directly inline (#106). They don't have to create a new SecretClientOptions object in advance and replace the current one.

public AzureAppConfigurationOptions ConfigureClientOptions(Action<ConfigurationClientOptions> configure)

I agree here, I prefer the case where the user gets passed a ClientOptions in the callback.

So in your example the usage becomes

IConfiguration config = new ConfigurationBuilder()
    .AddAzureAppConfiguration(options =>
    {
        options.ConfigureKeyVault(kv =>
        {
            kv.SetCredential(new DefaultAzureCredential());
            kv.ConfigureClientOptions(sco =>
            {
                        sco.Retry.MaxAttempts = 3;
            });
        });
    })
    .Build();

[zhiyuanliang-ms]

@jimmyca15 @zhenlan I agree with the proposal. But my only concern is that user will not be able to set SecretClientOptions.Version through the callback pattern.

This is the implementation of SecretClientOptions.

public class SecretClientOptions : ClientOptions
{
    internal const ServiceVersion LatestVersion = ServiceVersion.V7_5;

    public ServiceVersion Version { get; }

    public bool DisableChallengeResourceVerification { get; set; }

    public SecretClientOptions(ServiceVersion version = ServiceVersion.V7_5)
    {
        Version = version;
        this.ConfigureLogging();
    }
}

If user want to configure the keyvault version, it can only be achieved through constructor because Version property doesn't have a public setter.

[jimmyca15]

@zhiyuanliang-ms I think it's a good call out, but I still think it's the pattern we want.