-
Notifications
You must be signed in to change notification settings - Fork 82
Add support for public ecr #391
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
base: main
Are you sure you want to change the base?
Changes from all commits
c8be71c
3bd6b18
eaf2aa7
16e18e7
d33d50c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ import ( | |||||
| "github.com/jetstack/version-checker/pkg/client/acr" | ||||||
| "github.com/jetstack/version-checker/pkg/client/docker" | ||||||
| "github.com/jetstack/version-checker/pkg/client/ecr" | ||||||
| "github.com/jetstack/version-checker/pkg/client/ecrpublic" | ||||||
| "github.com/jetstack/version-checker/pkg/client/fallback" | ||||||
| "github.com/jetstack/version-checker/pkg/client/gcr" | ||||||
| "github.com/jetstack/version-checker/pkg/client/ghcr" | ||||||
|
|
@@ -39,6 +40,7 @@ type Options struct { | |||||
| ACR acr.Options | ||||||
| Docker docker.Options | ||||||
| ECR ecr.Options | ||||||
| ECRPublic ecrpublic.Options // Add ECRPublic options | ||||||
| GCR gcr.Options | ||||||
| GHCR ghcr.Options | ||||||
| OCI oci.Options | ||||||
|
|
@@ -54,6 +56,7 @@ func New(ctx context.Context, log *logrus.Entry, opts Options) (*Client, error) | |||||
| if opts.Transport != nil { | ||||||
| opts.Quay.Transporter = opts.Transport | ||||||
| opts.ECR.Transporter = opts.Transport | ||||||
| opts.ECRPublic.Transporter = opts.Transport // Add ECR public transporter | ||||||
|
||||||
| opts.ECRPublic.Transporter = opts.Transport // Add ECR public transporter | |
| opts.ECRPublic.Transporter = opts.Transport |
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,152 @@ | ||||||||||||||||
| package ecrpublic | ||||||||||||||||
|
|
||||||||||||||||
| import ( | ||||||||||||||||
| "context" | ||||||||||||||||
| "encoding/json" | ||||||||||||||||
| "errors" | ||||||||||||||||
| "fmt" | ||||||||||||||||
| "io" | ||||||||||||||||
| "net/http" | ||||||||||||||||
| "time" | ||||||||||||||||
|
|
||||||||||||||||
| "github.com/sirupsen/logrus" | ||||||||||||||||
|
|
||||||||||||||||
| retryablehttp "github.com/hashicorp/go-retryablehttp" | ||||||||||||||||
| "github.com/jetstack/version-checker/pkg/api" | ||||||||||||||||
| "github.com/jetstack/version-checker/pkg/client/util" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| const ( | ||||||||||||||||
| ecrPublicLookupURL = "https://public.ecr.aws/v2/%s/%s/tags/list" | ||||||||||||||||
| loginURL = "https://public.ecr.aws/token/?service=ecr-public" | ||||||||||||||||
| ) | ||||||||||||||||
|
|
||||||||||||||||
| type Options struct { | ||||||||||||||||
| Username string | ||||||||||||||||
| Password string | ||||||||||||||||
| Transporter http.RoundTripper | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
| type Client struct { | ||||||||||||||||
| *http.Client | ||||||||||||||||
| Options | ||||||||||||||||
| } | ||||||||||||||||
|
|
||||||||||||||||
|
||||||||||||||||
| var _ api.ImageClient = (*Client)(nil) |
Copilot
AI
Dec 9, 2025
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.
When image is empty (as in the test case "nginx" where repo="nginx" and image=""), the URL will be constructed as https://public.ecr.aws/v2/nginx//tags/list with a double slash. This may not be the correct format for the ECR Public API. Verify that this URL format works correctly, or handle the case where image is empty by constructing the URL differently, e.g., https://public.ecr.aws/v2/nginx/tags/list.
| url := fmt.Sprintf(ecrPublicLookupURL, repo, image) | |
| var url string | |
| if image == "" { | |
| url = fmt.Sprintf("https://public.ecr.aws/v2/%s/tags/list", repo) | |
| } else { | |
| url = fmt.Sprintf(ecrPublicLookupURL, repo, image) | |
| } |
Copilot
AI
Dec 9, 2025
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.
The error message formatting is inconsistent with the error returned from getAnonymousToken. The error should use %w instead of %s to properly wrap the error, allowing error unwrapping with errors.Is() and errors.As(). Change to: fmt.Errorf("failed to get anonymous token: %w", err)
| return nil, fmt.Errorf("failed to get anonymous token: %s", err) | |
| return nil, fmt.Errorf("failed to get anonymous token: %w", err) |
Copilot
AI
Dec 9, 2025
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.
Similarly, this error should use %w instead of %s to properly wrap the error. Change to: fmt.Errorf("failed to get %q image: %w", c.Name(), err)
| return nil, fmt.Errorf("failed to get %q image: %s", c.Name(), err) | |
| return nil, fmt.Errorf("failed to get %q image: %w", c.Name(), err) |
Copilot
AI
Dec 9, 2025
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.
The doRequest function doesn't check the HTTP status code before attempting to unmarshal the response body. If the API returns an error status (e.g., 404, 401, 500), the function will try to unmarshal the error response as a TagResponse, which will likely fail with a confusing error message. Consider adding a status code check similar to getAnonymousToken at line 142:
if resp.StatusCode != http.StatusOK {
return nil, fmt.Errorf("failed to get tags, status code: %d, body: %s", resp.StatusCode, body)
}
Copilot
AI
Dec 9, 2025
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.
The main client functionality in ecrpublic.go lacks test coverage. While path_test.go tests IsHost() and RepoImageFromPath(), there are no tests for the core functions Tags(), doRequest(), and getAnonymousToken(). Other client implementations like docker and ghcr include comprehensive tests for these core functions. Consider adding tests similar to those in pkg/client/docker/docker_test.go that use httptest to mock API responses.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package ecrpublic | ||
|
|
||
| import ( | ||
| "regexp" | ||
| "strings" | ||
| ) | ||
|
|
||
| var ( | ||
| ecrPublicPattern = regexp.MustCompile(`^public\.ecr\.aws$`) | ||
| ) | ||
|
|
||
| func (c *Client) IsHost(host string) bool { | ||
| return ecrPublicPattern.MatchString(host) | ||
| } | ||
|
|
||
| func (c *Client) RepoImageFromPath(path string) (string, string) { | ||
| parts := strings.Split(path, "/") | ||
|
|
||
| return parts[0], strings.Join(parts[1:], "/") | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,95 @@ | ||||||
| package ecrpublic | ||||||
|
|
||||||
| import "testing" | ||||||
|
|
||||||
| func TestIsHost(t *testing.T) { | ||||||
| tests := map[string]struct { | ||||||
| host string | ||||||
| expIs bool | ||||||
| }{ | ||||||
| "an empty host should be false": { | ||||||
| host: "", | ||||||
| expIs: false, | ||||||
| }, | ||||||
| "random string should be false": { | ||||||
| host: "foobar", | ||||||
| expIs: false, | ||||||
| }, | ||||||
| "path with two segments should be false": { | ||||||
| host: "joshvanl/version-checker", | ||||||
| expIs: false, | ||||||
| }, | ||||||
| "path with three segments should be false": { | ||||||
| host: "jetstack/joshvanl/version-checker", | ||||||
| expIs: false, | ||||||
| }, | ||||||
| "random string with dots should be false": { | ||||||
| host: "foobar.foo", | ||||||
| expIs: false, | ||||||
| }, | ||||||
| "docker.io should be false": { | ||||||
| host: "docker.io", | ||||||
| expIs: false, | ||||||
| }, | ||||||
| "docker.com should be false": { | ||||||
| host: "docker.com", | ||||||
| expIs: false, | ||||||
| }, | ||||||
| "just public.ecr.aws should be true": { | ||||||
| host: "public.ecr.aws", | ||||||
| expIs: true, | ||||||
| }, | ||||||
| "public.ecr.aws.foo should be false": { | ||||||
| host: "public.ecr.aws.foo", | ||||||
| expIs: false, | ||||||
| }, | ||||||
| "foo.public.ecr.aws should be false": { | ||||||
| host: "foo.public.ecr.aws", | ||||||
| expIs: false, | ||||||
| }, | ||||||
| } | ||||||
|
|
||||||
| handler := new(Client) | ||||||
| for name, test := range tests { | ||||||
| t.Run(name, func(t *testing.T) { | ||||||
| if isHost := handler.IsHost(test.host); isHost != test.expIs { | ||||||
| t.Errorf("%s: unexpected IsHost, exp=%t got=%t", | ||||||
| test.host, test.expIs, isHost) | ||||||
| } | ||||||
| }) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| func TestRepoImageFromPath(t *testing.T) { | ||||||
| tests := map[string]struct { | ||||||
| path string | ||||||
| expRepo, expImage string | ||||||
| }{ | ||||||
| "single image should return registry and image": { | ||||||
|
||||||
| "single image should return registry and image": { | |
| "single segment path should return repo with empty image": { |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| package ecrpublic | ||
|
|
||
| type AuthResponse struct { | ||
| Token string `json:"token"` | ||
| } | ||
|
|
||
| type TagResponse struct { | ||
| Next string `json:"next"` | ||
| Name string `json:"name"` | ||
| Tags []string `json:"tags"` | ||
| } |
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.
[nitpick] Remove the inline comment "Add ECRPublic options". Comments that simply describe what the code obviously does add no value and should be removed. The field name
ECRPublicis self-documenting.