🔍 Deep Dive: Fix docker trust signer removal by n4ss · Pull Request #1112 · docker/cli · GitHub

n4ss · 2018-06-06T02:18:28Z

Signed-off-by: Nassim 'Nass' Eddequiouaq eddequiouaq.nassim@gmail.com

- What I did

Only print "Successfully removed " if the signer is removed.

- How I did it

Make trust.removeSingleSigner return a second value besides the error; whether or not a removal actually happened and decides on output printing based on it.

- How to verify it

$> docker signer add foobar myrepo/isthebest --key path/to/pubkey
$> docker sign myrepo/isthebest:latest
$> docker signer remove foobar myrepo/isthebest
The signer "foobar" signed the last released version of myrepo/isthebest. Removing this signer will make myrepo/isthebest unpullable. Are you sure you want to continue? [y/N]
N

Aborting action.

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

n4ss · 2018-06-06T02:19:21Z

/cc @vdemeester @thaJeztah @moscowrage @endophage @vieux

this is a very high priority btw for the next release.

mat007 · 2018-06-06T04:46:28Z

cli/command/trust/signer_remove_test.go

 	cli := test.NewFakeCli(&fakeClient{})
 	cli.SetNotaryClient(notaryfake.GetLoadedNotaryRepository)
-	err := removeSingleSigner(cli, "signed-repo", "test", true)
+	_, err := removeSingleSigner(cli, "signed-repo", "test", true)


Don't we want to check the returned boolean?

mat007 · 2018-06-06T04:49:06Z

cli/command/trust/signer_remove.go

 			errRepos = append(errRepos, repo)
 		} else {
-			fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo)
+			if didRemove {


nit: you can do

} else if didRemove {

mat007 · 2018-06-06T04:53:29Z

cli/command/trust/signer_remove.go

 	}
-	return notaryRepo.Publish()
+
+	return true, nil


Wouldn't it be simpler to move the logged success message before this return?

fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", signerName, repoName) return nil

?

I'd say that I prefer to have it outside as it makes it more maintainable in the case where there are different codepaths that lead to a successful removal in this function and simply have this one return whether or not a removal happened.

OK but for now there is only one code path which does that, this would minimize the changes required and there is already output logged in a similar way in the function e.g.

fmt.Fprintf(cli.Out(), "\nAborting action.\n")

Anyway it's just my personal view, obviously :)

Makes sense! fixed.

vdemeester

gofmt

vdemeester · 2018-06-06T09:08:06Z

cli/command/trust/signer_remove.go

-		} else {
-			fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo)
+		} else if didRemove {
+				fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo)


Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

n4ss · 2018-06-06T14:26:06Z

@vdemeester done

vdemeester

LGTM 🐸

n4ss · 2018-06-07T18:57:15Z

cc @andrewhsu as well

mat007 · 2018-06-07T19:46:52Z

cli/command/trust/signer_remove.go

 	for _, repo := range options.repos {
 		fmt.Fprintf(cli.Out(), "Removing signer \"%s\" from %s...\n", options.signer, repo)
-		if err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil {
+		if didRemove, err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil {


nit: didRemove -> removed maybe?

silvin-lubecki · 2018-06-08T14:49:11Z

cli/command/trust/signer_remove_test.go

+
+	_, err = removeSingleSigner(cli, "signed-repo", "releases", true)
 	assert.Error(t, err, "releases is a reserved keyword and cannot be removed")
+	assert.Equal(t, didRemove, false, "No signer should be removed")


I may miss something, but I think didRemove was not updated on the second call here (as the boolean result is dropped _, err = removeSingleSigner)

good catch thanks, done.

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

mat007 · 2018-06-09T14:56:25Z

cli/command/trust/signer_remove.go

 	for _, repo := range options.repos {
 		fmt.Fprintf(cli.Out(), "Removing signer \"%s\" from %s...\n", options.signer, repo)
-		if err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil {
+		if _, err := removeSingleSigner(cli, repo, options.signer, options.forceYes); err != nil {


I would argue that there is no need to return a bool as all now that it's dropped here.

I will implement more tests that need to know if the signer got actually removed, and I need the information through this boolean.

mat007 · 2018-06-09T15:02:40Z

cli/command/trust/signer_remove.go


-func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes bool) error {
+// removeSingleSigner returns whether the signer has been removed during this operation and an error
+// Note: the signer not being removed doesn't necessarily raise an error (eg. User saying "No" to the confirmation prompt)


Besides dropping the bool I would remove the Note: and add punctuation, something like:

removeSingleSigner attempts to remove a single signer. The signer not being removed doesn't necessarily raise an error e.g. user choosing "No" when prompted for confirmation.

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

silvin-lubecki

LGTM 👍

thaJeztah

LGTM, but not very fond of the boolean return. Looks like there may be a bit too much logic in that function, so we should do some refactoring in a follow up

thaJeztah · 2018-06-15T17:45:43Z

Arf; probably should've needed some squashing 😞

Fix docker trust signer removal

448082f

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

GordonTheTurtle added the status/0-triage label Jun 6, 2018

mat007 reviewed Jun 6, 2018

View reviewed changes

vdemeester added status/2-code-review priority/P0 priority/P1 and removed status/0-triage priority/P0 labels Jun 6, 2018

vdemeester requested review from dnephin, silvin-lubecki and thaJeztah June 6, 2018 09:07

vdemeester requested changes Jun 6, 2018

View reviewed changes

Fix tests and nit

5ebb7a6

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

n4ss force-pushed the fix-trust-signer-remove branch from ccd0697 to 5ebb7a6 Compare June 6, 2018 14:25

vdemeester approved these changes Jun 6, 2018

View reviewed changes

mat007 reviewed Jun 7, 2018

View reviewed changes

silvin-lubecki reviewed Jun 8, 2018

View reviewed changes

Move the successful removal print

2b3361c

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

mat007 reviewed Jun 9, 2018

View reviewed changes

Fix removeSingleSigner description

92c39dd

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>

silvin-lubecki approved these changes Jun 11, 2018

View reviewed changes

thaJeztah approved these changes Jun 15, 2018

View reviewed changes

thaJeztah merged commit 8de0753 into docker:master Jun 15, 2018

GordonTheTurtle added this to the 18.06.0 milestone Jun 15, 2018

Feb	MAR	Apr
	14
2025	2026	2027

Conversation

n4ss commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

n4ss commented Jun 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n4ss commented Jun 6, 2018

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

n4ss commented Jun 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jun 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

n4ss commented Jun 6, 2018 •

edited

Loading

n4ss commented Jun 6, 2018 •

edited

Loading