use sortorder library for sorting network list output by adshmh · Pull Request #1266 · docker/cli

adshmh · 2018-08-03T11:23:29Z

- What I did
Use sortorder lib for sorting the output of network list command

- How I did it
Used NaturalLess for ordering the output, consistent with other commands

- How to verify it

create 3 networks: net-1, net-2, net-10
run docker network list --format '{{ .Name }}'
output:

...
net-1
net-10
net-2
...

output after the changes made by this PR:

...
net-1
net-2
net-10
...

- Description for the changelog
network list command uses sortorder lib to sort the output

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

codecov-io · 2018-08-03T11:28:57Z

Codecov Report

Merging #1266 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1266   +/-   ##
=======================================
  Coverage   54.29%   54.29%           
=======================================
  Files         268      268           
  Lines       17855    17855           
=======================================
  Hits         9695     9695           
  Misses       7550     7550           
  Partials      610      610

adshmh · 2018-08-03T11:29:12Z

There are still a few more commands, e.g. trust inspect and volume list where direct comparison is used. I can update the PR to modify those as well if such a change is desirable.

silvin-lubecki · 2018-08-03T16:06:04Z

Thank you @adshmh ! Yes please follow with the other commands in this very PR. Otherwise LGTM 😄

thaJeztah

LGTM, thanks!

thaJeztah · 2018-08-07T16:53:00Z

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 <[email protected]>

adshmh · 2018-08-08T16:31:51Z

Thank you for the reviews. I have updated the PR.

There are 2 remaining instances of direct comparison in the trust package, in cli/command/trust/inspect.go, but one is on canonical base role names, i.e. not user-defined. The second one:

cli/cli/command/trust/inspect.go

Lines 66 to 89 in 1d04f7d

    
           func getRepoTrustInfo(cli command.Cli, remote string) ([]byte, error) { 
        
           	signatureRows, adminRolesWithSigs, delegationRoles, err := lookupTrustInfo(cli, remote) 
        
           	if err != nil { 
        
           		return []byte{}, err 
        
           	} 
        
           	// process the signatures to include repo admin if signed by the base targets role 
        
           	for idx, sig := range signatureRows { 
        
           		if len(sig.Signers) == 0 { 
        
           			signatureRows[idx].Signers = append(sig.Signers, releasedRoleName) 
        
           		} 
        
           	} 
        
           	signerList, adminList := []trustSigner{}, []trustSigner{} 
        
           	signerRoleToKeyIDs := getDelegationRoleToKeyMap(delegationRoles) 
        
           	for signerName, signerKeys := range signerRoleToKeyIDs { 
        
           		signerKeyList := []trustKey{} 
        
           		for _, keyID := range signerKeys { 
        
           			signerKeyList = append(signerKeyList, trustKey{ID: keyID}) 
        
           		} 
        
           		signerList = append(signerList, trustSigner{signerName, signerKeyList}) 
        
           	} 
        
           	sort.Slice(signerList, func(i, j int) bool { return signerList[i].Name > signerList[j].Name })

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.

silvin-lubecki

LGTM (with a small nit), thank you @adshmh !!

silvin-lubecki · 2018-08-09T09:50:38Z

cli/command/trust/common_test.go

+)
+
+func TestMatchReleasedSignaturesSortOrder(t *testing.T) {
+	var releasesRole = data.DelegationRole{BaseRole: data.BaseRole{Name: trust.ReleasesRole}}


nit: releasesRole := data.DelegationRole{BaseRole: data.BaseRole{Name: trust.ReleasesRole}}

Thank you for the review. Seems the PR was merged before I modify this.

No worries; I looked at the comment, and thought it to be not relevant enough to update before merging 🤗

silvin-lubecki · 2018-08-09T09:53:06Z

@thaJeztah PTAL

thaJeztah

LGTM, thanks!

thaJeztah · 2018-08-09T10:05:29Z

cli/command/trust/inspect_pretty_test.go

+func TestPrintSignerInfoSortOrder(t *testing.T) {
+	roleToKeyIDs := map[string][]string{
+		"signer2-foo":  {"B"},
+		"signer10-foo": {"C"},


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

GordonTheTurtle added the status/0-triage label Aug 3, 2018

thaJeztah approved these changes Aug 7, 2018

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels Aug 7, 2018

use sortorder lib for sorting network list output

a921313

Signed-off-by: Arash Deshmeh <[email protected]>

adshmh force-pushed the use-natural-sort-order-for-network-list branch from 1ea6234 to 17f9236 Compare August 8, 2018 16:07

adshmh added 2 commits August 8, 2018 12:20

use sortorder lib for sorting in trust package

04bb3c7

Signed-off-by: Arash Deshmeh <[email protected]>

use sortorder lib for sorting the output of volume list command

5cc1f90

Signed-off-by: Arash Deshmeh <[email protected]>

adshmh force-pushed the use-natural-sort-order-for-network-list branch from 17f9236 to 5cc1f90 Compare August 8, 2018 16:21

silvin-lubecki approved these changes Aug 9, 2018

View reviewed changes

thaJeztah approved these changes Aug 9, 2018

View reviewed changes

thaJeztah merged commit 6ef11c5 into docker:master Aug 9, 2018

GordonTheTurtle added this to the 18.09.0 milestone Aug 9, 2018

Conversation

adshmh commented Aug 3, 2018

Uh oh!

codecov-io commented Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

adshmh commented Aug 3, 2018

Uh oh!

silvin-lubecki commented Aug 3, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Aug 7, 2018

Uh oh!

adshmh commented Aug 8, 2018

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki Aug 9, 2018

Choose a reason for hiding this comment

Uh oh!

adshmh Aug 10, 2018

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 10, 2018

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki commented Aug 9, 2018

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 9, 2018

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Aug 3, 2018 •

edited

Loading