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

use sortorder library for sorting network list output #1266

Merged

Conversation

adshmh
Copy link
Contributor

@adshmh adshmh commented Aug 3, 2018

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

codecov-io commented Aug 3, 2018

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
Copy link
Contributor Author

adshmh commented Aug 3, 2018

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

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

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, thanks!

@thaJeztah
Copy link
Member

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>
@adshmh adshmh force-pushed the use-natural-sort-order-for-network-list branch from 1ea6234 to 17f9236 Compare August 8, 2018 16:07
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
Signed-off-by: Arash Deshmeh <adeshmeh@ca.ibm.com>
@adshmh adshmh force-pushed the use-natural-sort-order-for-network-list branch from 17f9236 to 5cc1f90 Compare August 8, 2018 16:21
@adshmh
Copy link
Contributor Author

adshmh commented Aug 8, 2018

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:

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.

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a 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}}
Copy link
Contributor

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}}

Copy link
Contributor Author

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.

Copy link
Member

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 🤗

@silvin-lubecki
Copy link
Contributor

@thaJeztah PTAL

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, thanks!

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

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

@thaJeztah thaJeztah merged commit 6ef11c5 into docker:master Aug 9, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.09.0 milestone Aug 9, 2018
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.

5 participants