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

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

Conversation

Flydiverny
Copy link
Contributor

@Flydiverny Flydiverny commented Dec 20, 2021

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.

@crazy-max
Copy link
Member

crazy-max commented Dec 20, 2021

@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 Dec 20, 2021
@Flydiverny
Copy link
Contributor Author

Flydiverny commented Dec 20, 2021

@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 Dec 20, 2021
@Flydiverny Flydiverny requested a review from crazy-max as a code owner Dec 20, 2021
@Flydiverny
Copy link
Contributor Author

Flydiverny commented Dec 20, 2021

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

@Flydiverny Flydiverny marked this pull request as draft Dec 20, 2021
@Flydiverny
Copy link
Contributor Author

Flydiverny commented Dec 20, 2021

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
Signed-off-by: Markus Maga <markus@maga.se>
@Flydiverny Flydiverny marked this pull request as ready for review Dec 20, 2021
const credentials =
username && password
? {
accessKeyId: username,
secretAccessKey: password
}
: undefined;
Copy link
Member

@crazy-max crazy-max Dec 20, 2021

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

@Flydiverny Flydiverny Dec 20, 2021

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

@Flydiverny Flydiverny Dec 20, 2021

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
Copy link
Member

@crazy-max crazy-max left a comment

LGTM thanks!

@crazy-max crazy-max merged commit 1d7d864 into docker:master Dec 20, 2021
2 checks passed
@Flydiverny Flydiverny deleted the aws-sdk-v3 branch Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants