From 448082f333f0b4d7774c4085927e1206459fd2a0 Mon Sep 17 00:00:00 2001
From: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
Date: Tue, 5 Jun 2018 16:04:41 -0700
Subject: [PATCH] Fix docker trust signer removal

Signed-off-by: Nassim 'Nass' Eddequiouaq <eddequiouaq.nassim@gmail.com>
---
 cli/command/trust/signer_remove.go      | 35 +++++++++++++++----------
 cli/command/trust/signer_remove_test.go |  4 +--
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/cli/command/trust/signer_remove.go b/cli/command/trust/signer_remove.go
index 27774632bf..a8b697c242 100644
--- a/cli/command/trust/signer_remove.go
+++ b/cli/command/trust/signer_remove.go
@@ -43,11 +43,13 @@ func removeSigner(cli command.Cli, options signerRemoveOptions) error {
 	var errRepos []string
 	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 {
 			fmt.Fprintln(cli.Err(), err.Error()+"\n")
 			errRepos = append(errRepos, repo)
 		} else {
-			fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo)
+			if didRemove {
+				fmt.Fprintf(cli.Out(), "Successfully removed %s from %s\n\n", options.signer, repo)
+			}
 		}
 	}
 	if len(errRepos) > 0 {
@@ -78,24 +80,24 @@ func isLastSignerForReleases(roleWithSig data.Role, allRoles []client.RoleWithSi
 	return counter < releasesRoleWithSigs.Threshold, nil
 }
 
-func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes bool) error {
+func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes bool) (bool, error) {
 	ctx := context.Background()
 	imgRefAndAuth, err := trust.GetImageReferencesAndAuth(ctx, nil, image.AuthResolver(cli), repoName)
 	if err != nil {
-		return err
+		return false, err
 	}
 
 	signerDelegation := data.RoleName("targets/" + signerName)
 	if signerDelegation == releasesRoleTUFName {
-		return fmt.Errorf("releases is a reserved keyword and cannot be removed")
+		return false, fmt.Errorf("releases is a reserved keyword and cannot be removed")
 	}
 	notaryRepo, err := cli.NotaryClient(imgRefAndAuth, trust.ActionsPushAndPull)
 	if err != nil {
-		return trust.NotaryError(imgRefAndAuth.Reference().Name(), err)
+		return false, trust.NotaryError(imgRefAndAuth.Reference().Name(), err)
 	}
 	delegationRoles, err := notaryRepo.GetDelegationRoles()
 	if err != nil {
-		return errors.Wrapf(err, "error retrieving signers for %s", repoName)
+		return false, errors.Wrapf(err, "error retrieving signers for %s", repoName)
 	}
 	var role data.Role
 	for _, delRole := range delegationRoles {
@@ -105,11 +107,11 @@ func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes b
 		}
 	}
 	if role.Name == "" {
-		return fmt.Errorf("No signer %s for repository %s", signerName, repoName)
+		return false, fmt.Errorf("No signer %s for repository %s", signerName, repoName)
 	}
 	allRoles, err := notaryRepo.ListRoles()
 	if err != nil {
-		return err
+		return false, err
 	}
 	if ok, err := isLastSignerForReleases(role, allRoles); ok && !forceYes {
 		removeSigner := command.PromptForConfirmation(os.Stdin, cli.Out(), fmt.Sprintf("The signer \"%s\" signed the last released version of %s. "+
@@ -120,16 +122,21 @@ func removeSingleSigner(cli command.Cli, repoName, signerName string, forceYes b
 
 		if !removeSigner {
 			fmt.Fprintf(cli.Out(), "\nAborting action.\n")
-			return nil
+			return false, nil
 		}
 	} else if err != nil {
-		return err
+		return false, err
 	}
 	if err = notaryRepo.RemoveDelegationKeys(releasesRoleTUFName, role.KeyIDs); err != nil {
-		return err
+		return false, err
 	}
 	if err = notaryRepo.RemoveDelegationRole(signerDelegation); err != nil {
-		return err
+		return false, err
 	}
-	return notaryRepo.Publish()
+
+	if err = notaryRepo.Publish(); err != nil {
+		return false, err
+	}
+
+	return true, nil
 }
diff --git a/cli/command/trust/signer_remove_test.go b/cli/command/trust/signer_remove_test.go
index 4f11634be2..f2ca1e7af8 100644
--- a/cli/command/trust/signer_remove_test.go
+++ b/cli/command/trust/signer_remove_test.go
@@ -71,9 +71,9 @@ func TestTrustSignerRemoveErrors(t *testing.T) {
 func TestRemoveSingleSigner(t *testing.T) {
 	cli := test.NewFakeCli(&fakeClient{})
 	cli.SetNotaryClient(notaryfake.GetLoadedNotaryRepository)
-	err := removeSingleSigner(cli, "signed-repo", "test", true)
+	_, err := removeSingleSigner(cli, "signed-repo", "test", true)
 	assert.Error(t, err, "No signer test for repository signed-repo")
-	err = removeSingleSigner(cli, "signed-repo", "releases", true)
+	_, err = removeSingleSigner(cli, "signed-repo", "releases", true)
 	assert.Error(t, err, "releases is a reserved keyword and cannot be removed")
 }