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

Carry #10255: Docker ps format #14699

Merged
merged 3 commits into from Jul 22, 2015

Conversation

Projects
None yet
@estesp
Copy link
Contributor

estesp commented Jul 17, 2015

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

@@ -57,6 +57,7 @@ type AuthConfig struct {
type ConfigFile struct {
AuthConfigs map[string]AuthConfig `json:"auths"`
HttpHeaders map[string]string `json:"HttpHeaders,omitempty"`
PsFormat string `json:"psFormat,omitempty"`

This comment has been minimized.

@duglin

duglin Jul 17, 2015

Contributor

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?

This comment has been minimized.

@duglin

duglin Jul 17, 2015

Contributor

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

@calavera

This comment has been minimized.

Copy link
Contributor

calavera commented Jul 17, 2015

thanks for adding all those tests ❤️

LGTM

call func() string
}{
{types.Container{ID: containerId}, true, stringid.TruncateID(containerId), idHeader, ctx.ID},
{types.Container{Names: []string{"/foobar_baz"}}, true, "foobar_baz", namesHeader, ctx.Names},

This comment has been minimized.

@duglin

duglin Jul 17, 2015

Contributor

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?

@@ -59,6 +60,20 @@ the running containers.
**--since**=""
Show only containers created since Id or Name, include non-running ones.

**-F**, **--format**=*"TEMPLATE"*
Pretty-print containers using a Go template.
Valid placeholders:

This comment has been minimized.

@duglin

duglin Jul 17, 2015

Contributor

Thanks for adding the list of valid ones!!

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Jul 17, 2015

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

@calavera calavera added this to the 1.8.0 milestone Jul 17, 2015

@estesp estesp force-pushed the estesp:docker-ps-format branch from 5eb3d74 to 219f042 Jul 20, 2015

@estesp

This comment has been minimized.

Copy link
Contributor

estesp commented Jul 20, 2015

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

@calavera

This comment has been minimized.

Copy link
Contributor

calavera commented Jul 20, 2015

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

@@ -508,3 +508,22 @@ func (s *DockerSuite) TestPsListContainersFilterCreated(c *check.C) {
c.Fatalf("Expected id %s, got %s for filter, out: %s", cID, containerOut, out)
}
}

func (s *DockerSuite) TestPsFormatMultiNames(c *check.C) {

This comment has been minimized.

@duglin

duglin Jul 20, 2015

Contributor

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.

This comment has been minimized.

@estesp

estesp Jul 21, 2015

Contributor

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

This comment has been minimized.

@duglin

duglin Jul 21, 2015

Contributor

thanks!!

@estesp estesp force-pushed the estesp:docker-ps-format branch from 219f042 to ff2bc19 Jul 21, 2015

docker ps: add fields for ordering and column selection
* 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 <j@codemac.net>

Docker-DCO-1.1-Signed-off-by: Jeff Mickey <j@codemac.net>

@estesp estesp force-pushed the estesp:docker-ps-format branch from ff2bc19 to a397cfb Jul 21, 2015

Docker ps custom formatting.
Docker-DCO-1.1-Signed-off-by: David Calavera <david.calavera@gmail.com>

@estesp estesp force-pushed the estesp:docker-ps-format branch from a397cfb to b362891 Jul 21, 2015

@estesp

This comment has been minimized.

Copy link
Contributor

estesp commented Jul 21, 2015

@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

This comment has been minimized.

Copy link
Contributor

SvenDowideit commented Jul 21, 2015

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

@estesp estesp force-pushed the estesp:docker-ps-format branch from b362891 to 0811286 Jul 21, 2015

@estesp

This comment has been minimized.

Copy link
Contributor

estesp commented Jul 21, 2015

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

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Jul 21, 2015

code LGTM

When the `--format` flag is not provided with the `docker ps` command,
Docker's client uses this property. If this property is not set, the client
falls back to the default table format.

This comment has been minimized.

@thaJeztah

thaJeztah Jul 21, 2015

Member

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

This comment has been minimized.

@thaJeztah

thaJeztah Jul 21, 2015

Member

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

Following is a sample `config.json` file:

{
"HttpHeaders: {
"MyHeader": "MyValue"
}
},
"psFormat": "table {{.ID}}\\t{{.Image}}\\t{{.Command}}\\t{{.Labels}}"

This comment has been minimized.

@thaJeztah

thaJeztah Jul 21, 2015

Member

Either github is rendering incorrectly, or did you mix tabs/spaces

@tiborvass

This comment has been minimized.

Copy link
Collaborator

tiborvass commented Jul 22, 2015

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

@calavera

This comment has been minimized.

Copy link
Contributor

calavera commented Jul 22, 2015

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

ps --format: Add config.js doc, fix gofmt, add integration tests
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 <estesp@linux.vnet.ibm.com> (github: estesp)

@estesp estesp force-pushed the estesp:docker-ps-format branch from cc807e9 to 542b58d Jul 22, 2015

@estesp

This comment has been minimized.

Copy link
Contributor

estesp commented Jul 22, 2015

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

This comment has been minimized.

Copy link
Member

thaJeztah commented Jul 22, 2015

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

@calavera

This comment has been minimized.

Copy link
Contributor

calavera commented Jul 22, 2015

LGTM

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

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

4 checks passed

docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 3431 has succeeded
Details
janky Jenkins build Docker-PRs 11984 has succeeded
Details
windows Jenkins build Windows-PRs 8619 has succeeded
Details
@calavera

This comment has been minimized.

Copy link
Contributor

calavera commented Jul 22, 2015

🎉 💃

@estesp estesp deleted the estesp:docker-ps-format branch Jul 22, 2015

break
}
if table && len(header) == 0 {
header = containerCtx.fullHeader()

This comment has been minimized.

@tianon

tianon Jul 23, 2015

Member

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).

This comment has been minimized.

@tianon

tianon Jul 23, 2015

Member

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

This comment has been minimized.

@thaJeztah
@vieux

This comment has been minimized.

Copy link
Collaborator

vieux commented Aug 6, 2015

@calavera can we have the same in images :D

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Aug 6, 2015

@vieux PRs are always welcome 😉

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Aug 6, 2015

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

@estesp

This comment has been minimized.

Copy link
Contributor

estesp commented Aug 6, 2015

@thaJeztah that problem was fixed in #14919

@calavera

This comment has been minimized.

Copy link
Contributor

calavera commented Aug 6, 2015

@vieux so needy! 😝

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

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Aug 6, 2015

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

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Aug 6, 2015

@duglin

This comment has been minimized.

Copy link
Contributor

duglin commented Aug 6, 2015

@vieux oops, never mind - slightly different

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment