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 services using natural sorting #302

Closed
wants to merge 3 commits into from
Closed

Sort swarm services using natural sorting #302

wants to merge 3 commits into from

Conversation

boaz0
Copy link
Contributor

@boaz0 boaz0 commented Jul 6, 2017

This is a follow-up to moby/moby#31638

- What I did

Use natural sorting to display swarm services in docker stack ls.

- How I did it

Import vbom.ml/util/sortorder and update Less(i, j int) bool

- How to verify it

Run unit test

- Description for the changelog

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

@boaz0
Copy link
Contributor Author

boaz0 commented Jul 6, 2017

Should I add ./vendor/vbom.ml to this commit?

@aaronlehmann
Copy link
Contributor

Yes, you would have to vendor the package.

@aaronlehmann
Copy link
Contributor

Should this be done for nodes as well as services?

@thaJeztah
Copy link
Member

@ripcurld0 if you're not sure how to; if you're on a Mac, these are the steps to vendor the package;

start a shell in a development container;

$ make -f docker.Makefile shell

inside the container, vendor the package;

$ vndr vbom.ml/util

2017/07/06 22:55:10 Collecting initial packages
2017/07/06 22:55:13 Download dependencies
2017/07/06 22:55:14 	Clone vbom.ml/util, revision 928aaa586d7718c70f4090ddf83f2b34c16fdc8d
2017/07/06 22:55:15 	Finished clone vbom.ml/util
2017/07/06 22:55:15 Dependencies downloaded. Download time: 1.039323695s
2017/07/06 22:55:15 Collecting all dependencies
2017/07/06 22:55:23 Clean vendor dir from unused packages
2017/07/06 22:55:25 Success
2017/07/06 22:55:25 Running time: 14.71871232s

Then, exit the container, and add the vendored package to your commit

@GordonTheTurtle
Copy link

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "natural_sort" git@github.com:ripcurld0/cli.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354047976
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@boaz0
Copy link
Contributor Author

boaz0 commented Jul 7, 2017

@aaronlehmann probably yes. should I include it in this PR?

@aaronlehmann
Copy link
Contributor

Sure.

@codecov-io
Copy link

codecov-io commented Jul 7, 2017

Codecov Report

Merging #302 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master    #302      +/-   ##
=========================================
+ Coverage   48.68%   48.7%   +0.01%     
=========================================
  Files         186     185       -1     
  Lines       12416   12168     -248     
=========================================
- Hits         6045    5926     -119     
+ Misses       5996    5875     -121     
+ Partials      375     367       -8

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.

left a comment 👍

@@ -52,3 +52,4 @@ gopkg.in/yaml.v2 4c78c975fe7c825c6d1466c42be594d1d6f3aba6
github.com/tonistiigi/fsutil 0ac4c11b053b9c5c7c47558f81f96c7100ce50fb
github.com/stevvooe/continuity cd7a8e21e2b6f84799f5dd4b65faf49c8d3ee02d
golang.org/x/sync de49d9dcd27d4f764488181bea099dfe6179bcf0
vbom.ml/util 928aaa586d7718c70f4090ddf83f2b34c16fdc8d
Copy link
Member

Choose a reason for hiding this comment

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

can you include this change in the same commit that adds the vendored code?

given that this is a new vendor, it's probably ok as well to just squash your commits (for updating vendors we usually keep it in a separate commit, but perhaps it's ok to do everything in a single one to not complicate things)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done 👣

thaJeztah and others added 2 commits July 9, 2017 04:10
The `err` variable was set in a loop, so only
the last result was taken into account to return
"failure" or not.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: khaled souf <khaled.souf@gmail.com>
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.

Reading on my phone - not a full review, just a note 😄

// compared bytewise, while the latter are compared numerically (except that
// the number of leading zeros is used as a tie-breaker, so e.g. "2" < "02")
//
// Limitation: only ASCII digits (0-9) are considered.
Copy link
Member

Choose a reason for hiding this comment

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

Just realized this is something we should be aware of - depending for what we use this library (IIRC, some object types in docker allow emoji etc )

Copy link
Member

Choose a reason for hiding this comment

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

Objects that currently accept non-ascii characters (that I'm aware of);

  • networks
  • volumes using a volume-driver plugin

So for those we'd have to add some extra logic if we want to add natural sort

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe this is a problem. It looks like the sort works fine with non-ascii characters, but the detection and special handling of digits only considers ASCII digits as opposed to other numerals (https://en.wikipedia.org/wiki/Numerals_in_Unicode).

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.

changes LGTM, but looks like you need to do a rebase, because you pulled in two unrelated commits.

Let me know if you need help doing so

@boaz0
Copy link
Contributor Author

boaz0 commented Jul 10, 2017

Honestly I have no idea how to remove these commits from the PR.

This commit changes the order services 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>
@boaz0
Copy link
Contributor Author

boaz0 commented Jul 10, 2017

I am closing these and opening a new PR.

@boaz0 boaz0 closed this Jul 10, 2017
@boaz0
Copy link
Contributor Author

boaz0 commented Jul 10, 2017

Opened #315

@boaz0 boaz0 deleted the natural_sort branch July 10, 2017 09:58
@thaJeztah
Copy link
Member

Most likely the extra commits were introduced when you rebased and your "upstream" was out of date.

My usual steps are ("upstream" pointing to "docker/cli" and "origin" pointing to "my-fork/cli");

git fetch upstream -v; git fetch origin -v; git rebase upstream/master

After that, force-push to by branch

git push -f origin

@boaz0
Copy link
Contributor Author

boaz0 commented Jul 10, 2017

I wish I knew it. Thanks a lot for the tip, though.

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.

6 participants