Use distribution's reference.ParseDockerRef#2986
Conversation
…09655cc580 Signed-off-by: Sebastiaan van Stijn <[email protected]>
Using the cri utility caused other project to have containerd/cri as a dependency, only for this utility. The new `reference.ParseDockerRef` function does the same (other than having a different name). Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
ping @dmcgowan @Random-Liu PTAL I'll open a cherry-pick for the release/1.2 branch if this is accepted and merged 👍 /cc @silvin-lubecki |
|
could someone restart travis-ci? Don't think it should be related to my changes (stalled at the vendor step) |
| @@ -69,7 +69,7 @@ func isImagePrefix(s, prefix string) bool { | |||
|
|
|||
| func normalizeReference(ref string) (string, error) { | |||
| // TODO: Replace this function to not depend on reference package | |||
There was a problem hiding this comment.
I think this comment makes us not want to depend on this package?
There was a problem hiding this comment.
Confusing given that the CRI utility function definitely depends on the reference package as well. @dmcgowan ?
There was a problem hiding this comment.
Yes, this is not the "final state" of this I guess, but this was at least a step to prevent introducing get rid of an additional dependency in the CLI.
From distribution/distribution#2786 (comment)
Just as a note, I want to split the reference package out of this repository and explicitly have a
Dockerreferencing package for all projects to use which make use of this referencing logic. Ironically this reference package is least useful to this repository since the reference format used by the registry can be vastly simplified as a superset of what Docker uses. I don't have time to refactor this, but it is not harmful to have this functionality along side all the other Docker-specific normalization functions.
There was a problem hiding this comment.
I still agree with this comment, but I also agree with @thaJeztah about cutting out the middleman dependency. This makes it much more clear where we need to replace this functionality, in both CRI and containerd.
Codecov Report
@@ Coverage Diff @@
## master #2986 +/- ##
==========================================
+ Coverage 41.18% 43.96% +2.78%
==========================================
Files 71 102 +31
Lines 9531 10881 +1350
==========================================
+ Hits 3925 4784 +859
- Misses 5044 5362 +318
- Partials 562 735 +173
Continue to review full report at Codecov.
|
|
LGTM |
5 participants
Using the cri utility caused other project to have containerd/cri as a dependency, only for this utility.
The new
reference.ParseDockerReffunction does the same (other than having a different name).relates to distribution/distribution#2786 and docker/cli#1561 (comment)