-
Notifications
You must be signed in to change notification settings - Fork 2k
use sortorder library for sorting network list output #1266
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
use sortorder library for sorting network list output #1266
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1266 +/- ##
=======================================
Coverage 54.29% 54.29%
=======================================
Files 268 268
Lines 17855 17855
=======================================
Hits 9695 9695
Misses 7550 7550
Partials 610 610 |
|
There are still a few more commands, e.g. |
|
Thank you @adshmh ! Yes please follow with the other commands in this very PR. Otherwise LGTM 😄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
|
oh! sorry, missed the other comment; yes including other output in this PR would be good (but also fine with going ahead with just this one 😅) |
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
1ea6234 to
17f9236
Compare
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
17f9236 to
5cc1f90
Compare
|
Thank you for the reviews. I have updated the PR. There are 2 remaining instances of direct comparison in the cli/cli/command/trust/inspect.go Lines 66 to 89 in 1d04f7d
I think testing the above for order would require adding some mocking code to allow adding custom role names to a notary repository (perhaps that can be added at some point when required by more test cases on trust package) or, alternatively, adding another predefined notary repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (with a small nit), thank you @adshmh !!
| ) | ||
|
|
||
| func TestMatchReleasedSignaturesSortOrder(t *testing.T) { | ||
| var releasesRole = data.DelegationRole{BaseRole: data.BaseRole{Name: trust.ReleasesRole}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: releasesRole := data.DelegationRole{BaseRole: data.BaseRole{Name: trust.ReleasesRole}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the review. Seems the PR was merged before I modify this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries; I looked at the comment, and thought it to be not relevant enough to update before merging 🤗
|
@thaJeztah PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
| func TestPrintSignerInfoSortOrder(t *testing.T) { | ||
| roleToKeyIDs := map[string][]string{ | ||
| "signer2-foo": {"B"}, | ||
| "signer10-foo": {"C"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit; might want to swap A, B and C in this list, so that it's more clear that the list is sorted by signer, not keys

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

- What I did
Use sortorder lib for sorting the output of
network listcommand- How I did it
Used
NaturalLessfor ordering the output, consistent with other commands- How to verify it
docker network list --format '{{ .Name }}'- Description for the changelog
network list command uses sortorder lib to sort the output
- A picture of a cute animal (not mandatory but encouraged)