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

Suppressing image digest in docker ps #30848

Merged
merged 1 commit into from Feb 17, 2017

Conversation

Projects
None yet
6 participants
@nishanttotla
Contributor

nishanttotla commented Feb 8, 2017

Signed-off-by: Nishant Totla nishanttotla@gmail.com

This PR suppresses digest in docker ps, requiring the use of --no-trunc if digest is to be seen.

Without --no-trunc

$ docker ps -a
CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS                            PORTS               NAMES
67652ce46200        alpine              "sleep 1000"        10 minutes ago      Exited (137) About a minute ago                       nothing
001677952d7b        alpine:latest       "sleep 1000"        10 minutes ago      Exited (137) About a minute ago                       tag
d645f54a3b62        alpine              "sleep 1000"        10 minutes ago      Exited (137) About a minute ago                       digest
7e9e3fc29448        alpine:latest       "sleep 1000"        10 minutes ago      Exited (137) About a minute ago                       taganddigest

With --no-trunc

$ docker ps -a --no-trunc
CONTAINER ID                                                       IMAGE                                                                                   COMMAND             CREATED             STATUS                            PORTS               NAMES
67652ce46200bf406556b4220da0233bef229c5950899b2dabd81650677c7566   alpine                                                                                  "sleep 1000"        10 minutes ago      Exited (137) About a minute ago                       nothing
001677952d7b4e1a2b4dcff00db8e43c22bfc890a38d00393ce1fb0009e2e760   alpine:latest                                                                           "sleep 1000"        10 minutes ago      Exited (137) About a minute ago                       tag
d645f54a3b6287628389ac604eff553a8451c4ce038a6b137d15a56e8047f248   alpine@sha256:dfbd4a3a8ebca874ebd2474f044a0b33600d4523d03b0df76e5c5986cb02d7e8          "sleep 1000"        10 minutes ago      Exited (137) About a minute ago                       digest
7e9e3fc2944894097e8d168f735acf03e1c23ed50681d1576fff9aa0bd6dd56e   alpine:latest@sha256:dfbd4a3a8ebca874ebd2474f044a0b33600d4523d03b0df76e5c5986cb02d7e8   "sleep 1000"        11 minutes ago      Exited (137) About a minute ago                       taganddigest

This follows up from #28539 to make behavior consistent with service ls/ps.

@nishanttotla nishanttotla changed the title from Suppressing digest in docker ps to Suppressing image digest in docker ps Feb 8, 2017

@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla
Contributor

