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
provider/azure: update auth-types #6249
Conversation
QA
|
5c782a8
to
6e00364
Compare
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.
LGTM, with a couple small suggestions.
credAttrAppPassword = "application-password" | ||
|
||
clientCredentialsAuthType cloud.AuthType = "service-principal-secret" | ||
deviceCodeAuthType cloud.AuthType = "interactive" |
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.
deviceCodeAuthType? I don't understand how that relates to interactive. Maybe a comment here would be appropriate.
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.
"device code" and "client credentials" are the OAuth terms. I gave them names that would be more meaningful to a user. Added comments to the constants.
label := in.Label | ||
in = cloud.NewCredential(clientCredentialsAuthType, attrs) | ||
in.Label = label | ||
fallthrough |
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.
fallthrough seems extraneous here? Why not just return in, nil?
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.
Yeah, I was thinking we might need to do something for the resultant auth-type. I don't think we ever will, so updated.
We introduces two new auth-types, and deprecate use of "userpass", for Azure. The userpass auth-type is superseded by service-principal-secret, which is identical except that it does not include the tenant-id field. There is a new "interactive" auth-type which is defined, but not yet supported. If a user uses "userpass", they will receive a warning explaining that it is deprecated, and what they need to do to migrate. The credential will continue to work, we'll just ignore the tenant-id.
6e00364
to
67415e5
Compare
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
Build failed: Tests failed |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
We introduces two new auth-types, and deprecate
use of "userpass", for Azure. The userpass auth-type
is superseded by service-principal-secret, which is
identical except that it does not include the
tenant-id field. There is a new "interactive"
auth-type which is defined, but not yet supported.
If a user uses "userpass", they will receive a
warning explaining that it is deprecated, and what
they need to do to migrate. The credential will
continue to work, we'll just ignore the tenant-id.