Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(ecr): only set credentials if username & password is specified, refactor to use aws-sdk v3 #128

Merged
merged 1 commit into from Dec 20, 2021
Merged

fix(ecr): only set credentials if username & password is specified, refactor to use aws-sdk v3 #128

merged 1 commit into from Dec 20, 2021

Conversation

Flydiverny
Copy link
Contributor

@Flydiverny Flydiverny commented Dec 20, 2021 •
edited

Related to #126

I was thinking maybe we could use the v3 aws-sdk lib instead of the v2 one, ie @aws-sdk/client-ecr and @aws-sdk/client-ecr-public. I tried to make these changes locally but for some reason docker buildx bake validation fails saying there's diffs to output. Seems to be CRLF vs LF issue 馃槄
It seemed to reduce the dist size a bit since v3 sdk is modular.

@Flydiverny Flydiverny mentioned this pull request Dec 20, 2021
Merged
@crazy-max
Copy link
Member

@Flydiverny

I tried to make these changes locally but for some reason docker buildx bake validation fails saying there's diffs to output.

Can you prune your cache just in case? docker buildx prune -f. Also rebase.

@Flydiverny Flydiverny force-pushed the aws-sdk-v3 branch 3 times, most recently from 9a70db5 to ce8a232 Compare December 20, 2021 10:53
@Flydiverny
Copy link
Contributor Author

@Flydiverny

I tried to make these changes locally but for some reason docker buildx bake validation fails saying there's diffs to output.

Can you prune your cache just in case? docker buildx prune -f. Also rebase.

Tried nuking my docker cache and also docker buildx prune -f (thanks) but still end up with the same issue 馃槙 it worked fine with the other PR 馃槄
Tried to print the diff and it some kind of whitespace diff.
Made a fresh clone and checked out this PR again and ran docker buildx bake build

image

Disabled autocrlf and I was able to stag it and force push

I'm running on WSL

@Flydiverny Flydiverny marked this pull request as ready for review December 20, 2021 11:02
@Flydiverny
Copy link
Contributor Author

Interestingly enough its just one part of the file that had line changes e307f4e

@Flydiverny Flydiverny marked this pull request as draft December 20, 2021 11:08
@Flydiverny
Copy link
Contributor Author

Flydiverny commented Dec 20, 2021 •
edited

Marked as draft again as it failed manual test on our runner. Will have to pause this for now!
Master works as expected from my test (for my use case of no credentials provided)! :)

crazy-max pushed a commit that referenced this pull request Dec 20, 2021
@Flydiverny
Pull Request #128: refactor: use aws-sdk v3 sdk
0209f82
@Flydiverny
refactor: use v3 sdk
5885569 
Signed-off-by: Markus Maga <markus@maga.se>
@Flydiverny Flydiverny marked this pull request as ready for review December 20, 2021 12:49
crazy-max
crazy-max reviewed Dec 20, 2021
View reviewed changes
src/aws.ts
Comment on lines +57 to +63
const credentials =
username && password
? {
accessKeyId: username,
secretAccessKey: password
}
: undefined;
Copy link
Member

Choose a reason for hiding this comment

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

You beat me to it, this should fix #129

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! 馃槃 just saw the test branch :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should allow it to fallback to the default credential chain that the aws-sdk uses

@Flydiverny Flydiverny changed the title refactor: use aws-sdk v3 sdk fix(ecr): only set credentials if username & password is specified, refactor to use aws-sdk v3 Dec 20, 2021
crazy-max
crazy-max approved these changes Dec 20, 2021
View reviewed changes
Copy link
Member

@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@crazy-max crazy-max merged commit 1d7d864 into docker:master Dec 20, 2021
@Flydiverny Flydiverny deleted the aws-sdk-v3 branch December 20, 2021 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazy-max crazy-max crazy-max approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Flydiverny @crazy-max