-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Allows Cloudshell to acquire perm from AWS_CONTAINER_CREDENTIALS_FULL v2 #2401
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
Conversation
|
ATTN: @stobrien89 Previous PR #2324 , changes made to the comment. |
|
Hi @kuettai, Thanks for taking the time to submit the changes. I'll discuss with the team and get back to you as soon as I can. If it's alright with you, I'll close the older PR and focus on this one for now. |
stobrien89
left a comment
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.
Implementation looks good- the testing just needs to be refined. Ideally we'd want to have a separate test in EcsCredentialProviderTest that simulates a scenario where we'd be using these two env vars and checks that things like the Authorization header are being set correctly.
…ble, and create unit-test accordingly
stobrien89
left a comment
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.
Conditional approval to run checks
stobrien89
left a comment
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 a comment regarding the uri retrieval logic. We'll need to re-order some things for sake of consistency with other SDKs
… relative-uri before full-url
stobrien89
left a comment
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 one more comment. Appreciate your patience here. I think we'll be ready for a final review after it's addressed.
… retrieval from as well
stobrien89
left a comment
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.
Pending team review
|
Hi @kuettai and @BenHarris, Thanks again for your work on this. Tested manually using docker as well as cloudshell and everything works as expected. |
Issue #2323 , if available:
Description of changes:
Added supports for AWS_CONTAINER_CREDENTIALS_FULL_URI in EcsCredentialProvider.php, this will allows aws-php-sdk to works with CloudShell correctly. Permission in Cloudshell to follow IAM User.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.