✨Add a library to parse container image name and other fields #2869
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/L
|
/milestone v0.3.4 We'll need to follow-up with more PRs to refactor wherever else we use the /assign @benmoss |
|
@vincepri there is only a couple of places using the reference library, I can include them in this PR |
|
/hold making some more change to this PR to apply the change to other places in the code using these functions |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
size/XL
| @@ -119,53 +118,20 @@ func (i *imageMeta) Union(other *imageMeta) { | |||
|
|
|||
| // ApplyToImage changes an image name applying the transformations defined in the current imageMeta. | |||
| func (i *imageMeta) ApplyToImage(image string) (string, error) { | |||
There was a problem hiding this comment.
cc @fabriziopandini @wfernandes
to make sure we have tests and the logic looks good
There was a problem hiding this comment.
In imagemeta_client_test.go there were no specific unit tests for ApplyToImage but there were tests for AlterImage which calls out to ApplyImage. That being said I see that a lot of tests were added for ImageFromString. 👍
|
/hold remove |
| @@ -119,53 +118,20 @@ func (i *imageMeta) Union(other *imageMeta) { | |||
|
|
|||
| // ApplyToImage changes an image name applying the transformations defined in the current imageMeta. | |||
| func (i *imageMeta) ApplyToImage(image string) (string, error) { | |||
There was a problem hiding this comment.
In imagemeta_client_test.go there were no specific unit tests for ApplyToImage but there were tests for AlterImage which calls out to ApplyImage. That being said I see that a lot of tests were added for ImageFromString. 👍
| // ImageFromString parses a docker image string into three parts: repo, tag and digest. | ||
| // If both tag and digest are empty, a default image tag will be returned. | ||
| func ImageFromString(image string) (Image, error) { | ||
| named, err := reference.ParseNamed(image) |
There was a problem hiding this comment.
In the previous implementation, we used reference.ParseNormalizedNamed(image). Is there some essential difference between ParseNormalizedNamed and ParseName?
There was a problem hiding this comment.
ParseName does more validation
There was a problem hiding this comment.
Good catch. ParseNormalizedNamed will allow for "Docker hub"-style names, ala busybox:latest. ParseNamed will error ErrNameNotCanonical on those. I don't know if we want to support images like that.
There was a problem hiding this comment.
I found a few cases with checks to make sure we don't get ErrNameNotCanonical so I assumed its better to use ParsedName
There was a problem hiding this comment.
I think it's better just that it forces people to be explicit, the Docker default registry nonsense is terrible
There was a problem hiding this comment.
agreed, I took the default out
| }) | ||
| } | ||
|
|
||
| func TestModifyImageTag(t *testing.T) { |
There was a problem hiding this comment.
Feels like we can add more tests here. WDYT?
|
|
||
| // ModifyImageTag takes an imageName (e.g., repository/image:tag), and returns an image name with updated tag | ||
| func ModifyImageTag(imageName, tagName string) (string, error) { | ||
| normalisedTagName := SemverToOCIImageTag(tagName) |
There was a problem hiding this comment.
Should we be calling ImageTagIsValid in this function? Is any tag validation happening in the reference library?
There was a problem hiding this comment.
i think validation is happening in the reference library
- use func in util pkg instead of exteral docker reference library to simplify the code
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
/test pull-cluster-api-capd-e2e |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nader-ziada, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #2850