Add image provider for ECR #1213
Conversation
|
Code coverage for golang is
|
|
|
||
| "github.com/aws/aws-sdk-go/aws" | ||
| "github.com/aws/aws-sdk-go/service/ecr" | ||
| "github.com/magiconair/properties/assert" |
pkg/semver/semver.go
Outdated
| "strconv" | ||
| "strings" | ||
| ) | ||
|
|
There was a problem hiding this comment.
What are modifications compared to the original code?
There was a problem hiding this comment.
Only the difference from the original is sort.go, for sorting by newer.
The main reason why I borrowed the code instead of import is to avoid being dependent. It has tiny code so I thought copying is enough.
There was a problem hiding this comment.
I think if the code was not modified so instead of copying it into our source base, importing it via go-mod is better (as other libs).
Updating to the new version of it becomes easier to do.
Btw, sort.go can be removed by using https://golang.org/pkg/sort/#Slice to sort the slice.
There was a problem hiding this comment.
Thanks. Okay, I'd be happy to import it and use sort.Slice :-)
|
I found a significant issue. |
|
Code coverage for golang is
|
|
The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by You can check the build log from here. |
|
Code coverage for golang is
|
|
/golinter trigger |
|
Code coverage for golang is
|
| } else { | ||
| cfg = cfg.WithCredentials(credentials.NewEnvCredentials()) | ||
| } | ||
| sess := session.Must(session.NewSession()) |
There was a problem hiding this comment.
Instead of calling Must that causes a panic when an error occurs, returning that error.
| // Returns an error if one of any tag couldn't be parsed. | ||
| func latestBySemver(ids []*ecr.ImageIdentifier) (string, error) { | ||
| length := len(ids) | ||
| if length == 0 { |
There was a problem hiding this comment.
This does not need to be checked. Or an error should be returned.
| input.RegistryId = &e.registryID | ||
| } | ||
|
|
||
| // TODO: Consider the way to determine the latest tag other than fetching all tags |
There was a problem hiding this comment.
Or instead of checking by version, caching the image metadata in the memory cache to reduce the API calls.
(Not sure our clients follow the convention of semantic versioning.)
There was a problem hiding this comment.
Do they have any options for returning the sorted data by the created date? I think no. lol.
There was a problem hiding this comment.
Do they have any options for returning the sorted data by the created date? I think no. lol.
As you assumed, it doesn't ensure it, haha.
caching the image metadata in the memory cache
Sorry if this sounds dumb, does it mean caching the image digest and compare the latest image's, then start fetching all tags if there is any deviation?
|
And I found all tags can be given back if we pass no tags to |
|
@nakabonne lol. Right, the |
|
Code coverage for golang is
|
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Consider the way to determine the latest tag other than fetching all tagsThis was created by todo plugin since "TODO:" was found in 27e8f9a when #1213 was merged. cc: @nakabonne. |
|
@nghialv Changed to call |
|
Code coverage for golang is
|
|
@nakabonne I found the And at this thread they were talking about this magic: |
| // The maximum number of image results returned by the APIs about images. | ||
| // The API allows this value to be between 1 and 1000. | ||
| // See more: https://pkg.go.dev/github.com/aws/aws-sdk-go/service/ecr#ListImagesInput | ||
| const maxResults = 1000 |
There was a problem hiding this comment.
If the returned list was already sorted, we can decrease this number.
Not sure but I think they are already sorted so can you make a real request to check their order?
There was a problem hiding this comment.
you are right, actually i did. as far as i tried, they seem to be sorted by older (means the first value is always the oldest).
There was a problem hiding this comment.
anyway, seems to be needed to gain all tags. (i know this isn't the best, but i decided to make this a future task because it would require some hack as long as any official API wasn't provided.
There was a problem hiding this comment.
btw, flux actually does fetch all tags to determine the latest as well.
There was a problem hiding this comment.
Please forgive me it is all lowercase due to the smartphone.
There was a problem hiding this comment.
they seem to be sorted by older (means the first value is always the oldest).
😄 Got it. Thanks for your explaination.
|
yep, i saw them and have given it a try! but that just does sort the results locally after fetching... |
|
Great job! |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
This PR does:
semverpackage to parse and sort tags that is according to Semantic Versiong.This mainly aims to let
GetLatestImagework properly to determine the latest tag. Hopefully AWS provides the API to give back the latest tag in the specified repository. But real-world is not so easy. As far as I investigate, the only way to determine the latest tag is:To avoid reaching the API rate limit, it attempts to determine by the semantic versioning as much as possible.
The way to list all tags doesn't scale, so I left a TODO comment and will be a future issue.
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: