Skip to content

Sort swarm stacks and nodes using natural sorting#315

Merged
thaJeztah merged 1 commit into
docker:masterfrom
boaz0:nat_sort
Jul 18, 2017
Merged

Sort swarm stacks and nodes using natural sorting#315
thaJeztah merged 1 commit into
docker:masterfrom
boaz0:nat_sort

Conversation

@boaz0

@boaz0 boaz0 commented Jul 10, 2017

Copy link
Copy Markdown
Contributor

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

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@thaJeztah

Copy link
Copy Markdown
Member

ping @aaronlehmann PTAL

@aaronlehmann

aaronlehmann commented Jul 10, 2017

Copy link
Copy Markdown

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

boaz0 commented Jul 10, 2017

Copy link
Copy Markdown
Contributor Author

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 Jul 11, 2017

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

Copy link
Copy Markdown
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

codecov-io commented Jul 13, 2017

Copy link
Copy Markdown

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

@vdemeester vdemeester left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown

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

@boaz0

boaz0 commented Jul 18, 2017

Copy link
Copy Markdown
Contributor Author

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