Skip to content

Convert inspect command to Cobra#23902

Merged
LK4D4 merged 2 commits intomoby:masterfrom
dnephin:inspect-to-cobra
Aug 4, 2016
Merged

Convert inspect command to Cobra#23902
LK4D4 merged 2 commits intomoby:masterfrom
dnephin:inspect-to-cobra

Conversation

@dnephin
Copy link
Copy Markdown
Member

@dnephin dnephin commented Jun 23, 2016

@dnephin dnephin added the area/cli Client label Jun 23, 2016
@dnephin dnephin added this to the 1.13.0 milestone Jun 23, 2016
@GordonTheTurtle GordonTheTurtle added status/0-triage dco/no Automatically set by a bot when one of the commits lacks proper signature labels Jun 23, 2016
@dnephin dnephin mentioned this pull request Jun 23, 2016
Closed
43 tasks
@dnephin dnephin force-pushed the inspect-to-cobra branch from 1ca1b1f to c85b019 Compare June 23, 2016 15:12
@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jun 23, 2016
@dnephin dnephin force-pushed the inspect-to-cobra branch from c85b019 to 50e351a Compare June 23, 2016 15:17
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Jun 23, 2016

hmm, it looks like there was a bug in the old command. I treated any error in task inspect as "not found". I tried to correct that, but now a bunch of tests are failing because they are inspecting without swarm mode being enabled.

I could restore the old behaviour, but maybe I should just special case that error as well, and treat it as a not found.

@dnephin dnephin force-pushed the inspect-to-cobra branch from 5036738 to e4eb785 Compare June 23, 2016 18:15
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Jun 28, 2016

This conflicts heavily with #23614

@dnephin dnephin mentioned this pull request Jul 16, 2016
Closed
allencloud
allencloud reviewed Jul 17, 2016
View reviewed changes
Comment thread api/client/system/inspect.go Outdated
Copy link
Copy Markdown
Contributor

@allencloud allencloud Jul 17, 2016

Choose a reason for hiding this comment

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

doesn't exist, maybe some typo

@dnephin dnephin force-pushed the inspect-to-cobra branch 5 times, most recently from ad428fc to b3e3099 Compare August 2, 2016 18:49
@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Aug 3, 2016

rebased

@dnephin dnephin mentioned this pull request Aug 3, 2016
Merged
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Aug 3, 2016

Thanks! Been testing this a bit, and wonder if we should tweak the response a bit, for example, running on a non-swarm-manager:

root@470a59904960:/go/src/github.com/docker/docker# docker inspect foobarz
[]
Error: No such image, container or task: foobarz

I think we should change the response to No such image or container, otherwise it suggests that it would've returned the task if it existed on that node. If you change it, perhaps change the order in which items are queried (containers, images, tasks)

Possibly something that was already there, but I just noticed; when using --type=task, the error message is correct, but I noticed that an empty array ([]) is shown;

root@470a59904960:/go/src/github.com/docker/docker# docker inspect --type=task foobarz
[]
Error response from daemon: This node is not a swarm manager. Use "docker swarm init" or "docker swarm join" to connect this node to swarm and try again.

Since this is an error, the [] should probably be omitted (but that's just a nit), maybe we should change it in every case where no results are found.

@thaJeztah thaJeztah mentioned this pull request Aug 3, 2016
Merged
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Aug 3, 2016

Other than the nits above, LGTM

@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Aug 3, 2016

perhaps change the order in which items are queried (containers, images, tasks)

That is already the order items are queried. I've updated the error message to match the order. We can't really change the order because that would lead to different behaviour from previous versions when names or ids overlap.

the error message is correct, but I noticed that an empty array ([]) is shown

The empty array is shown for all other errors, so I think it should be for the "no swarm" error as well.

@dnephin
Convert inspect to cobra.
851c388 
Signed-off-by: Daniel Nephin <[email protected]>
@dnephin dnephin force-pushed the inspect-to-cobra branch 2 times, most recently from bb40331 to 0e804a0 Compare August 3, 2016 15:11
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Aug 3, 2016

That is already the order items are queried.

Oh, that's what I meant; sorry for the confusion; meant to say, the message doesn't match the actual order.

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Aug 3, 2016

Do you think we should change the output in non-swarm (manager) mode, and remove "task" from the error? (No such container or image)

@dnephin
Copy link
Copy Markdown
Member Author

dnephin commented Aug 3, 2016

Do you think we should change the output in non-swarm (manager) mode, and remove "task" from the error?

Not really, I don't think we gain anything from the extra code to do that.

tonistiigi
tonistiigi reviewed Aug 4, 2016
View reviewed changes
Comment thread integration-cli/docker_cli_daemon_test.go
}

// make sure the container is not running
runningOut, err := s.d.Cmd("inspect", "--format='{{.State.Running}}'", "top")
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi Aug 4, 2016

Choose a reason for hiding this comment

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

These quote changes are just a cleanup?

Copy link
Copy Markdown
Member Author

@dnephin dnephin Aug 4, 2016

Choose a reason for hiding this comment

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

Yup, we made this change to a bunch of tests as we ported things to spf13/pflags. The old pkg/mflag had special code that was used exclusively by tests to strip these single quotes. Normally on a shell they would be removed by bash, but since this command isn't passed to bash they don't have any purpose.

@tonistiigi
Copy link
Copy Markdown
Member

tonistiigi commented Aug 4, 2016

LGTM

1 similar comment
@LK4D4
Copy link
Copy Markdown
Contributor

LK4D4 commented Aug 4, 2016

LGTM

@LK4D4 LK4D4 merged commit 979d48b into moby:master Aug 4, 2016
@dnephin dnephin deleted the inspect-to-cobra branch August 4, 2016 18:58
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

area/cli Client status/2-code-review

Projects

None yet

Milestone

1.13.0

Development

Successfully merging this pull request may close these issues.

7 participants

@dnephin @thaJeztah @tonistiigi @LK4D4 @allencloud @vdemeester @GordonTheTurtle