-
Notifications
You must be signed in to change notification settings - Fork 867
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
Expose CredentialProvider #4235
Conversation
cc @roeap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. Thanks @tustvold
|
||
Arc::new(TokenCredentialProvider::new( | ||
let credentials = if let Some(credentials) = self.credentials { | ||
credentials |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is worth considering a warn!
here if access_key_id
or secret_access_key
is set that they are being ignored in favor of the other credentials. I know the docs say this is the behavior but I think as a user I would find it somewhat confusing if the access_key_id got silently ignored -- I would rather the library loudly complained so I can fix it
Maybe in that vein, the builder should return an error if conflicting credentials are supplied
🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this as some systems may source config from the environment, which may then contain redundant credentials. I've filed #4240 to track this, and will come back to this after I've had some time to think on it, and others have had time to weigh in
@@ -937,7 +954,9 @@ impl MicrosoftAzureBuilder { | |||
let url = Url::parse(&account_url) | |||
.context(UnableToParseUrlSnafu { url: account_url })?; | |||
|
|||
let credential = if let Some(bearer_token) = self.bearer_token { | |||
let credential = if let Some(credential) = self.credentials { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here about erroring / warning
Which issue does this PR close?
Closes #4163
Rationale for this change
This exposes the CredentialProvider abstraction added in #4225. This allows users to provide custom mechanisms for sourcing credentials (#4163) and opening the door to exposing the authorisation logic (#4223)
What changes are included in this PR?
Are there any user-facing changes?