-
Notifications
You must be signed in to change notification settings - Fork 428
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: support amazon-ecr-credential-helper & check for bucket existence before emptying the bucket #2365
Conversation
…helper aws#2323 Depending on the value of `credsStore` attribute (inside docker config file), the code either performs the `docker login` or skips it.
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.
Thank you so much for making this change! This is awesome! Just had some nits and a quick question.
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.
Awesome, this is so good thank you so much for fixing this issue 😍 !
Some suggestions for enabling writing unit tests for the new functionality 🙏
Co-authored-by: Efe Karakus <[email protected]>
Co-authored-by: Penghao He <[email protected]>
Co-authored-by: Efe Karakus <[email protected]>
Based on the documentation available [here](https://github.com/awslabs/amazon-ecr-credential-helper), `ecr-login` can be enabled globally using `credsStore` or on individual registry level. The recent code update considers both of this scenarios before letting it perform `docker login`
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.
Just some nits. Other than that lgtm!
Co-authored-by: Penghao He <[email protected]>
Co-authored-by: Penghao He <[email protected]>
Thanks I have accepted the suggestions, appreciate the support on 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.
Looks great thank you so much! And I'm sorry for the delay on the review 🙇
Just small requests to flip the if-statement to reduce nesting and then the PR should auto-merge once we remove the label.
…e before emptying the bucket (aws#2365) ### Summary aws#2323 - Depending on the value of `credsStore` attribute (inside docker config file), the code either performs the `docker login` or skips it. aws#2070 - Added code to check for the existence of the bucket using `s3.HeadBucket` API before proceed to empty the contents ### Issue number Fixes aws#2323 & aws#2070 ---- By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Summary
#2323 - Depending on the value of
credsStore
attribute (inside docker config file), the code either performs thedocker login
or skips it.#2070 - Added code to check for the existence of the bucket using
s3.HeadBucket
API before proceed to empty the contentsIssue number
Fixes #2323 & #2070
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.