-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Allow multiple segments in DockerImageReference #11173
Allow multiple segments in DockerImageReference #11173
Conversation
599eea9
to
d5005ae
Compare
@miminar PTAL |
stream, tag, id := parseRepositoryTag(spec) | ||
|
||
repoParts := strings.Split(stream, "/") | ||
if len(repoParts) < 4 { |
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.
explain the magic 4 in a comment?
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.
Right now we can have maximum 3 part (registry/namespace/name
). Multi-segment path means 4 or more. So, old behavior will be in case of < 4.
|
||
ref.Name = strings.Join(repoParts[2:], "/") | ||
if len(ref.Name) == 0 { | ||
return ref, fmt.Errorf("the docker pull spec %q must be more than three segments separated by slashes", spec) |
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.
"more than three" or "three or more"?
testCases := []struct { | ||
From string | ||
Registry, Namespace, Name, Tag, ID string | ||
Err bool | ||
MSegErr bool |
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.
this is hard to follow, just duplicate the cases and make the parsing function one of the inputs to the test
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.
I wanted to make sure that all the tests pass by both functions except for some testcases and wanted to quickly find out which tests are different.
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.
I agree with Jordan - more explicit tests are better. Use a common core array and add variant cases.
// ParseDockerImageReference parses a Docker pull spec string into a | ||
// DockerImageReference. | ||
func ParseDockerImageReference(spec string) (DockerImageReference, error) { | ||
// TODO replace with docker version once docker/docker PR11109 is merged upstream |
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.
I think it's about time to resolve this. moby/moby#11109 was marched in March 2015.
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.
Yes, but the parser is very different from what we do. I tried it.
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.
If you think it's not worth it, just delete the comment. On the other hand, I'd like to have a way to easily turn our DockerImageReference
into upstream's reference.Reference
.
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.
@miminar No, I mean that must be a completely different PR because we have some additional magic guessing (https://github.com/openshift/origin/blob/master/pkg/image/api/helper.go#L142-L156) and restriction how many components we can accept. So, we can't use reference.Reference
without extra work anyway.
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.
Please document the special behavior in the godoc for these methods - anything this fundamental should be documented well. Please include the official spec, and each point where we differ.
|
||
// registry/namespace/name/namecomponent/namecomponent/... | ||
if strings.HasSuffix(repoParts[0], ":") || IsRegistryDockerHub(repoParts[0]) { | ||
return ref, fmt.Errorf("the docker pull spec %q must be two or three segments separated by slashes", spec) |
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 looks outdated.
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.
No. This is the error message that was returned by ParseDockerImageReference
in same case.
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 contradicts a comment two lines above.
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.
Will be OK if I put comment // http://registry/namespace/name
inside if
?
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.
I don't quite understand, why a function that is supposed to parse a reference with way more than 3 segments would ever return "the docker pull spec %q must be two or three segments separated by slashes"
. The error statement just isn't right.
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.
@miminar because this case for http://example.com/namespace/name
.
But I just realized that I can't act like that and in case of specifying of schema I need to parse the rest path. Thanks!
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.
@miminar I was wrong again. We don't support schema in DockerImageReference
:
https://github.com/openshift/origin/blob/master/pkg/image/api/helper_test.go#L290-L291
https://github.com/openshift/origin/blob/master/pkg/image/api/helper_test.go#L298-L299
// ParseDockerImageReference parses a Docker pull spec string into a | ||
// DockerImageReference. | ||
func ParseDockerImageReference(spec string) (DockerImageReference, error) { | ||
func parseDockerImageReference(repoParts []string, spec string) (DockerImageReference, error) { |
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.
Having two different functions with the same name looks suspicious or at least redundant.
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.
OK. I will change it to something more common.
} | ||
ref.Registry = repoParts[0] | ||
|
||
if len(repoParts[1]) == 0 { |
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.
Can this be applied to all the name components?
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.
@miminar I'm not sure I understand.
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.
I mean all the name components shall be not empty. Why not checking them all in a loop?
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.
Yes, I miss check for ref.Registry
. Thanks! I'll fix it.
@@ -56,7 +56,7 @@ func validateImage(image *api.Image, fldPath *field.Path) field.ErrorList { | |||
if len(image.DockerImageReference) == 0 { | |||
result = append(result, field.Required(fldPath.Child("dockerImageReference"), "")) | |||
} else { | |||
if _, err := api.ParseDockerImageReference(image.DockerImageReference); err != nil { | |||
if _, err := api.ParseMultiSegmentsDockerImageReference(image.DockerImageReference); err != nil { |
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.
What about parsing docker image reference in ValidateImageStream()
.
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.
@miminar ValidateImageStream
must be unmodified.
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.
Also this is a good candidate for a new unit test.
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.
@miminar I'll do more tests when this approach will be approved by @liggitt, and @smarterclayton and you, because I'm not sure that we want to make these changes.
@@ -394,7 +394,7 @@ func validateFromImageReference(reference *kapi.ObjectReference, fldPath *field. | |||
} | |||
if len(name) == 0 { | |||
allErrs = append(allErrs, field.Required(fldPath.Child("name"), "")) | |||
} else if _, err := imageapi.ParseDockerImageReference(name); err != nil { | |||
} else if _, err := imageapi.ParseMultiSegmentsDockerImageReference(name); err != nil { |
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.
It seems to me like we need to update validateToImageReference
as well.
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.
@miminar I was not sure about that. I will fix it.
Could this add an integration test which would launch a fake registry server serving manifests from multi-slash repositories which we would import? |
return ref, err | ||
} | ||
|
||
func ParseMultiSegmentsDockerImageReference(spec string) (DockerImageReference, error) { |
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.
This should not be public - we should not have two DockerImageReferences in the system. IF something is using ParseDockerImageReference that is not a valid DockerImageReference, we should create a new method which is "ParseNotQuiteDockerImageReference" (name can be better) that other code should call. DockerImageReferences are multi segment by default, which means ParseDockerImageReference should support multi-segment.
67cb80e
to
456e042
Compare
456e042
to
1a43a2e
Compare
// Handle multiple segments in form: registry/namespace/namesegment1/namesegment2/.../name | ||
// but not this form: http://registry/namespace/name. We don't support using the schema in | ||
// the DockerImageReference. | ||
if len(repoParts) > 3 && !strings.HasSuffix(repoParts[0], ":") { |
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.
Isn't this going to miss validating myregistry.com:5000/a/b/c?
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.
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.
Oh I see it now, thanks. Nevermind :-) But when would it have a suffix of :?
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.
http://example.com/foo/bar
If you split it by /
then first part (registry) will be http:
.
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.
That's not valid. We've never supported a scheme. Is that all you're testing the suffix for?
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.
Also hostname can't have :
at the end anyway.
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.
Right... I guess this is ok if we actually have users putting in http://
at the front of their pull specs...
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.
ref.Registry = repoParts[0] | ||
|
||
if len(repoParts[1]) == 0 { | ||
return ref, fmt.Errorf("the docker pull spec %q must contain non-empty namespace", spec) |
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.
Technically it's not a "namespace". It's just a "component". See https://github.com/docker/distribution/blob/744ae974a5bfc47c219320460eead041fc9f64f4/reference/reference.go#L7.
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.
Yes, but the text of the error made in the context of DockerImageReference.
Should I write something like:
the docker pull spec %q must be three or more segments separated by slashes
?
it seemed to me that it's less obvious in this case.
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.
I would prefer something that indicates that you can't have any empty segments/components. Also see my comment below about removing any specificity of number of segments.
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.
Ok. I will change comment.
|
||
for i, part := range repoParts[2:] { | ||
if len(part) == 0 { | ||
continue |
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.
Shouldn't this error?
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.
OK. I will change it.
ref.Name += part | ||
} | ||
if len(ref.Name) == 0 { | ||
return ref, fmt.Errorf("the docker pull spec %q must be three or more segments separated by slashes", spec) |
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.
I don't think this error message is accurate. It does not have to be three or more segments. In fact, for all of the error messages we return from this function, wouldn't it make more sense to just say it's not a valid pull spec, and if we can, give a reason why it failed to parse (registry is missing, segment can't be empty, etc). I think it might be time to remove the error text saying it has to be some specific # of segments.
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.
Please please please change the error text. I do not want it saying that it has to be three or more segments.
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.
@ncdc Actually this if
is useless because if have more than 3 parts in slice (repoParts
), two of them are not empty (Registry
and Namespace
) and rest of them are not empty as well after check inside for
. It means that Name
can't be empty.
I will remove this check.
From: "bar/foo/baz/biz", | ||
Err: true, | ||
From: "bar/foo/baz/biz", | ||
Registry: "bar", |
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.
Technically, I think if the docker hub ever starts supporting multiple components, this would resolve to registry=docker.io, namespace=bar, name=foo/baz/biz.
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.
That's right. See my above comment https://github.com/openshift/origin/pull/11173/files#r81712056
@@ -1269,7 +1269,7 @@ func TestValidateCommonSpec(t *testing.T) { | |||
Output: buildapi.BuildOutput{ | |||
To: &kapi.ObjectReference{ | |||
Kind: "DockerImage", | |||
Name: "some/long/value/with/no/meaning", | |||
Name: "index.docker.io/some/long/value/with/no/meaning", |
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.
Why these changes?
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.
it's changing because this is now a valid image spec... only by pinning it to dockerhub can we trigger the invalid error?
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.
Ah, right, at least for the hub. Although as I wrote in another comment, if/when the hub allows [user]/[one]/[or]/[more]/[segments]
, these tests won't be valid any more
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.
But we use our parser, so these tests will be valid until we change it and allow mutliple segments for dockerhub. These tests we will change along with the parser.
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 purpose of these tests is not the link itself but any invalid value will cause an error.
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.
Why do we have to special case the docker.io
? Docker client allows me to tag docker.io/miminiar/my/image/with/many/slashes
without any warning. Even tab completion works with this name.
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.
@miminar try to create such repository on docker.io
@@ -184,7 +184,41 @@ func ParseDockerImageReference(spec string) (DockerImageReference, error) { | |||
ref.ID = id | |||
break | |||
default: | |||
// TODO: this is no longer true with V2 |
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.
Regarding case 3
: I feel like it's about time to recognize registry
part based on special characters it contains. The same way upstream does it.
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.
@miminar parser refactoring out of scope of PR. I can open following issue about it.
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.
Ahh, I hate devcuts.
@@ -315,24 +315,6 @@ func TestValidateImageStreamMappingNotOK(t *testing.T) { | |||
field.ErrorTypeRequired, | |||
"image.metadata.name", | |||
}, | |||
"invalid repository pull spec": { |
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.
Instead of removing, could you change the test to check this is a valid reference? Also could you add another case that would trigger ErrorTypeInvalid
?
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.
@miminar This test is called the TestValidateImageStreamMappingNotOK
. So, this case can't be here anyway.
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.
Ok, so at least change the repository to something like ///a/b/c/d
to generate the ErrorTypeInvalid
.
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.
@miminar done
aaab282
to
bdc90d8
Compare
@@ -316,6 +316,14 @@ func TestImageStreamImportOfV1ImageFromV2Repository(t *testing.T) { | |||
} | |||
} | |||
|
|||
func TestImageStreamImportOfV1ImageFromV2Repository(t *testing.T) { | |||
testImageStreamImportWithPath(t, "/test/image") |
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.
Just a nit: repo name doesn't start with a slash.
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.
This is a part of path.
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.
@miminar I need to rename second argument ?
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.
@miminar Done. /
removed.
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.
Thanks!
if len(repoParts) > 3 && !strings.HasSuffix(repoParts[0], ":") { | ||
if IsRegistryDockerHub(repoParts[0]) { | ||
// DockerHub doesn't support multiple segments. | ||
return ref, fmt.Errorf("the docker pull spec %q must be two or three segments separated by slashes", spec) |
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.
I would reword as "the docker pull spec %q must be of the form / to be valid for the docker hub" or something like that
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.
OK. I will use your message.
b914a20
to
191f258
Compare
I think we'll be best served by doing the following (which, yes, is a parser refactoring):
|
@ncdc devcut is tomorrow. We don't have enough time to make such refactoring. |
We should do it right. What release are you currently targeting? |
191f258
to
eeff969
Compare
@ncdc proposed-3.4 |
[test] |
Ok, I looked at the docker/distribution code. It doesn't cover everything we have ("case 2:" for repoParts). So we can proceed with this PR for now and hopefully switch to the upstream parser later. |
if len(repoParts) > 3 && !strings.HasSuffix(repoParts[0], ":") { | ||
if IsRegistryDockerHub(repoParts[0]) { | ||
// DockerHub doesn't support multiple segments. | ||
return ref, fmt.Errorf("the docker pull spec %q must be of the form / to be valid for the docker hub", spec) |
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.
Looks like something got lost in translation from my original suggestion. the docker pull spec %q must be of the form <username>/<repository> to be valid for the docker hub
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.
It's not true. You can use docker.io/busybox
. So, <username>
not required.
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.
Yeah, the fun of the implicit library
. I'd personally prefer to remove this logic that makes a special case out of the hub, even though a hub spec with more than 2 segments isn't valid at the moment. WDYT? @liggitt @mfojtik @pweil- @smarterclayton
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.
@ncdc How about invalid docker hub pull spec %q
?
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.
Either that, or remove this logic if everyone else agrees
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.
@ncdc I added this condition because we have previously banned more than 3 parts, and by this we banned docker.io/foo/bar/baz
as well. I wanted to allow multiple segments only for custom docker/distribution
servers.
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.
If the hub moves to add more than 2 segments, then we'd have to refactor, rebuild, and put out a new release to support it. I'd rather avoid that if we can.
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.
@ncdc error message changed.
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.
@ncdc This check has been removed.
return ref, fmt.Errorf("the docker pull spec %q must be of the form / to be valid for the docker hub", spec) | ||
} | ||
if len(repoParts[0]) == 0 { | ||
return ref, fmt.Errorf("the docker pull spec %q must contain non-empty registry name", spec) |
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.
I would either say you can't have empty segments, or you can't have 2 consecutive slashes (//
). I'm really trying hard to avoid us telling a user that they have to have something when it's not necessarily required.
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.
Or you could say "registry cannot be empty"; I could live with that 😄
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.
But first segment here have to be registry. No? Why can't tell user that he miss and what we expect to see ?
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.
If/when the hub supports multiple segments, no, the first segment doesn't have to be the registry. But today, yes, it has to be the registry. This is fine for now.
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.
@ncdc the docker pull spec %q cannot have empty segments
-- it's ok for you ?
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.
That's fine, thanks.
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.
@ncdc fixed.
ad59728
to
37f2403
Compare
// the DockerImageReference. | ||
if len(repoParts) > 3 && !strings.HasSuffix(repoParts[0], ":") { | ||
if len(repoParts[0]) == 0 { | ||
return ref, fmt.Errorf("the docker pull spec %q cannot have empty segments", spec) |
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.
Make this error text consistent with the 2 below please
ref.Tag = tag | ||
ref.ID = id | ||
break | ||
} | ||
return ref, fmt.Errorf("the docker pull spec %q must be two or three segments separated by slashes", spec) |
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.
To get to this line of code, it means either:
- len(repoParts) > 3 and repoparts[0] is a scheme (http: or https:)
- len(repoParts) = 0
Either way, I don't think we should keep the error message as is, saying that it has to be two or three segments. I'm not sure what the best error message is, though.
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.
@ncdc This error message describes the common requirements for the DockerImageReference. It seems to me that it is better to leave it as is. Otherwise we need to add much more checks to make it better.
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.
You can leave it for now, to get this PR in, but I would like it fixed when we refactor parsing.
LGTM |
Err: true, | ||
}, | ||
{ | ||
From: "docker.io/library/foo/myapp", | ||
Err: true, |
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.
This should be a non-error. Confirmed by jenkins:
--- FAIL: TestParseDockerImageReference (0.00s)
helper_test.go:328: docker.io/library/foo/myapp: unexpected non-error
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.
Sure.
I'm pretty sure that should error On Tuesday, October 4, 2016, Michal Minář notifications@github.com wrote:
|
Heh, forgot that was removed On Tuesday, October 4, 2016, Alexey Gladkov notifications@github.com
|
We need to be able to handle a repository with more than one component in the name (separated by slash). This necessity follows from the fact that docker/distribution supports such names, but we can't parse them. https://bugzilla.redhat.com/show_bug.cgi?id=1373281 Signed-off-by: Gladkov Alexey <agladkov@redhat.com>
37f2403
to
a5ce371
Compare
Evaluated for origin test up to a5ce371 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9647/) |
@ncdc fixed and tested. |
Good enough... can't wait to see this refactored though. |
[merge] (bumped priority on follow up) |
Flake #11240 in the merge job https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9696/. |
[merge] |
Evaluated for origin merge up to a5ce371 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9701/) (Image: devenv-rhel7_5139) |
@mfojtik @liggitt @smarterclayton PTAL