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

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

Merged
merged 1 commit into from Feb 2, 2017

Conversation

@yongtang
Member

yongtang commented Nov 9, 2016

- 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:

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

Related docs has been updated.

cc @thaJeztah @vdemeester @dnephin

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

1223

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

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Nov 9, 2016

Member

SGTM, can you add tests?

Member

AkihiroSuda commented Nov 9, 2016

SGTM, can you add tests?

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Nov 9, 2016

Member

Design SGTM 👼

Member

vdemeester commented Nov 9, 2016

Design SGTM 👼

Show outdated Hide outdated cli/command/service/list.go Outdated
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Nov 9, 2016

Member

@AkihiroSuda Added a test for the PR.

Member

yongtang commented Nov 9, 2016

@AkihiroSuda Added a test for the PR.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Nov 9, 2016

Member

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

Member

thaJeztah commented Nov 9, 2016

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

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Jan 12, 2017

Member

ping @yongtang, needs a rebase 👼

Member

vdemeester commented Jan 12, 2017

ping @yongtang, needs a rebase 👼

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 13, 2017

Member

Thanks @vdemeester. The PR has been rebased.

Member

yongtang commented Jan 13, 2017

Thanks @vdemeester. The PR has been rebased.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Jan 13, 2017

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

AkihiroSuda commented Jan 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 13, 2017

Member

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

Member

yongtang commented Jan 13, 2017

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

Show outdated Hide outdated cli/command/service/list.go Outdated
Show outdated Hide outdated cli/command/service/list.go Outdated
Show outdated Hide outdated cli/command/service/list.go Outdated
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 14, 2017

Member

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

Member

yongtang commented Jan 14, 2017

@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 🐸

`.ID` | Service ID
`.Name` | Service name
`.Mode` | Service mode (replicated, global)
`.Replicas` | Service replicas

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Jan 19, 2017

Member

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

WDYT?

@AkihiroSuda

AkihiroSuda Jan 19, 2017

Member

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

WDYT?

This comment has been minimized.

@yongtang

yongtang Jan 20, 2017

Member

@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?

@yongtang

yongtang Jan 20, 2017

Member

@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?

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Jan 25, 2017

Member

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

@AkihiroSuda

AkihiroSuda Jan 25, 2017

Member

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

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 20, 2017

Member

@AkihiroSuda @vdemeester Thanks for the review. The PR has been updated with integration tests have been replaced by unit tests. Please take a look.

Member

yongtang commented Jan 20, 2017

@AkihiroSuda @vdemeester Thanks for the review. The PR has been updated with integration tests have been replaced by unit tests. Please take a look.

@AkihiroSuda AkihiroSuda referenced this pull request Jan 25, 2017

Closed

[UX] implementation status of the `--format` #30431

23 of 24 tasks complete
@AkihiroSuda

LGTM

PTAL @vdemeester @thaJeztah for confirmation.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 25, 2017

Member

Let me put it in code-review for a bit, I'd like to confer with other maintainers on the shorten approach from a UX consistency perspective. I think it's useful, but need to be sure before we merge ❤️

Member

thaJeztah commented Jan 25, 2017

Let me put it in code-review for a bit, I'd like to confer with other maintainers on the shorten approach from a UX consistency perspective. I think it's useful, but need to be sure before we merge ❤️

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 25, 2017

Member

@thaJeztah in case we want to stick with shorten we may want to consider deprecated --no-trunc for other commands (for consistency). Either is fine for me though.

Member

yongtang commented Jan 25, 2017

@thaJeztah in case we want to stick with shorten we may want to consider deprecated --no-trunc for other commands (for consistency). Either is fine for me though.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jan 26, 2017

Member

@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 😄)

Member

thaJeztah commented Jan 26, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 26, 2017

Member

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

Member

yongtang commented Jan 26, 2017

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

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Feb 1, 2017

Member

Moving to docs-review
/cc @thaJeztah

Member

vdemeester commented Feb 1, 2017

Moving to docs-review
/cc @thaJeztah

@vdemeester

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

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 1, 2017

Member

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

Member

yongtang commented Feb 1, 2017

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

Add `--format` to `docker service ls`
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

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

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30206 has succeeded
Details
janky Jenkins build Docker-PRs 38820 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9892 has succeeded
Details

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

@yongtang yongtang deleted the yongtang:11062016-service-ls-format branch Feb 2, 2017

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

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