-
Notifications
You must be signed in to change notification settings - Fork 4k
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
feat(cli): support SSO #19454
feat(cli): support SSO #19454
Changes from 18 commits
f12a0ac
03c3a26
27437e8
0d43399
6d8f6d9
71a9596
182635f
841ab25
2e1b803
c5e3153
7972f52
7c9884c
24df923
17348cd
30768d6
504a56c
47b4d93
11a9a01
b4d7c18
5d36999
00da3f7
778a58b
7f99bc1
0cccd51
e322095
c44e364
e0cf8b1
d61de2b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ export class AwsCliCompatible { | |
const theProfile = options.profile; | ||
return new AWS.CredentialProviderChain([ | ||
() => profileCredentials(theProfile), | ||
() => new AWS.SsoCredentials({ profile: theProfile }), | ||
() => new AWS.ProcessCredentials({ profile: theProfile }), | ||
]); | ||
} | ||
|
@@ -53,6 +54,11 @@ export class AwsCliCompatible { | |
() => new AWS.EnvironmentCredentials('AMAZON'), | ||
]; | ||
|
||
if (process.env.AWS_PROFILE) { | ||
await forceSdkToReadConfigIfPresent(); | ||
sources.push(() => new AWS.SsoCredentials({ profile: implicitProfile })); | ||
} | ||
|
||
if (await fs.pathExists(credentialsFileName())) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you try it without this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @rix0rrr yep, this change works, with both an existing (but empty) credentials file and with no credentials file. Removing the config file breaks it, but that's expected. I also had to change one of the tests to use the 5th member of the chain instead of the 2nd (see diff), but this should be fine because that test seems to check for existence only. |
||
// Force reading the `config` file if it exists by setting the appropriate | ||
// environment variable. | ||
|
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 want Eli to have a look at this.
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 why, but looks like the
aws-sdk
upgrade in the CLI package broke the hoisting.Currently on master we have:
<repoRoot>/node_modules/aws-sdk (version 2.1094.0)
When I upgraded
aws-sdk@^2.979.0
toaws-sdk@^2.1093.0
and ranyarn install
, I got the following directory structure:I expected version 2.1096.0 to be hoisted and replace 2.1094.0 - but thats not what happens.
So in practice the CLI uses two different versions of
aws-sdk
. One coming from its own direct dependency, which isn't hoisted anymore (2.1096.0), and one coming transitively fromcdk-assets
, which is hoisted (2.1094.0).We need to figure out why yarn isn't hoisting it. @rix0rrr Any ideas?
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.
No, feels like it should be able to. Wonder if there's a constraint somewhere that's preventing the hoist?