nishanttotla commented Feb 9, 2017

} else {
// case for when a tag is not provided
named := reference.TrimNamed(ref)
return reference.FamiliarString(named)

This comment has been minimized.

@aaronlehmann

aaronlehmann Feb 10, 2017

Contributor

Thinking about this some more, I'm not sure we want to discard the digest in this case.

It makes sense for the digest+tag case, because the digest is primarily internal information that doesn't need to clutter the output.

But if there's only a digest, it feels like it might be misleading to only show the name.

Not sure what others think. If there's a good UX reason to never show digests here, I guess it's okay.

@aaronlehmann

aaronlehmann Feb 10, 2017

Contributor

Thinking about this some more, I'm not sure we want to discard the digest in this case.

It makes sense for the digest+tag case, because the digest is primarily internal information that doesn't need to clutter the output.

But if there's only a digest, it feels like it might be misleading to only show the name.

Not sure what others think. If there's a good UX reason to never show digests here, I guess it's okay.

This comment has been minimized.

@nishanttotla

nishanttotla Feb 10, 2017

Contributor

@aaronlehmann I see your point. Although, given that --no-trunc exists, it's not the case that there's no way to easily retrieve full information. Wouldn't the same reasoning apply to service ps/ls too? Anyhow, I'm happy to discuss the UX implications of this. Not sure who else to tag here.

(My personal opinion is that I find it cleaner to not have to look at digests unless I specifically ask for them. @cpuguy83 indicated the same earlier. This also came up in another issue that I can't find right now.)

@nishanttotla

nishanttotla Feb 10, 2017

Contributor

@aaronlehmann I see your point. Although, given that --no-trunc exists, it's not the case that there's no way to easily retrieve full information. Wouldn't the same reasoning apply to service ps/ls too? Anyhow, I'm happy to discuss the UX implications of this. Not sure who else to tag here.

(My personal opinion is that I find it cleaner to not have to look at digests unless I specifically ask for them. @cpuguy83 indicated the same earlier. This also came up in another issue that I can't find right now.)

This comment has been minimized.

@aaronlehmann

aaronlehmann Feb 11, 2017

Contributor

That's fair, given that --no-trunc exists removing digests by default might be reasonable.

@cpuguy83: Any thoughts?

@aaronlehmann

aaronlehmann Feb 11, 2017

Contributor

That's fair, given that --no-trunc exists removing digests by default might be reasonable.

@cpuguy83: Any thoughts?

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 13, 2017

Contributor

Yeah I see the digest as an ID, which is generally undesirable in ps unless that's exactly what was specified.

@cpuguy83

cpuguy83 Feb 13, 2017

Contributor

Yeah I see the digest as an ID, which is generally undesirable in ps unless that's exactly what was specified.

This comment has been minimized.

@cpuguy83

cpuguy83 Feb 13, 2017

Contributor

And even if I did specify it... digest is really big for docker ps.

@cpuguy83

cpuguy83 Feb 13, 2017

Contributor

And even if I did specify it... digest is really big for docker ps.

This comment has been minimized.

@thaJeztah

thaJeztah Feb 13, 2017

Member

I think it makes sense to omit it (alternatively, truncate the output, but we don't have a fixed length defined for these)

@thaJeztah

thaJeztah Feb 13, 2017

Member

I think it makes sense to omit it (alternatively, truncate the output, but we don't have a fixed length defined for these)

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 13, 2017

Contributor

LGTM

Contributor

aaronlehmann commented Feb 13, 2017

LGTM

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 17, 2017

Contributor
$ docker swarm init
...
$ docker service create --name test busybox top
...
ERRO[0037] pulling image failed                          error="Cannot overwrite digest sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e" module="node/agent/taskmanager" task.id=8kzv0vh79rigpkz38uqc9okh4
ERRO[0037] fatal task error                              error="No such image: busybox:latest@sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e" module="node/agent/taskmanager" task.id=8kzv0vh79rigpkz38uqc9okh4
Contributor

cpuguy83 commented Feb 17, 2017

$ docker swarm init
...
$ docker service create --name test busybox top
...
ERRO[0037] pulling image failed                          error="Cannot overwrite digest sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e" module="node/agent/taskmanager" task.id=8kzv0vh79rigpkz38uqc9okh4
ERRO[0037] fatal task error                              error="No such image: busybox:latest@sha256:817a12c32a39bbe394944ba49de563e085f1d3c5266eb8e9723256bc4448680e" module="node/agent/taskmanager" task.id=8kzv0vh79rigpkz38uqc9okh4
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 17, 2017

Contributor

Really strange...

Contributor

cpuguy83 commented Feb 17, 2017

Really strange...

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 17, 2017

Contributor

@cpuguy83: Have you rebased against the latest master? You may be experiencing #30889.

Contributor

aaronlehmann commented Feb 17, 2017

@cpuguy83: Have you rebased against the latest master? You may be experiencing #30889.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Feb 17, 2017

Contributor

My master works, this PR is not.

Contributor

cpuguy83 commented Feb 17, 2017

My master works, this PR is not.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 17, 2017

Contributor

It needs to be rebased.

Contributor

aaronlehmann commented Feb 17, 2017

It needs to be rebased.

Suppressing image digest in docker ps
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@nishanttotla

This comment has been minimized.

Show comment
Hide comment
@nishanttotla

nishanttotla Feb 17, 2017

Contributor

@cpuguy83 @aaronlehmann I just rebased this PR.

Contributor

nishanttotla commented Feb 17, 2017

@cpuguy83 @aaronlehmann I just rebased this PR.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Feb 17, 2017

Contributor

Thanks!

Contributor

aaronlehmann commented Feb 17, 2017

Thanks!

@cpuguy83

LGTM

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Feb 17, 2017

Member

windows failure unrelated 😓

Member

vdemeester commented Feb 17, 2017

windows failure unrelated 😓

@vdemeester vdemeester merged commit 4053214 into moby:master Feb 17, 2017

3 of 4 checks passed

windowsRS1 Jenkins build Docker-PRs-WoW-RS1 10535 has failed
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30858 has succeeded
Details
janky Jenkins build Docker-PRs 39473 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Feb 17, 2017

@nishanttotla nishanttotla deleted the nishanttotla:suppress-digest-docker-ps branch Feb 17, 2017

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30848 from nishanttotla/suppress-digest-docke…
…r-ps

Suppressing image digest in docker ps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment