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.

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

Flag Addition: --type flag added for docker inspect command #13187

Merged
merged 1 commit into from Jul 1, 2015

Conversation

Projects
None yet
10 participants
@shishir-a412ed
Copy link
Contributor

shishir-a412ed commented May 13, 2015

Fixes #12483

Signed-off-by: Shishir Mahajan shishir.mahajan@redhat.com

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented May 13, 2015

why two flags its not really scalable...

@calavera

This comment has been minimized.

Copy link
Contributor

calavera commented May 13, 2015

I'm 👎 to the solution, but I acknowledge the problem.

There are a couple possibilities that might be better suitable to solve the problem:

  1. Since we acknowledge that a container and an image can have the same name, we can argue that we should always perform the two calls. This is probably the easier solution, 0 flags needed. We can be slightly fancy and try to detect whether the argument is a container id or not, and only send the container request in the first case.
  2. If we don't want to send two requests every time, we can add a --all flag and do what I propose in 1 when the flag is enabled.
@shishir-a412ed

This comment has been minimized.

Copy link
Contributor Author

shishir-a412ed commented May 13, 2015

@calavera
we contemplated that option but we felt it can present an awful lot of information to user.
e.g. docker inspect --all arg1 arg2 arg3 ..... argN can show N * 2 JSON to the user, whereas all user wanted was to see the image JSON which were being shadowed by the container JSON, since they shared the same name.

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 14, 2015

What if we do --image and --container, but make them both true by default.

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented May 14, 2015

No 2 flags!!!

On Wednesday, May 13, 2015, Brian Goff notifications@github.com wrote:

What if we do --image and --container, but make them both true by default.


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

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented May 14, 2015

Ok, --type

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented May 14, 2015

oh man, can we please consider just adding a new command: docker inspecti ... like we have docker rmi .... I'm not thrilled with just adding an 'i' to the end of the command but at least we're consistent and it would then be very clear what we're talking about.

@rhatdan

This comment has been minimized.

Copy link
Contributor

rhatdan commented May 14, 2015

I don't really understand the argument against two arguments. It is not like the docker inspect has a whole ton of options now. Originally we were going to use just --image-only, but then why couldn't you ask for only container matches.

Bottom line is we have bug reports from people trying to look at images with the same name as a container, and we want a simple way for the users to be able to examine them without having to understand where to search for the UUid field in a container json file.

I actually hate rmi versus rm, but I guess that somewhat follows the standard of rm versus rmdir.

If we went with this behaviour we should break existing behaviour for consistency.

This seems like a simple fix, but simple fixes often bring out Bike Shedding arguments.

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented May 14, 2015

It's a UI decision I know the struggle with it being the same name but two
flags is imo dumb and doesn't scale

On Thursday, May 14, 2015, Daniel J Walsh notifications@github.com wrote:

I don't really understand the argument against two arguments. It is not
like the docker inspect has a whole ton of options now. Originally we were
going to use just --image-only, but then why couldn't you ask for only
container matches.

Bottom line is we have bug reports from people trying to look at images
with the same name as a container, and we want a simple way for the users
to be able to examine them without having to understand where to search for
the UUid field in a container json file.

I actually hate rmi versus rm, but I guess that somewhat follows the
standard of rm versus rmdir.

If we went with this behaviour we should break existing behaviour for
consistency.

This seems like a simple fix, but simple fixes often bring out Bike
Shedding arguments.


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

@shishir-a412ed

This comment has been minimized.

Copy link
Contributor Author

shishir-a412ed commented May 14, 2015

@jfrazelle what do you mean by 2 flags doesn't scale ?
Can you give an example or scenario ?

@rhatdan

This comment has been minimized.

Copy link
Contributor

rhatdan commented May 14, 2015

Not sure how adding additional commands scales better then adding options. I would argue the exact opposite.

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented May 14, 2015

Adding 2 flags that are super specific to 1 thing is "not scalable" because
now if you inspect anything other than images or containers in docker, whoa
look we need to add a new flag for that too

But honestly I don't want to fight about this, do what you want, see what
happens when users think docker is hard to use because we have a bajillion
flags, each one does 1 thing

On Thursday, May 14, 2015, Daniel J Walsh notifications@github.com wrote:

Not sure how adding additional commands scales better then adding options.
I would argue the exact opposite.


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

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented May 14, 2015

Also I'm definitely -1 on a inspecti, rmi is awful and we should not go
down that road again, the only ok example given here was --type because at
least we can add more things to inspect in the future without a new flag
for each one

On Thursday, May 14, 2015, Jessica Frazelle jess@docker.com wrote:

Adding 2 flags that are super specific to 1 thing is "not scalable"
because now if you inspect anything other than images or containers in
docker, whoa look we need to add a new flag for that too

But honestly I don't want to fight about this, do what you want, see what
happens when users think docker is hard to use because we have a bajillion
flags, each one does 1 thing

On Thursday, May 14, 2015, Daniel J Walsh <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Not sure how adding additional commands scales better then adding
options. I would argue the exact opposite.


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

@shishir-a412ed

This comment has been minimized.

Copy link
Contributor Author

shishir-a412ed commented May 14, 2015

@jfrazelle
boolean flags are binary {true/false, 0/1} in nature. If that is your concern, then @cpuguy83 suggested --type flag which would be a string flag and can take all kind of values.

I understand your concern and its a valid one, but IMHO docker inspect really doesn't have that many flags to begin with. n the use-case is legitimate.

@rhatdan

This comment has been minimized.

Copy link
Contributor

rhatdan commented May 14, 2015

I am fine with --type=image or --type=container.

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented May 14, 2015

The only way a new flag helps, IMO, is if its mandatory. W/o it being required we’re then
still left with the current mess of the code guessing as to what the user intended to take action
on. That’s just a horrible design. While “rmi” and “inspecti” are not my favorite things,
at least its unambiguous and no room for confusion.

Ideally, I would have preferred if we had “docker container inspect” and “docker image inspect”
but that’s another discussion.

On May 14, 2015, at 7:25 AM, Jessie Frazelle notifications@github.com wrote:

Also I'm definitely -1 on a inspecti, rmi is awful and we should not go
down that road again, the only ok example given here was --type because at
least we can add more things to inspect in the future without a new flag
for each one

On Thursday, May 14, 2015, Jessica Frazelle jess@docker.com wrote:

Adding 2 flags that are super specific to 1 thing is "not scalable"
because now if you inspect anything other than images or containers in
docker, whoa look we need to add a new flag for that too

But honestly I don't want to fight about this, do what you want, see what
happens when users think docker is hard to use because we have a bajillion
flags, each one does 1 thing

On Thursday, May 14, 2015, Daniel J Walsh <notifications@github.com
javascript:_e(%7B%7D,'cvml','notifications@github.com');> wrote:

Not sure how adding additional commands scales better then adding
options. I would argue the exact opposite.


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


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

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented May 14, 2015

I think there is a real use case for adding a flag for this, I've hit numerous times w my irssi container

@shishir-a412ed shishir-a412ed force-pushed the shishir-a412ed:docker_inspect_issue branch 4 times, most recently from 17fed35 to 77f256f May 14, 2015

@shishir-a412ed

This comment has been minimized.

Copy link
Contributor Author

shishir-a412ed commented May 14, 2015

@jfrazelle @calavera @rhatdan
I have updated the code and docs.

@shishir-a412ed shishir-a412ed force-pushed the shishir-a412ed:docker_inspect_issue branch from 77f256f to ae180ed May 14, 2015

@shishir-a412ed shishir-a412ed changed the title Flags Addition: --image-only and --container-only flags added for docker inspect command. Flag Addition: --type flag added for docker inspect command May 14, 2015

@shishir-a412ed shishir-a412ed force-pushed the shishir-a412ed:docker_inspect_issue branch 2 times, most recently from af114c0 to 27491c4 May 18, 2015

@jessfraz

This comment has been minimized.

Copy link
Contributor

jessfraz commented May 21, 2015

design LGTM thanks

@@ -24,8 +24,85 @@ each result.
**-f**, **--format**=""
Format the output using the given go template.

**--type**=*container*|*image*

This comment has been minimized.

