Add `--format` to `docker service ls` by yongtang · Pull Request #28199 · moby/moby

yongtang · 2016-11-09T07:56:32Z

- What I did

This fix tries to improve the display of docker service ls and adds --format flag to docker service ls.

In addition to --format flag, several other improvement:

Adds --no-trunc to docker service ls
Updates docker stacks service.
Adds servicesFormat to config file.

Related docs has been updated.

cc @thaJeztah @vdemeester @dnephin

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Yong Tang yong.tang.github@outlook.com

AkihiroSuda · 2016-11-09T08:07:21Z

SGTM, can you add tests?

vdemeester · 2016-11-09T09:33:34Z

Design SGTM 👼

thaJeztah · 2016-11-09T11:31:09Z

cli/command/service/list.go

I wonder if we should consider introducing ShortID and/or TruncID placeholders. The TruncID placeholder has the behavior of what ID has now (so, truncated by default, but show full ID if --trunc is added. That way people can define a template where the output always has the full-id, or has an ID that can be truncated or full.

@thaJeztah does that mean we will leave ID alone regardless of --no-trunc=false/true, and only trunc TruncID based on --no-trunc=true?

@yongtang perhaps, yes. My initial thought was "do we need a --no-trunc flag? That's when I started thinking if we perhaps need other options for the format.

It's just a thought though, so perhaps @vdemeester or @dnephin has some thoughts

Same though as @thaJeztah, I don't really like --no-trunc flag, especially since format uses go template. We could provide a function that shorten "or not" the ID : --format="{{ .ID | shorten }}" or something like that.

yongtang · 2016-11-09T15:01:57Z

@AkihiroSuda Added a test for the PR.

thaJeztah · 2016-11-09T15:03:45Z

Design LGTM, let's keep #28199 (comment) for another discussion

vdemeester · 2017-01-12T16:45:12Z

ping @yongtang, needs a rebase 👼

yongtang · 2017-01-13T03:34:16Z

Thanks @vdemeester. The PR has been rebased.

AkihiroSuda · 2017-01-13T04:07:12Z

00:00:59.032 Errors from golint:
00:00:59.032 cli/command/service/list.go:94:1: comment on exported function GetServicesStatus should be of the form "GetServicesStatus ..."
00:00:59.032 
00:00:59.032 Please fix the above errors. You can test via "golint" and commit the result.

yongtang · 2017-01-13T04:12:26Z

Thanks @AkihiroSuda. The PR has been updated with golint issue addressed.

vdemeester

Few comments 👼
I do agree with @thaJeztah on the --no-trunc flags (on other commands as well but 😅)

vdemeester · 2017-01-13T09:42:28Z

cli/command/service/list.go

Same though as @thaJeztah, I don't really like --no-trunc flag, especially since format uses go template. We could provide a function that shorten "or not" the ID : --format="{{ .ID | shorten }}" or something like that.

vdemeester · 2017-01-13T09:43:44Z

cli/command/service/list.go

This else is empty right ? should go 👼

vdemeester · 2017-01-13T09:44:43Z

cli/command/service/list.go

Should/Could this return a map of struct instead, with mode and replica in the struct, instead of two maps ? 👼

yongtang · 2017-01-14T03:05:25Z

@vdemeester @thaJeztah Thanks for the review. The PR has been updated and now --no-trunc are replaced with template func of shorten. Please take a look and let me know if there are any issues.

vdemeester

LGTM 🐸

AkihiroSuda · 2017-01-19T08:07:15Z

integration-cli/docker_cli_swarm_test.go

I wonder this test can be moved to UT, but it can be another PR in the future

AkihiroSuda · 2017-01-19T08:09:12Z

docs/reference/commandline/service_ls.md

I wonder we should have two separate placeholders e.g. RunningReplicas and AllReplicas.

WDYT?

@AkihiroSuda I feel like that means we introduce new concepts that does not necessary match the existing Table header. Should we change the table header to RUNNING/ALL REPLICAS? Or maybe we could leave it alone for now?

I think it is ok to introduce all .Replicas, .RunningReplicas, and .AllReplicas.
But it is ok to leave it alone for now.

thaJeztah · 2017-01-26T20:12:07Z

@yongtang hm, okay discussed this in the maintainers meeting, and we think it would be better to put the shorten option in a separate PR, and make this PR just add the regular "formatting" option, so we can discuss the shorten option a bit more in depth

(sorry 😄)

yongtang · 2017-01-26T21:13:08Z

Thanks @thaJeztah. The PR has been updated. Also created another PR #30484 for shorten. Please take a look.

Also in this PR I didn't add the --no-trunc flag and the output of ID will be truncated by default (same as old behavior). Please let me know if we want to introduce --no-trunc in this PR, or we want to have the full ID output by default (behavior will be different from original).

vdemeester

LGTM 🐯

vdemeester · 2017-02-01T15:23:33Z

Moving to docs-review
/cc @thaJeztah

vdemeester

@yongtang Could you update docs/reference/commandline/cli.md to add an example for ServicesFormat ?

yongtang · 2017-02-01T16:31:53Z

Thanks @vdemeester. The docs has been updated in the PR. Please take a look and let me know if there are any issues.

This fix tries to improve the display of `docker service ls` and adds `--format` flag to `docker service ls`. In addition to `--format` flag, several other improvement: 1. Updates `docker stacks service`. 2. Adds `servicesFormat` to config file. Related docs has been updated. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

vdemeester

docs LGTM 🐸
/cc @thaJeztah for docs revisit

Add `--format` to `docker service ls`

GordonTheTurtle added the status/0-triage label Nov 9, 2016

vdemeester added status/1-design-review and removed status/0-triage labels Nov 9, 2016

thaJeztah reviewed Nov 9, 2016

View reviewed changes

yongtang force-pushed the 11062016-service-ls-format branch from 1e001a5 to c5ba328 Compare November 9, 2016 15:01

thaJeztah added status/2-code-review and removed status/1-design-review labels Nov 9, 2016

yongtang force-pushed the 11062016-service-ls-format branch from c5ba328 to 9113044 Compare January 13, 2017 03:33

yongtang force-pushed the 11062016-service-ls-format branch from 9113044 to c411a9b Compare January 13, 2017 04:11

vdemeester added the rebuild/experimental label Jan 13, 2017

GordonTheTurtle removed the rebuild/experimental label Jan 13, 2017

vdemeester suggested changes Jan 13, 2017

View reviewed changes

yongtang force-pushed the 11062016-service-ls-format branch 3 times, most recently from 45533cd to 09edc91 Compare January 14, 2017 03:02

AkihiroSuda added the rebuild/experimental label Jan 14, 2017

GordonTheTurtle removed the rebuild/experimental label Jan 14, 2017

vdemeester approved these changes Jan 15, 2017

View reviewed changes

AkihiroSuda reviewed Jan 19, 2017

View reviewed changes

yongtang force-pushed the 11062016-service-ls-format branch from 09edc91 to 6bf3b16 Compare January 20, 2017 21:05

yongtang force-pushed the 11062016-service-ls-format branch from 6bf3b16 to e60ec5d Compare January 26, 2017 21:09

yongtang mentioned this pull request Jan 26, 2017

Add truncate function for Go templates #30484

Merged

vdemeester approved these changes Jan 27, 2017

View reviewed changes

AkihiroSuda added the rebuild/experimental label Jan 30, 2017

GordonTheTurtle removed the rebuild/experimental label Jan 30, 2017

vdemeester added status/3-docs-review and removed status/2-code-review labels Feb 1, 2017

vdemeester suggested changes Feb 1, 2017

View reviewed changes

yongtang force-pushed the 11062016-service-ls-format branch from e60ec5d to 90e4711 Compare February 1, 2017 16:32

yongtang force-pushed the 11062016-service-ls-format branch from 90e4711 to 000f040 Compare February 1, 2017 16:34

vdemeester approved these changes Feb 1, 2017

View reviewed changes

vdemeester added docs/revisit status/4-merge and removed status/3-docs-review labels Feb 1, 2017

vdemeester assigned thaJeztah Feb 1, 2017

vdemeester added the rebuild/windowsRS1 label Feb 1, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Feb 1, 2017

vdemeester merged commit 4ca00c0 into moby:master Feb 2, 2017

GordonTheTurtle added this to the 1.14.0 milestone Feb 2, 2017

yongtang deleted the 11062016-service-ls-format branch February 2, 2017 12:42

albers mentioned this pull request Feb 3, 2017

Add bash completion for (stack services|service ls) --format #30710

Merged

sdurrheimer mentioned this pull request Feb 12, 2017

Add zsh completion for 'docker {service ls,stack services} --format' #30948

Merged

thaJeztah added impact/changelog impact/cli labels Mar 13, 2017

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

Merge pull request moby#28199 from yongtang/11062016-service-ls-format

16c1a50

Add `--format` to `docker service ls`

Conversation

yongtang commented Nov 9, 2016

Uh oh!

AkihiroSuda commented Nov 9, 2016

Uh oh!

vdemeester commented Nov 9, 2016

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!

Choose a reason for hiding this comment

Uh oh!

yongtang commented Nov 9, 2016

Uh oh!

thaJeztah commented Nov 9, 2016

Uh oh!

vdemeester commented Jan 12, 2017

Uh oh!

yongtang commented Jan 13, 2017

Uh oh!

AkihiroSuda commented Jan 13, 2017

Uh oh!

yongtang commented Jan 13, 2017

Uh oh!

vdemeester left a comment

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!

Choose a reason for hiding this comment

Uh oh!

yongtang commented Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vdemeester left a comment

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!

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!

thaJeztah commented Jan 26, 2017

Uh oh!

yongtang commented Jan 26, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester commented Feb 1, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

yongtang commented Feb 1, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yongtang commented Jan 14, 2017 •

edited

Loading