adjust newapp/newbuild error messages (arg classification vs. actual …#18272
Conversation
pkg/oc/cli/cmd/newapp.go
Outdated
| // only print it if we precede with classification errors, to help distinguish | ||
| // between the two | ||
| fmt.Fprintln(buf, "Errors occurred during resource creation:") | ||
| fmt.Fprintln(buf, "Errors occurred during %s processing of arguments:") |
There was a problem hiding this comment.
ahhhh .... fix coming ....
8f7f63f to
1805dbc
Compare
pkg/oc/cli/cmd/newapp.go
Outdated
| // only print it if we precede with classification errors, to help distinguish | ||
| // between the two | ||
| fmt.Fprintln(buf, "Errors occurred during resource creation:") | ||
| fmt.Fprintf(buf, "Errors occurred during %s processing of arguments:", commandName) |
There was a problem hiding this comment.
can we see a sample of this new output? i'm not clear on what values "commandName" takes on
There was a problem hiding this comment.
For a new-build invocation it would be:
Errors occurred during new-build processing of arguments:
where the dump of the errorGroup follows ...i.e. error: ....
substitute new-app for new-build if it comes from oc new-app ...
|
the cmd failure was a result of masking the classification error text (I was thinking there was a test for it but could not find it earlier) some more adjustments coming ... |
|
same for one of the two items noted in the integration test failure |
pkg/oc/cli/cmd/newapp.go
Outdated
| // between the two | ||
| fmt.Fprintln(buf, "Errors occurred during resource creation:") | ||
| fmt.Fprintf(buf, "Errors occurred during %s processing of arguments:", commandName) | ||
| fmt.Fprint(buf, "\n") |
There was a problem hiding this comment.
why not just put this in the Fprintf above?
There was a problem hiding this comment.
will do
also, an fyi, this path is proving rare now, in that most args pass the IsComponentReference test
1805dbc to
a6e71b7
Compare
|
pushed changes for latest @bparees comments, as well as test adjustments for the changes in messages |
test/cmd/newapp.sh
Outdated
| os::cmd::expect_failure_and_text 'oc new-build https://www.google.com/openshift/nodejs-e' 'as a Git repository URL: ' | ||
| os::cmd::expect_failure_and_text 'oc new-build https://examplegit.com/openshift/nodejs-e' 'as a Git repository URL: ' | ||
| os::cmd::expect_failure_and_text 'oc new-app mysql-persisten mysql' 'only a partial match was found' | ||
| os::cmd::expect_failure_and_text 'oc new-app --strategy=docker https://192.30.253.113/openshift/ruby-hello-world.git' 'only a partial match was found' |
There was a problem hiding this comment.
I wouldn't expect this change... feels like a step backwards from the current output:
$ oc new-app --strategy=docker https://192.30.253.113/openshift/ruby-hello-world.git
error: Errors occurred while determining argument types:
https://192.30.253.113/openshift/ruby-hello-world.git as a Git repository URL: fatal: unable to access 'https://192.30.253.113/openshift/ruby-hello-world.git/': SSL: no alternative certificate subject name matches target host name '192.30.253.113'
https://192.30.253.113/openshift/ruby-hello-world.git as a local directory pointing to a Git repository: stat https://192.30.253.113/openshift/ruby-hello-world.git: no such file or directory
Errors occurred during resource creation:
error: --strategy is specified and none of the arguments provided could be classified as a source code location
There was a problem hiding this comment.
(If i were to change anything about the above output, i'd skip the resource creation error output and just abort after the argument determination errors)
There was a problem hiding this comment.
this change centers around my observation about the component ref classification rarely failing
I had been consider shifting my check from the classification logic (though it was more elegant) to the handleError logic ... your concern has prompted me to do that
I believe that will result in reverting the change here
There was a problem hiding this comment.
One more point as a result of the shift though ... I'll be curious after I start posting some sample output if you'll want to qualify on which runtime errors the classification errors get posted.
I wonder if we'll want to reconcile the type of error noted in functions like https://github.com/openshift/origin/blob/master/pkg/oc/cli/cmd/newapp.go#L752-L878 with the output you want to see with say the invocation from @GrahamDumpleton that stemmed this issue.
I'll provide some precise examples in a bit if that helps the determination.
There was a problem hiding this comment.
OK, I have a set of changes on my laptop, that if we go with, would result in:
-
only changing the newapp.sh test to account for the @bparees requested transition of
no match fortounable to locate resource for -
output of the following for @GrahamDumpleton 's example:
error: unable to locate resource for "xxx"
The 'oc new-build' command will match arguments to the following types:
1. Images tagged into image streams in the current project or the 'openshift' project
- if you don't specify a tag, we'll add ':latest'
2. Images in the Docker Hub, on remote registries, or on the local Docker engine
3. Git repository URLs or local paths that point to Git repositories
--allow-missing-images can be used to force the use of an image that was not matched
See 'oc new-build -h' for examples.
For further assistance, new-build classification of argument types resulted in the following:
python~https://github.com/openshift-katacoda/blog-django-py as a Git repository URL: fatal: I don't handle protocol 'python~https'
python~https://github.com/openshift-katacoda/blog-django-py as a local directory pointing to a Git repository: stat python~https://github.com/openshift-katacoda/blog-django-py: no such file or directory
The argument python~https://github.com/openshift-katacoda/blog-django-py passed the component classification tests
Thoughts ?
test/cmd/newapp.sh
Outdated
| os::cmd::expect_failure_and_text 'oc new-app mysql-persisten mysql' 'only a partial match was found' | ||
| os::cmd::expect_failure_and_text 'oc new-app --strategy=docker https://192.30.253.113/openshift/ruby-hello-world.git' 'only a partial match was found' | ||
| os::cmd::expect_failure_and_text 'oc new-app https://www.google.com/openshift/nodejs-e' 'unable to lo' | ||
| os::cmd::expect_failure_and_text 'oc new-app https://examplegit.com/openshift/nodejs-e' 'unable to lo ' |
There was a problem hiding this comment.
similar concerns/questions on these 2.
There was a problem hiding this comment.
also i think you have an extraneous space in the second one? but i'd prefer to see a lengthier text anyway, cutting it in the middle of a word is odd.
a6e71b7 to
533e96a
Compare
|
refactor / redirection from latest discussion thread pushed |
|
hard for me to visualize the impact of the change, can you create some sample output? That said, taking a step back in terms of how we can help users, I think there's a flow/filter (which i think you've sort of evolved towards):
1+2 are a bit combined today which makes this hard, of course. |
|
I suspect the gcp failure https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/18272/test_pull_request_origin_extended_conformance_gce/15085/ is related to all the in flight work @smarterclayton is doing with prometheus and the routers (#18245 and #18254) |
|
@bparees - sure thing:
|
pkg/oc/cli/cmd/newapp.go
Outdated
| fmt.Fprint(buf, group.suggestion) | ||
| } | ||
| if len(config.ArgumentClassificationErrors) > 0 && len(groups) > 0 { | ||
| fmt.Fprintf(buf, "\nFor further assistance, %s classification of the argument types resulted in the following:\n", commandName) |
There was a problem hiding this comment.
I don't think including the cmd name in this output helps (also you've got an extra space after the comma).
I think i'd just trim it down to "Classification of arguments resulted in the following:\n"
pkg/oc/generate/app/cmd/newapp.go
Outdated
| c.ArgumentClassificationWinner = fmt.Sprintf(winnerFmt, s, "template") | ||
| case c.tryToAddComponentArguments(s): | ||
| // NOTE, component argument classification currently is the most lenient, so we save it for the end | ||
| c.ArgumentClassificationWinner = fmt.Sprintf(winnerFmt, s, "component") |
There was a problem hiding this comment.
"component" is a confusing term here...as a user when i see "the argument x passedt he component classification tests" i don't really know what that means my argument was treated as. Is there another term we can use?
pkg/oc/generate/app/cmd/newapp.go
Outdated
| // AddArguments converts command line arguments into the appropriate bucket based on what they look like | ||
| func (c *AppConfig) AddArguments(args []string) []string { | ||
| unknown := []string{} | ||
| winnerFmt := "The argument %s passed the %s classification tests" |
There was a problem hiding this comment.
can an argument be classified as more than one thing? if not, i'd change this to "Argument x was classified as y"
|
@gabemontero it seems a bit strange to me that we report that we couldn't match "xxx" to a resource, but we aren't printing any information about what "xxx" was classified(or not classified) as. also I don't think its useful to print the classification information for arguments that we ultimately successfully classified, unless something else goes wrong. Again breaking this down i think we should fail as early as possible and only report on the first thing we failed out on. Let users iterate their way to a correct set of args. As it stands right now, the fact that we couldn't understand "xxx" gets a bit lost w/ all the other output of the things that were not actually a problem. |
While you percolate on that, I'll make updates per your inline PR comments and push. And can resubmit those 3 examples I posted afterward. |
c30f8ba to
816b5a7
Compare
|
Updates on those 3 examples: |
816b5a7 to
8adf826
Compare
|
@bparees and I had a chance to talk yesterday. We'll go with:
|
|
Something happened to my push with the unit/integration test fixes ... trying again Once I have that sorted out, I'll circle back to @bparees 's #18272 (comment) |
167d81c to
bafcae4
Compare
|
OK they are there now ... waiting for the tests to complete. |
|
OK everything passed but cmd ... it was newapp related, but I don't understand yet why if failed. It seems like the text it was looking for was there: Sure enough it is failing for me now locally (had been passing I though) ... investigating |
|
Ahhh ... @bparees had me replace |
sorry :) |
bafcae4 to
b51c45b
Compare
|
/retest |
|
Something became amiss forwarding params to the remote host across the board: @dobbymoodge fyi ^^ |
b51c45b to
923dce7
Compare
|
ok @bparees I've made the change which will include errors for git repo access check for args ultimately classified as image, image~code, or loaded templates give it a whirl |
923dce7 to
cdebd14
Compare
|
Forgot ... here are the outputs for the samples I've been posting: |
|
So the unit test failure was weird. The tests passed, but 24 were skipped: And then it said while no failures were found, test-go.sh "failed" : Will poke around and try to find out if this has been reported elsewhere. |
|
gcp tests failed provisioning the test cluster |
|
unit test failure is #17881 |
|
/test unit |
|
barring a complete rewrite of how this arg matching+classifying logic works, i guess this is as good as it's going to get. but please reintroduce a test around partial matches in some form. |
cdebd14 to
2785d75
Compare
|
partial match test sorted out @bparees |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, gabemontero The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
|
Automatic merge from submit-queue (batch tested with PRs 18454, 18504, 18510, 18481, 18272). |
…processing
Fixes #17925
@openshift/sig-developer-experience ptal
Now produces: