Skip to content
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

remove COMMAND column from service ls output. closes #27994 #28029

Merged
merged 1 commit into from
Nov 7, 2016
Merged

remove COMMAND column from service ls output. closes #27994 #28029

merged 1 commit into from
Nov 7, 2016

Conversation

tealtail
Copy link
Contributor

@tealtail tealtail commented Nov 3, 2016

- What I did
removes COMMAND col from service ls output

- How to verify it
run tests, run docker service ls
- A picture of a cute animal (not mandatory but encouraged)
image

Signed-off-by: Alicia Lauerman [email protected]

@aaronlehmann
Copy link
Contributor

I'd vote for removing the COMMAND column instead. I don't think it's very useful. It would keep things simpler not thave this column at all.

@tealtail
Copy link
Contributor Author

tealtail commented Nov 3, 2016

@aaronlehmann okay, yeah, that's much simpler :) I'll update this PR soon to remove the column

@thaJeztah
Copy link
Member

We should think about adding a --format flag, so that users can customize the output. I'll check if there's an issue for that already, otherwise open one

@GordonTheTurtle GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed dco/no Automatically set by a bot when one of the commits lacks proper signature labels Nov 3, 2016
@tealtail tealtail changed the title truncate service ls command text, add no-trunc option. closes #27994 remove COMMAND column from service ls output. closes #27994 Nov 3, 2016
@tealtail
Copy link
Contributor Author

tealtail commented Nov 3, 2016

locally tests pass but the builds triggered by push here failed with

18:33:21 Error response from daemon: No such container: docker-pr-exp26009
[...]
18:33:41 Finished: FAILURE

@thaJeztah
Copy link
Member

@tealtail looks like this is the actual failure;

20:58:29 ---> Making bundle: validate-vet (in bundles/1.13.0-dev/validate-vet)
20:58:29 cli/command/service/list.go:112: missing argument for Fprintf("%s"): format reads arg 5, have only 4 args
20:58:29 cli/command/service/list.go:120: missing argument for Fprintf("%s"): format reads arg 5, have only 4 args
20:58:29 exit status 1

@thaJeztah thaJeztah added the status/failing-ci Indicates that the PR in its current state fails the test suite label Nov 3, 2016
@tealtail
Copy link
Contributor Author

tealtail commented Nov 4, 2016

ohhh thanks @thaJeztah okay, so I'm thinking the problem is that listItemFmt would need 1 less %s now? I'm running tests after making that change and will update the PR again tonight

@tealtail
Copy link
Contributor Author

tealtail commented Nov 4, 2016

all builds are passing now 👍

@thaJeztah thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label Nov 4, 2016
@thaJeztah
Copy link
Member

Awesome!

design LGTM, but a quick check with @aluzzardi if he agrees

@aluzzardi
Copy link
Member

LGTM

We actually discussed removing this column a few days ago as it was consuming precious screen real estate with no meaningful use, so 👍

@aaronlehmann
Copy link
Contributor

LGTM

@thaJeztah
Copy link
Member

docs changes here LGTM, but ping @mstanleyjones as it's possible other parts of the docs have some example output that needs to be updated

@thaJeztah thaJeztah added this to the 1.13.0 milestone Nov 4, 2016
Copy link
Contributor

@mdlinville mdlinville left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it also affect service update, service rm, service inspect, etc?

@tealtail
Copy link
Contributor Author

tealtail commented Nov 4, 2016

@mstanleyjones yes! some of those still have COMMAND output included in examples. Update incoming.

@tealtail
Copy link
Contributor Author

tealtail commented Nov 7, 2016

@mstanleyjones I've updated this PR with the changes you requested

@mdlinville
Copy link
Contributor

LGTM. cc @allencloud please sync #27710 with this change.

@mdlinville mdlinville merged commit c16860b into moby:master Nov 7, 2016
dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
…cate-opt

remove COMMAND column from `service ls` output. closes moby#27994
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants