Carry #10255: Docker ps format by estesp · Pull Request #14699 · moby/moby

estesp · 2015-07-17T04:15:28Z

Include original #10255 PR content, replace field selection/output implementation with Go template version from @calavera PR #12931

duglin · 2015-07-17T15:10:53Z

cliconfig/config.go

I'm not sure we should have this here since this info, I think, is just specific for one command and should probably live in the ps.go file. If we ever get to the point where each command is an independent exe then the common/config-info shouldn't have any cmd specific info as that would break composibility.
Is there any reason this must be in here instead of ps.go?

never mind - I see now that this is part of the config file - so this makes sense!

calavera · 2015-07-17T15:12:00Z

thanks for adding all those tests ❤️

LGTM

duglin · 2015-07-17T15:25:41Z

api/client/ps/custom_test.go

Can we add a test with multiple Names to verify that with --no-trunc we see 'em all but w/o it we only see one?

duglin · 2015-07-17T15:29:19Z

Just one suggestion about an additional test but aside from that LGTM!

estesp · 2015-07-20T20:55:02Z

@duglin FYI--test added for mutli-names with --no-trunc and using formatter

calavera · 2015-07-20T21:53:16Z

@estesp it looks like there is a racy test. Maps are not guaranteed to be ordered.

duglin · 2015-07-20T23:04:19Z

integration-cli/docker_cli_ps_test.go

I looked and I couldn't find a test that used --link and didn't use --no-trunc - do you see one? If not it might be good to add one.

I've updated the test to do both given we already have linked containers running in the same test

* api/client/ps.go: Refactor CmdPs to use a fields list of characters to determine which columns to print on `docker ps` invocation. This adds an ability for the docker command to print the columns of output in arbitrary order. Signed-off-by: Jeff Mickey <[email protected]> Docker-DCO-1.1-Signed-off-by: Jeff Mickey <[email protected]>

Docker-DCO-1.1-Signed-off-by: David Calavera <[email protected]>

estesp · 2015-07-21T03:02:59Z

@calavera thanks for catching that; I've updated the test to use maps with reflect.DeepEqual if there are comma-separated values in the expected and actual result--given this code is only used for this test, I did not add a lot of error checking/validation of input as the inputs are well-known and hardcoded a few lines above.

SvenDowideit · 2015-07-21T05:28:22Z

@estesp can you please add the same documentation to docs/reference/commandline/ps.md ?

estesp · 2015-07-21T14:00:12Z

@SvenDowideit good catch; I've added text to reference/commandline/ps.md -- please let me know if sufficient or any review comments.

duglin · 2015-07-21T15:51:35Z

code LGTM

thaJeztah · 2015-07-21T19:29:53Z

docs/reference/commandline/cli.md

Could you add a reference here / describe that the psFormat is a string containing a Go template?

Perhaps a link to the Formatting section in ps.md is actually best; it describes the format in detail

tiborvass · 2015-07-22T16:12:18Z

@estesp I'm +1 on removing -F, --format should suffice. We can always change our minds later if we have to.

calavera · 2015-07-22T16:31:57Z

It looks like everyone agrees to remove -F. @estesp can you make that change?

@calavera

Re-add the docs from @calavera's PR to the moved cli cmd reference docs. Fix gofmt and vet issues from carried commits Add integration test for using format with --no-trunc and multi-names Fix custom_test map order dependency on expected value check Add docs to reference/commandline/ps.md Remove "-F" flag option from original carried PR content Docker-DCO-1.1-Signed-off-by: Phil Estes <[email protected]> (github: estesp)

estesp · 2015-07-22T16:57:08Z

Thanks everyone-- -F is gone from implementation and docs, and link from "cliconfig" settings section to ps.md is added (and tested with make docs).

thaJeztah · 2015-07-22T17:03:58Z

Thanks @estesp! re-LGTM for the docs (and removal of -F)

calavera · 2015-07-22T17:27:58Z

LGTM

Carry #10255: Docker ps format

calavera · 2015-07-22T17:58:42Z

🎉 💃

tianon · 2015-07-23T19:57:35Z

api/client/ps/custom.go

We're getting a side-effect from this PR (and I think it's this specific line causing it) where docker ps when you have no containers now prints a single blank line instead of an empty header row (like docker ps used to and docker images still does).

I have an idea to fix it and am working on a PR now.

Thanks @tianon!

vieux · 2015-08-06T00:52:10Z

@calavera can we have the same in images :D

thaJeztah · 2015-08-06T06:08:45Z

@vieux PRs are always welcome 😉

