bz1373281 allow >2 slashes in docker URI #594
Conversation
jim-minter
force-pushed
the
bz1373281
branch
from
ec3b41c to
b6964cd
Compare
| default: | ||
| return ref, fmt.Errorf("the docker pull spec %q must be two or three segments separated by slashes", spec) | ||
| // registry(/dir)+/name, we suppose | ||
| ref.Registry = nameParts[0] |
There was a problem hiding this comment.
how do we safely make this assumption? should we check it for : and . like above?
(are : or . valid characters in a docker image path segment?)
There was a problem hiding this comment.
I don't know how to make the assumption more safe. ":" is not a valid character in a docker image path segment but is used for hostnames and tags. Unfortunately "." is a valid character in a docker image path segment; nevertheless this heuristic is used today in the openshift code and it seemed plausible to copy it.
There is an element of guesswork to this. The previous version of the code made the same assumption here as this version does.
Technically, I still think there's an argument to using "github.com/docker/distribution/reference" as the canonical parser so we match docker behaviour - principle of least surprise
|
@bparees I've ended up testing the registry in the 2x'/' case as you suggested above - ptal |
| if strings.ContainsAny(nameParts[0], ":.") || nameParts[0] == "localhost" { | ||
| // registry/name, we suppose | ||
| ref.Registry = nameParts[0] | ||
| ref.Name = "library/" + nameParts[1] |
There was a problem hiding this comment.
ah, i see, ref.name will stay as it came out of parseRepoTag.
There was a problem hiding this comment.
but don't we still need to add "library" in the case where "ref.Name" is something like "mysql"?
| if strings.ContainsAny(nameParts[0], ":.") || nameParts[0] == "localhost" { | ||
| // registry(/dir)+/name, we suppose | ||
| ref.Registry = nameParts[0] | ||
| ref.Name = strings.Join(nameParts[1:], "/") |
There was a problem hiding this comment.
a 1 segment name that's just a registry can't be valid, can it?
|
@bparees ok, here's a different attempt, which is actually to remove the ParseImageReference function entirely and use the docker registry functions... |
| newBuilderImage = fmt.Sprintf("%s%s/", newBuilderImage, dockerImageReference.Namespace) | ||
| } | ||
| newBuilderImage = fmt.Sprintf("%s%s:s2i-layered-%d", newBuilderImage, dockerImageReference.Name, time.Now().UnixNano()) | ||
| newBuilderImage = fmt.Sprintf("%s-%d", builder.config.BuilderImage, time.Now().UnixNano()) |
There was a problem hiding this comment.
can you see a reason not to include "s2i-layered" in the name here like we do in the other path?
There was a problem hiding this comment.
is there a good reason not to just use namedReference.Name() as the base of the new image name in all cases, regardless of whether it's a "digest" or not?
There was a problem hiding this comment.
I don't think so - updated.
|
squash and lgtm |
…function and use %s:s2i-layered-%d for layered images
jim-minter
force-pushed
the
bz1373281
branch
from
3763783 to
6f064a8
Compare
|
Squashed! |
|
[merge] |
|
Evaluated for source to image test up to 6f064a8 |
|
Evaluated for source to image merge up to 6f064a8 |
|
Source To Image Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_sti/238/) |
|
Source To Image Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_sti/237/) |
@bparees ptal