Follow up on #4660: new app error msg

[rhcarvalho]

Given a JSON template with invalid syntax:

$ cat bad-template.json
{
"there should not be a trailing comma": "here",
}

Before #4660, oc new-app produced this error message:

$ oc new-app bad-template.json
error: no image or template matched "bad-template.json"

After:

$ oc new-app bad-template.json
error: invalid character '}' looking for beginning of object key string

But @smarterclayton commented that the implementation was leaking an abstraction, thus, this follow up.

[rhcarvalho]

@smarterclayton what do you think of this approach?

You suggested of a different type of error that would not be ignored by resolvers, but there are many places in the code path of new-app that ignores errors and/or replace them with new errors, what makes it difficult to solve the abstraction leak with a simple change, or at least I could not find one simple change that would do the trick.

The current solution here is to catch the SyntaxError and print a message to Stderr immediately, what would make it like this:

$ oc new-app bad-template.json 
bad-template.json at offset 51: invalid character '}' looking for beginning of object key string
error: no image or template matched "bad-template.json"

The problem with this is that it might stutter:

$ oc new-app -f bad-template.json 
bad-template.json at offset 51: invalid character '}' looking for beginning of object key string
error: invalid character '}' looking for beginning of object key string

[danmcp]

@smarterclayton Bump

[danmcp]

@csrwng @bparees Review please.

[bparees]

@rhcarvalho squash please.
@smarterclayton approve?

lgtm otherwise.

[csrwng]

lgtm

[rhcarvalho]

This got quite aged. Here's how it is affecting the scenario described in the PR description:

$ cat template.json 
{"broken

a. Current state before the PR:

$ oc new-app template.json 
error: invalid character '\n' in string literal

b. With the changes introduced in this PR:

$ oc new-app template.json 
template.json at offset 9: invalid character '\n' in string literal
error: no match for "template.json", specify --allow-missing-images to use this image name.

The 'new-app' 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. Templates in the current project or the 'openshift' project
  4. Git repository URLs or local paths that point to Git repositories

--allow-missing-images can be used to point to an image that does not exist yet.

See 'new-app' for examples.

Hmm new-app changed considerably meanwhile, that it makes my change produce a worse output then the current state IMHO.

The initial motivator for this change was to try to address @smarterclayton's comment about leaking an abstraction...

Do we still want to fix the abstraction leak? If so, I'd look for a different solution. Otherwise I'll just close this.

[bparees]

i'd still like to clean it up, i kinda hate where that isFile logic lives right now.

[bparees]

as for the right fix... yeah if we had an exact match (of the file "template.json" in this case) we should not be also assuming it might be an image name for an image we couldn't even find.

(if there were an image named "template.json", that would be a different story.... in that case we should return a multiple match error, they should then specify the file explicitly, and then they should get an error that the file is invalid)

[rhcarvalho]

i kinda hate where that isFile logic lives right now.

Yep, it's ugly, but what worked back then. I'm +1 on making it better.

I'll probably defer finding a more appropriate fix post dev-cut, is that alright?

[bparees]

I'll probably defer finding a more appropriate fix post dev-cut, is that alright?

yup, that's the right priority.

[smarterclayton]

As long as we think users won't be confused by the error output this is fine.

[bparees]

it seems a little confusing, but i could be convinced to live with it and merge this if you're actually ok with it :)

[rhcarvalho]

I'm not ok with the output after the current state of this PR. It's very easy to miss the error message in the first line. I'd miss it if I was not intentionally looking for it.

[rhcarvalho]

(note to self) this PR might simply die after #6670 merges.

[rhcarvalho]

The little improvement that was present here and not included in #6670 is printing the offset where a syntax error happened:

$ oc new-app template.json 
error: unable to load template file "template.json": at offset 9: invalid character '\n' in string literal
error: no match for "template.json"

...

@bparees PTAL [test]

[smarterclayton]

LGTM - maybe add a test for it?

[rhcarvalho]

@smarterclayton the test in test/cmd/newapp.sh includes the offset already, do you mean something more than that?

[bparees]

lgtm [merge]

[openshift-bot]

continuous-integration/openshift-jenkins/test Running (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/861/)

[openshift-bot]

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4866/) (Image: devenv-rhel7_3346)

[openshift-bot]

Evaluated for origin merge up to df1f5e9

[openshift-bot]

Evaluated for origin test up to df1f5e9