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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix not implemented errors #575

Closed
wants to merge 1 commit into from
Closed

fix not implemented errors #575

wants to merge 1 commit into from

Conversation

merlinran
Copy link

Issue #, if available:
Closes #574
Closes #474
Closes #316

Description of changes:
The current code assumes some methods are not called, which is no longer true.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@merlinran merlinran requested review from a team as code owners July 23, 2023 21:14
sparr
sparr previously approved these changes Jul 24, 2023
@sparr sparr dismissed their stale review July 24, 2023 17:56

Mistaken review

@sparr
Copy link
Contributor

sparr commented Jul 24, 2023

Simply returning no error when a caller would expect functionality to be triggered does not seem appropriate. Can you elaborate on the current use cases for these methods, to make review smoother?

@ginglis13
Copy link
Contributor

Simply returning no error when a caller would expect functionality to be triggered does not seem appropriate. Can you elaborate on the current use cases for these methods, to make review smoother?

Agreed. The ECR credential helper is intended to be used instead of login and logout. I think silently returning no error is not enough here, I'd suggest keeping the error return and adding a warning log statement that this tool is intended to be set up as a credential helper in ~/.docker/config.json (or ~/.finch/config.json if using finch)

See https://github.com/awslabs/amazon-ecr-credential-helper#troubleshooting and https://github.com/awslabs/amazon-ecr-credential-helper#usage

@merlinran
Copy link
Author

Sorry I thought it's what prevented my ecr-login from working. It turns out to be a different issue. I noticed the error message during troubleshooting, then came to the conclusion too quickly.

As a side note, for anyone who by chance lands here, what prevented ecr-login from working for docker 24.0.4 with containerd-snapshotter enabled was because of the following:

time="2023-07-23T21:28:08.336275463Z" level=warning msg="Host doesn't match" cfgHost=registry-1.docker.io host=xxx.amazonaws.com
time="2023-07-23T21:28:08.342797167Z" level=error msg="Handler for POST /v1.43/images/create returned error: failed to resolve reference \"xxx.amazonaws.com/<image>:latest\": pulling from host xxx.amazonaws.com failed with status code [manifests latest]: 401 Unauthorized"

and what fixed it for me was with this in ~/.docker/config.json:

{
	"auths": {
		"xxx.amazonaws.com": {}  <=== has to add this to avoid the "Host doesn't match" warning
	},
	"credHelpers": {
		"xxx.amazonaws.com": "ecr-login"
	}
}

@merlinran merlinran closed this Jul 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment