-
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
adjust newapp/newbuild error messages (arg classification vs. actual … #18272
adjust newapp/newbuild error messages (arg classification vs. actual … #18272
Conversation
pkg/oc/cli/cmd/newapp.go
Outdated
@@ -723,7 +723,7 @@ func handleError(err error, baseName, commandName, commandPath string, config *n | |||
// this print serves as a header for the printing of the errorGroups, but | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing arg for %s
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 nm, i 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.
nope, i don't 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.
ahhhh .... fix coming ....
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.
fix pushed
8f7f63f
to
1805dbc
Compare
pkg/oc/cli/cmd/newapp.go
Outdated
@@ -723,7 +723,8 @@ func handleError(err error, baseName, commandName, commandPath string, config *n | |||
// this print serves as a header for the printing of the errorGroups, but | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
nit and lgtm.
pkg/oc/cli/cmd/newapp.go
Outdated
@@ -723,7 +723,8 @@ func handleError(err error, baseName, commandName, commandPath string, config *n | |||
// this print serves as a header for the printing of the errorGroups, but | |||
// 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) | |||
fmt.Fprint(buf, "\n") |
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 not just put this in the Fprintf 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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 for
tounable 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar concerns/questions on these 2.
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 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
for _, group := range groups { | ||
fmt.Fprint(buf, kcmdutil.MultipleErrors("error: ", group.errs)) | ||
if len(group.suggestion) > 0 { | ||
fmt.Fprintln(buf) | ||
} | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"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
@@ -312,6 +313,7 @@ func (c *AppConfig) tryToAddTemplateArguments(s string) bool { | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Needs 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: