Skip to content

Conversation

@kuettai
Copy link
Contributor

@kuettai kuettai commented Mar 14, 2022

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.

@kuettai
Copy link
Contributor Author

kuettai commented Mar 14, 2022

ATTN: @stobrien89

Previous PR #2324 , changes made to the comment.
Would also solve PR #2249 as per highlighted in PR #2324

@stobrien89
Copy link
Member

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 stobrien89 self-requested a review March 14, 2022 15:04
@stobrien89 stobrien89 self-assigned this Mar 14, 2022
@stobrien89 stobrien89 added the dev-review This issue/pr needs review from an internal developer. label Mar 14, 2022
Copy link
Member

@stobrien89 stobrien89 left a 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.

Copy link
Member

@stobrien89 stobrien89 left a 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

Copy link
Member

@stobrien89 stobrien89 left a 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

Copy link
Member

@stobrien89 stobrien89 left a 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.

Copy link
Member

@stobrien89 stobrien89 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending team review

@SamRemis SamRemis self-requested a review March 29, 2022 17:04
@stobrien89
Copy link
Member

Hi @kuettai and @BenHarris,

Thanks again for your work on this. Tested manually using docker as well as cloudshell and everything works as expected.

@stobrien89 stobrien89 merged commit 6ecbc87 into aws:master Mar 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dev-review This issue/pr needs review from an internal developer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants