Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New API model #80

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

New API model #80

wants to merge 2 commits into from

Conversation

Kralizek
Copy link
Owner

@Kralizek Kralizek commented Nov 8, 2022

This PR introduces a new API model for registering and configuring the provider.

[skip ci]
@avalara-stephen-hickey
Copy link
Contributor

avalara-stephen-hickey commented Nov 8, 2022

I like having the credentials and region moved to the options.

I don't think I care for the examples in Sample8. For instance, if someone wants to make changes to the options they are passing to .UseConfiguration, they should just do it before passing it to the library.

// Sample8
var configuration = configurationBuilder.Build();
var options = configuration.GetSection("SecretsManager").GetSecretsManagerOptions()
configurationBuilder.AddSecretsManager(configure =>
{
    configure.UseConfiguration(configuration.GetSection("SecretsManager").GetSecretsManagerOptions());
    configure.Configure(o => o.AcceptedSecretArns.Add("arn:example:03"));
    ...
});

// Idea
var configuration = configurationBuilder.Build();
var options = configuration.GetSection("SecretsManager").GetSecretsManagerOptions()
options.AcceptedSecretArns.Add("arn:example:03");

configurationBuilder.AddSecretsManager(configure =>
{
    configure.UseConfiguration(options);
});

I think the options inside another options class is also a bit confusing.

And if you go that way, I don't know why a user doesn't just build the entire options class themselves and pass it to the library.

var configuration = configurationBuilder.Build();
var options = configuration.GetSection("SecretsManager").GetSecretsManagerOptions()
options.AcceptedSecretArns.Add("arn:example:03");
options.AwsOptions = configuration.GetSection("SecretsManagerProfile").GetAWSOptions();

// The method signature would look like
// `.AddSecretsManager(SecretsManagerConfigurationProviderOptions = null, Action<SecretsManagerConfigurationProviderOptions>? configure = null)`
// Or you could have several overloads, whichever you prefer.
configurationBuilder.AddSecretsManager(options, configure =>
{
    configure.AcceptedSecretArns.Add("arn:example:03");
});

That way, users who want to pull from configuration and who want to configure it programmatically both look very similar. Or even users who want part of it in configuration and part of it done programmatically.

@Kralizek
Copy link
Owner Author

Kralizek commented Nov 9, 2022

I don't know why a user doesn't just build the entire options class themselves and pass it to the library.

@avalara-stephen-hickey Thank you for your comment. It inspired me to greatly simplify the API.

I'm not sure if I went too far and cut too much now. I still have to decide the final name for the options classes and I want to rework on the scopes marking more stuff as internal and possibly sealed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants