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

Sort swarm stacks and nodes using natural sorting #315

Merged
merged 1 commit into from
Jul 18, 2017
Merged

Sort swarm stacks and nodes using natural sorting #315

merged 1 commit into from
Jul 18, 2017

Conversation

boaz0
Copy link
Contributor

@boaz0 boaz0 commented Jul 10, 2017

follow up to moby/moby#31638

- What I did

Use natural sorting to display swarm stacks and nodes.

- How I did it

Import vbom.ml/util/sortorder and update Less(i, j int) bool in both cli/command/node/list.go and cli/command/stack/list.go

- How to verify it
Run unit-tests

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah
Copy link
Member

ping @aaronlehmann PTAL

@aaronlehmann
Copy link
Contributor

aaronlehmann commented Jul 10, 2017

LGTM

Actually, the description says this is being applied to service and node listing, but I see that the code changes are only in stack/list.go, not service/list.go. Any reason not to apply this to service ls as well?

@boaz0
Copy link
Contributor Author

boaz0 commented Jul 10, 2017

Well, yeah I know it might be misleading but when running docker stack ls you get the services or that's what I thought. I guess I will rename it.

@boaz0 boaz0 changed the title Sort swarm services and nodes using natural sorting Sort swarm stacks and nodes using natural sorting Jul 11, 2017

func TestNodeListOrder(t *testing.T) {
buf := new(bytes.Buffer)
cli := test.NewFakeCli(&fakeClient{
Copy link
Contributor

Choose a reason for hiding this comment

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

This function has changed on master. It no longer takes a second argument, but defaults to using a buffer.

You can access the buffer from cli.OutBuffer()

This commit changes the order stacks and nodes are displayed.
For example, running "docker stack ls" is expected to
display the following list:

NAME          SERVICES
service-1     1
service-2     1
service-10    1

However, currently this is what is printed:

NAME          SERVICES
service-1     1
service-10    1
service-2     1

To fix this, "docker stack ls" and "docker node ls" are using
natural sorting to make it more human readable.

Signed-off-by: Boaz Shuster <ripcurld.github@gmail.com>
@codecov-io
Copy link

codecov-io commented Jul 13, 2017

Codecov Report

Merging #315 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
+ Coverage   48.84%   48.87%   +0.02%     
==========================================
  Files         186      186              
  Lines       12413    12418       +5     
==========================================
+ Hits         6063     6069       +6     
+ Misses       5976     5975       -1     
  Partials      374      374

Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐯

@thaJeztah thaJeztah merged commit 79e4d63 into docker:master Jul 18, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.07.0 milestone Jul 18, 2017
@aaronlehmann
Copy link
Contributor

Can we please get a followup PR to get service ls to behave the same way?

@boaz0
Copy link
Contributor Author

boaz0 commented Jul 18, 2017

Ooh sorry I didn't know that this is what you meant in your comment.
@aaronlehmann I am working on this right away.

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.

None yet

7 participants