-
Notifications
You must be signed in to change notification settings - Fork 219
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
PS and CLI creds #2433
PS and CLI creds #2433
Conversation
720fb69
to
01a7b8b
Compare
@@ -114,6 +114,17 @@ func GetOAuthTokenManagerInstance() (*common.UserOAuthTokenManager, error) { | |||
|
|||
case "DEVICE": | |||
lca.identity = false | |||
|
|||
case "AZCLI": | |||
lca.identity = false |
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.
in the future we might be able to improve this by creating an enum for oauth cred type instead of having a bunch of booleans
return output, nil | ||
} | ||
|
||
func (c *PowershellContextCredential) createAccessToken(tk []byte) (azcore.AccessToken, error) { |
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.
can we add UT for some of these methods?
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.
Did you add tests for some of these methods?
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.
The test covers entire workflow.
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.
Which test? I only see some changes to runner, how does that get picked up in CI?
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.
KMN. Thanks for getting this. I had not checked in the file. I've checked in now, need to get some environment vars correct in CI.
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.
great! thanks for double checking that. I thought I was going crazy for a minute
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.
left some comments
…nto nakulkar/private/psCLICreds2
@@ -32,7 +32,7 @@ import ( | |||
var loginCmdArg = loginCmdArgs{tenantID: common.DefaultTenantID} | |||
|
|||
var loginNotice = "'azcopy %s' command will be deprecated starting release 10.22. " + |
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.
You could probably update this to 10.23 since we are skipping this release, right?
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'll add that as a different review. Deviates from topic.
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.
please create an item to add automated tests for this in the future
No description provided.