Copy link
@thaJeztah
@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jun 30, 2015

Thanks @shishir-a412ed ! Added some comments inline.

Also, this needs documentation in the online reference as well; https://github.com/docker/docker/blob/master/docs/reference/commandline/inspect.md

@shishir-a412ed shishir-a412ed force-pushed the shishir-a412ed:docker_inspect_issue branch 6 times, most recently from b9f8b0c to 97275c5 Jul 1, 2015

@shishir-a412ed

This comment has been minimized.

Copy link
Contributor Author

shishir-a412ed commented Jul 1, 2015

@moxiegirl @thaJeztah I have made the changes.
Let me know if I missed anything.

# EXAMPLES

Getting information on an image where image name conflict with the container name.
e,g both image and container are named rhel7.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jul 1, 2015

Member

Really minor; some dots/comma's are incorrect here. But I'm okay with leaving that for someone else to fix.

Think it should be;

Getting information on an image where image name conflict with the container name, 
e.g., both image and container are named rhel7.

This comment has been minimized.

Copy link
@shishir-a412ed

shishir-a412ed Jul 1, 2015

Author Contributor

Fixed.

## Getting information on a container

To get information on a container use its ID or instance name:

$ docker inspect d2cc496561d6
[{
[{

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Jul 1, 2015

Member

Thanks for fixing these!

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jul 1, 2015

LGTM, thanks for being patient, @shishir-a412ed, I know it took a while!

ping @moxiegirl

Flag Addition: --type flag added for docker inspect command
Signed-off-by: Shishir Mahajan <shishir.mahajan@redhat.com>

@shishir-a412ed shishir-a412ed force-pushed the shishir-a412ed:docker_inspect_issue branch from 97275c5 to 2cb74e6 Jul 1, 2015

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jul 1, 2015

Kicked experimental build, ready to merge if that passes

@shishir-a412ed

This comment has been minimized.

Copy link
Contributor Author

shishir-a412ed commented Jul 1, 2015

@thaJeztah Sounds good !

thaJeztah added a commit that referenced this pull request Jul 1, 2015

Merge pull request #13187 from shishir-a412ed/docker_inspect_issue
Flag Addition: --type flag added for docker inspect command

@thaJeztah thaJeztah merged commit 98f988f into moby:master Jul 1, 2015

4 checks passed

docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 2138 has succeeded
Details
janky Jenkins build Docker-PRs 10653 has succeeded
Details
windows Jenkins build Windows-PRs 7351 has succeeded
Details
@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Jul 1, 2015

\o/ merged!

@rhatdan

This comment has been minimized.

Copy link
Contributor

rhatdan commented Jul 2, 2015

Woo Hoo

sdurrheimer added a commit to sdurrheimer/docker that referenced this pull request Jul 14, 2015

Zsh completion update for the following commits:
- Add fluentd logging driver to zsh completion moby#12876
- Add inspect --type flag to zsh completion moby#13187
- Respect -H option in zsh completion moby#13195
- Fix number of argument limit for pause and unpause in zsh completion

Signed-off-by: Steve Durrheimer <s.durrheimer@gmail.com>

calavera added a commit to calavera/docker that referenced this pull request Jul 25, 2015

Zsh completion update for the following commits:
- Add fluentd logging driver to zsh completion moby#12876
- Add inspect --type flag to zsh completion moby#13187
- Respect -H option in zsh completion moby#13195
- Fix number of argument limit for pause and unpause in zsh completion

Signed-off-by: Steve Durrheimer <s.durrheimer@gmail.com>

tiborvass added a commit to tiborvass/docker that referenced this pull request Jul 27, 2015

Zsh completion update for the following commits:
- Add fluentd logging driver to zsh completion moby#12876
- Add inspect --type flag to zsh completion moby#13187
- Respect -H option in zsh completion moby#13195
- Fix number of argument limit for pause and unpause in zsh completion

Signed-off-by: Steve Durrheimer <s.durrheimer@gmail.com>

@thaJeztah thaJeztah added the impact/cli label Aug 2, 2015

@shishir-a412ed shishir-a412ed deleted the shishir-a412ed:docker_inspect_issue branch Aug 13, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.