-
Notifications
You must be signed in to change notification settings - Fork 65
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
update to az identity 1.0.0 #30
Conversation
6df68fd
to
2a28382
Compare
2a28382
to
dcade40
Compare
Codecov Report
@@ Coverage Diff @@
## main #30 +/- ##
==========================================
+ Coverage 71.20% 71.33% +0.12%
==========================================
Files 24 24
Lines 5418 5418
==========================================
+ Hits 3858 3865 +7
+ Misses 1314 1309 -5
+ Partials 246 244 -2
Continue to review full report at Codecov.
|
azuread/configuration.go
Outdated
@@ -188,8 +205,15 @@ func (p *azureFedAuthConfig) provideActiveDirectoryToken(ctx context.Context, se | |||
cred, err = azidentity.NewManagedIdentityCredential(nil) | |||
} | |||
case ActiveDirectoryInteractive: | |||
cred, err = azidentity.NewInteractiveBrowserCredential(&azidentity.InteractiveBrowserCredentialOptions{AuthorityHost: azidentity.AuthorityHost(authority), ClientID: p.applicationClientID}) | |||
cloudConfig, cerr := getCloudConfig(authority) |
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.
why do we need to look up the cloud config? There are TDS frontends that support AAD auth whose authority URL is not one of the standard Azure URLs.
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.
Breaking change in az identity that I mentioned. I would doubt we have many people using the interactive option
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.
We could alternatively support them passing the full configuration
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.
Either way — this is blocked unless we update to use go1.18 I think
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.
sqlcmd uses 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.
hah, fair enough
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.
this seems to work
case ActiveDirectoryInteractive:
c := cloud.Configuration{ActiveDirectoryAuthorityHost: authority}
config := azcore.ClientOptions{Cloud: c}
cred, err = azidentity.NewInteractiveBrowserCredential(&azidentity.InteractiveBrowserCredentialOptions{ClientOptions: config, ClientID: p.applicationClientID})
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.
that's effectively what's in the PR now, with yours having more done in-line
cloudConfig := cloud.Configuration{ActiveDirectoryAuthorityHost: authority, Services: map[cloud.ServiceName]cloud.ServiceConfiguration{}}
opts := &azidentity.InteractiveBrowserCredentialOptions{ClientID: p.applicationClientID}
opts.Cloud = cloudConfig
cred, err = azidentity.NewInteractiveBrowserCredential(opts)
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 was looking at an older commit somehow
@@ -1,63 +1,63 @@ | |||
package azuread |
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.
why is the online diff showing every line as changed in these files?
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.
go fmt has apparently changed since version of go these were originally written in -- given the move to 1.18 for this package, makes sense to go ahead and use the current version of go fmt too
} | ||
|
||
} | ||
package azuread |
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 seeing any changes in this file
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.
go fmt apparently adjusted whitespace -- from the original version which was formatted with go1.11 to this using go1.18
} | ||
return mssql.NewConnectorConfig(config.mssqlConfig), nil | ||
} | ||
package azuread |
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 seeing any changes in this file
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.
go fmt apparently adjusted whitespace?
I'm trying to compile this PR, and it's failing with: Build constraints exclude all the Go files in 'C:/Users//go/pkg/mod/github.com/!azure/azure-sdk-for-go/sdk/[email protected]' Any ideas? I have set the Project "Go SDK" to v1.18.3 |
What I'd like to do: |
I think we can make this change without a new module, which I tried at #34 |
#29