Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fail when there is an error executing an inspect template. #17284

Merged
merged 1 commit into from
Oct 23, 2015
Merged

Fail when there is an error executing an inspect template. #17284

merged 1 commit into from
Oct 23, 2015

Conversation

calavera
Copy link
Contributor

  • Do not execute the template directly in the cli outout, go is not atomic
    in this operation and can send bytes before failing the execution.
  • Fail after evaluating a raw interface if the typed execution also
    failed, assuming there is a template parsing error.

Closes #15566
Closes #17026

Signed-off-by: David Calavera david.calavera@gmail.com

@calavera calavera added this to the 1.9.0 milestone Oct 22, 2015
@tiborvass
Copy link
Contributor

@calavera needs rebase

@calavera
Copy link
Contributor Author

Rebased

tiborvass
tiborvass reviewed Oct 22, 2015
View reviewed changes
api/client/inspect.go
status = 1
continue
break
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why break? it used to continue to the next argument to inspect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is that the fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because there is something very wrong when we cannot decode an api response and we should not continue with the next arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous behavior explained like a movie:

  • Client: hey, can I have this container's info?
  • API: sure, here you have some gibberish that might not even be a json payload
  • Client: cool, can I have this other container's info?

The new behavior goes like this:

  • Client: hey, can I have this container's info?
  • API: sure, here you have some gibberish that might not even be a json payload
  • Client: what's wrong with you API?? abort!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆

@calavera
Copy link
Contributor Author

After further investigation, it looks like this fix also solves #17026:

➜  docker inspect -f '{{.State.Running}} {{.State.ExitCode}} {{.HostConfig.Dns}}' test
false 143 []

@calavera calavera mentioned this pull request Oct 22, 2015
Closed
@duglin
Copy link
Contributor

duglin commented Oct 22, 2015

at one point I thought @shishir-a412ed had a fix for this that just tried using the current path, and then if that failed it tried again using the raw - but didn't output anything until the end. Would be easier?

@calavera
Copy link
Contributor Author

Would be easier?

that's exactly what this patch does. Look at the tmpl.Execute methods, the stopped using cli.out as the template output and now they use a buffer.

@duglin
Copy link
Contributor

duglin commented Oct 22, 2015

yea, its just I think he did it w/o a new func and I remember it being just a few more lines added - but perhaps I'm remembering incorrectly.

@calavera
Copy link
Contributor Author

No, he didn't do any of that, he's PR modified the the api returned types and removed the raw decoding.

@duglin
Copy link
Contributor

duglin commented Oct 22, 2015

the latest did yea, but I think an older version didn’t do all of that.

On Oct 22, 2015, at 4:15 PM, David Calavera notifications@github.com wrote:

No, he didn't do any of that, he's PR modified the the api returned types and removed the raw decoding.

—
Reply to this email directly or view it on GitHub #17284 (comment).

@calavera
Fail when there is an error executing an inspect template.
3b9c138 
- Do not execute the template directly in the cli outout, go is not atomic
in this operation and can send bytes before failing the execution.
- Fail after evaluating a raw interface if the typed execution also
failed, assuming there is a template parsing error.

Signed-off-by: David Calavera <david.calavera@gmail.com>
tiborvass
tiborvass reviewed Oct 22, 2015
View reviewed changes
api/client/inspect.go
@@ -101,42 +100,45 @@ func (cli *DockerCli) CmdInspect(args ...string) error {
} else {
rdr := bytes.NewReader(obj)
dec := json.NewDecoder(rdr)
buf := bytes.NewBufferString("")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buf := new(bytes.Buffer) is more idiomatic but dont update just for this

@tiborvass
Copy link
Contributor

LGTM

1 similar comment
@crosbymichael
Copy link
Contributor

LGTM

crosbymichael added a commit that referenced this pull request Oct 23, 2015
@crosbymichael
Merge pull request #17284 from calavera/buffer_inspect_template
21aecf6 
Fail when there is an error executing an inspect template.
@crosbymichael crosbymichael merged commit 21aecf6 into moby:master Oct 23, 2015
@phemmer
Copy link
Contributor

phemmer commented Oct 23, 2015

Do we want to do the same for docker version (buffer the output)? It doesn't repeat things, but it will send partial output before failing:

# docker version -f 'My foo is {{.Foo}}'
My foo is 
template: :1:12: executing "" at <.Foo>: Foo is not a field of struct type client.VersionData

@calavera calavera deleted the buffer_inspect_template branch October 23, 2015 16:12
@calavera
Copy link
Contributor Author

@phemmer I'd say yes. We already handle ps like that and I think it's a good UX behavior, partial outputs are not very friendly.

@tiborvass tiborvass removed their assignment Oct 27, 2015
@thaJeztah thaJeztah mentioned this pull request Nov 4, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
impact/changelog status/2-code-review
Projects
None yet
Milestone
1.9.0
Development

Successfully merging this pull request may close these issues.
7 participants
@calavera @tiborvass @duglin @crosbymichael @phemmer @thaJeztah @GordonTheTurtle