thaJeztah · 2015-08-06T06:09:17Z

@tianon is #14699 (comment) still an issue?

estesp · 2015-08-06T15:07:51Z

@thaJeztah that problem was fixed in #14919

calavera · 2015-08-06T15:45:45Z

@vieux so needy! 😝

However, I'm pretty sure we can make that logic work for several commands with a couple of interfaces more.

thaJeztah · 2015-08-06T16:52:19Z

thanks @estesp! I noticed the comment, but no PR linked

duglin · 2015-08-06T17:36:19Z

@vieux see #14570

duglin · 2015-08-06T17:36:48Z

@vieux oops, never mind - slightly different

GordonTheTurtle added the status/0-triage label Jul 17, 2015

estesp added status/2-code-review and removed status/0-triage labels Jul 17, 2015

estesp mentioned this pull request Jul 17, 2015

Proposal: docker ps: add fields for ordering and column selection #10255

Closed

estesp force-pushed the docker-ps-format branch 2 times, most recently from 73012f1 to 5eb3d74 Compare July 17, 2015 04:22

duglin reviewed Jul 17, 2015
View reviewed changes

calavera added this to the 1.8.0 milestone Jul 17, 2015

icecrime added status/3-docs-review and removed status/2-code-review labels Jul 17, 2015

estesp force-pushed the docker-ps-format branch from 5eb3d74 to 219f042 Compare July 20, 2015 20:53

duglin reviewed Jul 20, 2015
View reviewed changes

estesp force-pushed the docker-ps-format branch from 219f042 to ff2bc19 Compare July 21, 2015 02:40

estesp force-pushed the docker-ps-format branch from ff2bc19 to a397cfb Compare July 21, 2015 02:46

Docker ps custom formatting.

3720919

Docker-DCO-1.1-Signed-off-by: David Calavera <[email protected]>

estesp force-pushed the docker-ps-format branch from a397cfb to b362891 Compare July 21, 2015 02:54

estesp force-pushed the docker-ps-format branch from b362891 to 0811286 Compare July 21, 2015 13:58

thaJeztah reviewed Jul 21, 2015
View reviewed changes

estesp force-pushed the docker-ps-format branch from cc807e9 to 542b58d Compare July 22, 2015 16:51

calavera added status/4-merge and removed status/2-code-review labels Jul 22, 2015

calavera added a commit that referenced this pull request Jul 22, 2015

Merge pull request #14699 from estesp/docker-ps-format

40b9224

Carry #10255: Docker ps format

calavera merged commit 40b9224 into moby:master Jul 22, 2015

estesp deleted the docker-ps-format branch July 22, 2015 17:59

thaJeztah added impact/changelog impact/cli labels Jul 22, 2015

calavera mentioned this pull request Jul 22, 2015

Default "docker ps" output is too wide #7477

Closed

tianon reviewed Jul 23, 2015
View reviewed changes

This was referenced Jul 27, 2015

Proposal: Disable auto-genarated container names to ease cleanup #10839

Closed

Proposal: show container IP address with docker ps command #8786

Open

albers mentioned this pull request Aug 10, 2015

Add docker ps --format to bash completion #15447

Merged

thaJeztah mentioned this pull request Nov 4, 2015

7477-adds_small_flag #17686

Closed

thaJeztah mentioned this pull request Dec 7, 2015

Verbose ls docker-archive-public/docker.machine#2513

Closed

Conversation

estesp commented Jul 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calavera commented Jul 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duglin commented Jul 17, 2015

Uh oh!

estesp commented Jul 20, 2015

Uh oh!

calavera commented Jul 20, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estesp commented Jul 21, 2015

Uh oh!

SvenDowideit commented Jul 21, 2015

Uh oh!

estesp commented Jul 21, 2015

Uh oh!

duglin commented Jul 21, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiborvass commented Jul 22, 2015

Uh oh!

calavera commented Jul 22, 2015

Uh oh!

estesp commented Jul 22, 2015

Uh oh!

thaJeztah commented Jul 22, 2015

Uh oh!

calavera commented Jul 22, 2015

Uh oh!

calavera commented Jul 22, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vieux commented Aug 6, 2015

Uh oh!

thaJeztah commented Aug 6, 2015

Uh oh!

thaJeztah commented Aug 6, 2015

Uh oh!

estesp commented Aug 6, 2015

Uh oh!

calavera commented Aug 6, 2015

Uh oh!

thaJeztah commented Aug 6, 2015

Uh oh!

duglin commented Aug 6, 2015

Uh oh!

duglin commented Aug 6, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants