-
Notifications
You must be signed in to change notification settings - Fork 34
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
Upgrade our authentication library to support MSAL #1942
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
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.
Changes LGTM overall. Is this already tested as part of CI?
4d08db8
to
d101df4
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
d101df4
to
abd9328
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
abd9328
to
dc817a3
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
dc817a3
to
755b29b
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
provider/pkg/provider/provider.go
Outdated
} | ||
|
||
if actual.LessThan(minAzVersion) || actual.GreaterThanOrEqual(nextMajorAzVersion) { | ||
return fmt.Errorf("found incompatible az version %s. %s", actual, versionHint) |
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.
Will this break users on older CLI versions that currently work? Is this a breaking change that we can ship? Could we fall back to the old auth mechanism for older CLIs?
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.
Don't know if there's a fallback option, but if the version is outside the expected range perhaps we could print a big warning but attempt to carry on?
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.
CLI version 2.34 from March 1 2022 broke our users, like in #1565. Version 2.37 from May 24 2022 works again. The hashicorp/go-azure-helpers library only requires that the version is at least 2.0.81. So you're right @mikhailshilkov, we can actually allow versions between 2.0.81 and 2.33 and only exclude 2.34-2.36.
755b29b
to
08bbaa0
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
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 this is basically ready to merge. I've left a couple of suggestions around messages back to the user if something goes wrong:
- Aim to point them to where to help themselves.
- Include information so we could reason about exactly why failures happened based on their local system.
Having said that, these are all things that could be added later, so let's not delay too much as this will already be a huge step forward for many users!
08bbaa0
to
6bbbe87
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
dbfce6a
to
b0bbaed
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
b0bbaed
to
0d3d2db
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
1 similar comment
Does the PR have any schema changes?Looking good! No breaking changes found. |
111c536
to
31115e0
Compare
Does the PR have any schema changes?Looking good! No breaking changes found. |
Implemented by #2320 |
The latest version of
hashicorp/go-azure-helpers/authentication
supports the new MSAL authentication (#1566).This PR also addresses #1565 by requiring the user to have a compatible version of the Azure CLI installed and printing an informative message if not. The corresponding documentation change is in pulumi/registry#1420.
Here's a test of the version handling, done by putting a shell script named
az
in my path that just prints some version.