Limits the amount of suggestions of a faulty new-build command to 5, fixes #7729 #9353
Conversation
|
That sort order doesn't look right - I would expect origin-base to be below origin:latest. Where's v1.1*? |
|
@smarterclayton At some point the v1.1 gets ignored and it returns latest. Any idea where this could be? I've been looking, can't find it. From what I've been reading in code, the openshift/origin:v1.1 is passed as one term to the search. This may also solve the origin-base issue. |
|
I think we want to implement a scoring sort that does closeness based on On Jun 21, 2016, at 11:30 AM, Ryan Murphy notifications@github.com wrote: @smarterclayton https://github.com/smarterclayton At some point the v1.1 — |
|
Based on dockerimagelookup.go:matchTag() and scorers.go:partialScorer(), it looks like we already implement pretty specific scoring, which gets sorted by sort.Sort. As for the v1.1 vs latest, I only had :latest images on my machine. If I pull older images, it returns those as well. After a rebase, I've tried to reproduce docker.io/openshift/origin:latest being second on the list, but it is now returned as the top result. Output is now: @bparees @smarterclayton ptal? |
|
and what happens if you also have an "openshift/origin-test:v1.1.2" image and look for origin-test:v1.1? |
|
An exact match would occur with origin-test:v1.1, partial match for v1.1.2 using the search term as a prefix. Because of the partial match, the process errors out. The result is: I added openshift/origin-test:v1.1.1 to test what would happen with multiple version numbers. |
|
thanks. Just wanted to confirm it appears before :latest in the list. |
|
@oatmealraisin you might experiment w/ retagging things on your machine. I suspect whether origin or origin-base shows up first depends on which one was tagged/created more recently on your local machine, as opposed to a definitive order. It would be good to check that before we conclude it's working correctly. |
|
I changed the scoring so that the versions are prefixed, so v1.1 will partially match with v1.1*, etc. |
|
lgtm. @smarterclayton was there a reason you didn't want to partially score the tags? (or anything else for that matter?) |
|
[test] |
|
My primary concern was "the smell test" - when a user looks at the
list, what matches would they consider "obvious". Tag prefixes are
easy (when a user gives us a tag), but it seems unlikely that a user
asking for foo/bar expects bar or foo to match the tag.
A lot of this is asking yourself which match should be higher in
common input cases and figuring out how to prioritize them. Users
will be angry if we look lazy, but if we're even broadly helpful
that's enough.
|
|
actually i'm going to retract that. if we're cutting the result list at 5, we should print a message letting the user know that not all matches are being displayed. |
|
Yeah, and tell them to use -S to do it (I assume -S doesn't trim, right?) On Sat, Jul 9, 2016 at 11:13 AM, Ben Parees notifications@github.com
|
|
@smarterclayton |
|
re[test] |
|
New-build should have -S. IF we haven't added it yet.... well... that makes this harder. I assume you fixed new-app as well? |
|
new-build doesn't have -S. Should be pretty easy to implement, though! Maybe have a more general oc search? Or point at docker command? This should fix anything that returns a ErrMulti, so I believe it fixes new-app. |
Old output of a mistyped or not found image in a new-build call would return all possibly matching images. This commit limits that to five, and instead suggests using the new-app search function. Does not make search suggestion if there are less than 5 matches. As a side fix, we also now check for version prefixing, so v1.1 will partially match to v1.1.1 and v1.1.2.
|
Added some logic to point at Previous output, more than 5 results: New output, more than 5 results: Previous output, less than 5 results: New output, less than 5 results: |
|
[test] |
|
@bparees @smarterclayton ptal? |
| t.Errs..., | ||
| ) | ||
|
|
||
| return |
There was a problem hiding this comment.
probably safer to break instead of return, lest we add additional processing/output at the bottom of this method.
There was a problem hiding this comment.
nm, i see why you returned. this is fine.
|
lgtm |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6171/) (Image: devenv-rhel7_4576) |
|
Evaluated for origin merge up to 989fbf1 |
|
Evaluated for origin test up to 989fbf1 |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6171/) |
|
Quick question - you showed: oc new-build -D $'FROM openshif/origin:v1.1\nENV ok=1' --to origin-test:v1.1 The argument "openshif/origin:v1.1" could apply to the following
To view a full list of matches, use 'oc new-app -S openshif/origin:v1.1' Why is openshift/origin:v1.1 not the top match? None of those others are On Tue, Jul 12, 2016 at 8:21 PM, OpenShift Bot notifications@github.com
|
|
i don't think @oatmealraisin actually had the "openshif/origin:v1.1" tag in his local docker daemon. notice it wasn't in the full listing either (the "previous output" example) |
|
@smarterclayton like @bparees said I didn't have any closer images unfortunately. I had been messing with my docker images, so they are a bit odd. I tested the v1.1 functionality earlier in the thread, if that helps! |
|
OK. On Tue, Jul 12, 2016 at 10:06 PM, Ryan Murphy notifications@github.com
|
Limits the amount of suggestions of a faulty new-build command to 5, with highest match score appearing first.
fixes #7729
Old output:
New output:
@bparees