-
Notifications
You must be signed in to change notification settings - Fork 468
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
fix(policychecks): Update Policy Checks to use profile selected by AWS Toolkits instead of always default #6074
base: master
Are you sure you want to change the base?
Conversation
|
…S Toolkits instead of always default
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.
When someone from your team approves I'll merge it
@@ -179,7 +179,9 @@ export class IamPolicyChecksWebview extends VueWebview { | |||
documentType, |
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.
Firstly, thank you @kevluu-aws for fixing it! 🙌
@@ -919,6 +943,11 @@ export function isJsonPolicyLanguage(document: string) { | |||
return policyLanguageFileTypes.some((t) => document.endsWith(t)) | |||
} | |||
|
|||
export function getProfileName(): string | undefined { | |||
// We neeed to split the name on 'profile:' to extract the correct profile name | |||
return globals.awsContext.getCredentialProfileName()?.split('profile:')[1] |
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.
Is this a constant format? I am trying to understand who enforces this format?
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.
Also, can we add an error message on the backend to determine any incorrect format here?
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.
Toolkits uses the format in displaying the profile name to users after authenticating through the extension. It's stored with profile:
prepended in the Credentials object.
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.
Agreed we should probably refactor the profile name in to a data object instead. It doesnt need to be done in this PR.
Also, can we add an error message on the backend to determine any incorrect format here?
There shouldn't be a need to determine incorrect formats as the format of this data should have been validated here and then right after we create the name with this format.
@@ -155,6 +155,8 @@ describe('validatePolicy', function () { | |||
'us-east-1', |
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 a test that will help us determine if there is any change to the expected format in the getProfileName()
function?
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 CLI tool tests check for a profile to be passed as undefined
. This will check if the 'profile:' part gets changed
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.
Had a chat with Kevin offline regarding if there is a way for us to catch the change in the format of the profile name, and he confirmed that it should be caught in the unit tests.
Does this fix a user-facing symptom? If so, please add a changelog as described in #6074 (comment) |
Problem
Policy Checks currently only uses the default profile within the credentials file for the CLI tool and SDK calls made to A2.
Solution
Pass on the profile name from the current profile setup in AWS Toolkits to the CLI tool and SDK.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.