Repository: cli/cli
Pull request #7884 (2023-08-22 09:57:50)

feat: delete local tag when running `gh release delete --cleanup-tag`
<!--
  Thank you for contributing to GitHub CLI!
  To reference an open issue, please write this in your description: `Fixes #NUMBER`
-->

Fixes https://github.com/cli/cli/issues/7853

andyfeller: @kemingy : Thank you for opening up this pull request! ✨ 

I see you were discussing this with Sam and believe I understand the problem.  Let me review the changes to see how we can close this gap! 🙇 
andyfeller: @kemingy : Think everything looks good, going to see how the tests run.  However, your branch will need to be updated before being able to clear required status checks.
kemingy: > @kemingy : Think everything looks good, going to see how the tests run. However, your branch will need to be updated before being able to clear required status checks.

Updated.
kemingy: > @kemingy Thanks for getting started on this work. There is one improvement I would like to see made before we get this merged. We need to be more defensive about when we try to delete the local tag. The `release delete` command can be run from anywhere, not necessarily in a local git repository, and not necessarily in the correct local git repository. We should do some checking that the local repository matches the GitHub repository before tying to delete the local tag.

Done. Please take a look.

Patch:
diff --git a/git/client.go b/git/client.go
index a029c8ba6..d90990cc7 100644
--- a/git/client.go
+++ b/git/client.go
@@ -322,6 +322,19 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc
 	return
 }
 
+func (c *Client) DeleteLocalTag(ctx context.Context, tag string) error {
+	args := []string{"tag", "-d", tag}
+	cmd, err := c.Command(ctx, args...)
+	if err != nil {
+		return err
+	}
+	_, err = cmd.Output()
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
 func (c *Client) DeleteLocalBranch(ctx context.Context, branch string) error {
 	args := []string{"branch", "-D", branch}
 	cmd, err := c.Command(ctx, args...)
diff --git a/pkg/cmd/release/delete/delete.go b/pkg/cmd/release/delete/delete.go
index 94b09cfcb..622b18893 100644
--- a/pkg/cmd/release/delete/delete.go
+++ b/pkg/cmd/release/delete/delete.go
@@ -6,6 +6,7 @@ import (
 	"net/http"
 
 	"github.com/cli/cli/v2/api"
+	"github.com/cli/cli/v2/git"
 	"github.com/cli/cli/v2/internal/ghinstance"
 	"github.com/cli/cli/v2/internal/ghrepo"
 	"github.com/cli/cli/v2/pkg/cmd/release/shared"
@@ -19,10 +20,12 @@ type iprompter interface {
 }
 
 type DeleteOptions struct {
-	HttpClient func() (*http.Client, error)
-	IO         *iostreams.IOStreams
-	BaseRepo   func() (ghrepo.Interface, error)
-	Prompter   iprompter
+	HttpClient   func() (*http.Client, error)
+	GitClient    *git.Client
+	IO           *iostreams.IOStreams
+	BaseRepo     func() (ghrepo.Interface, error)
+	RepoOverride string
+	Prompter     iprompter
 
 	TagName     string
 	SkipConfirm bool
@@ -33,6 +36,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co
 	opts := &DeleteOptions{
 		IO:         f.IOStreams,
 		HttpClient: f.HttpClient,
+		GitClient:  f.GitClient,
 		Prompter:   f.Prompter,
 	}
 
@@ -43,6 +47,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co
 		RunE: func(cmd *cobra.Command, args []string) error {
 			// support `-R, --repo` override
 			opts.BaseRepo = f.BaseRepo
+			opts.RepoOverride, _ = cmd.Flags().GetString("repo")
 
 			opts.TagName = args[0]
 
@@ -93,12 +98,13 @@ func deleteRun(opts *DeleteOptions) error {
 	}
 
 	var cleanupMessage string
-	mustCleanupTag := opts.CleanupTag
-	if mustCleanupTag {
-		err = deleteTag(httpClient, baseRepo, release.TagName)
-		if err != nil {
+	if opts.CleanupTag {
+		if err := deleteTag(httpClient, baseRepo, release.TagName); err != nil {
 			return err
 		}
+		if opts.RepoOverride == "" {
+			_ = opts.GitClient.DeleteLocalTag(context.Background(), release.TagName)
+		}
 		cleanupMessage = " and tag"
 	}
 
@@ -108,7 +114,7 @@ func deleteRun(opts *DeleteOptions) error {
 
 	iofmt := opts.IO.ColorScheme()
 	fmt.Fprintf(opts.IO.ErrOut, "%s Deleted release%s %s\n", iofmt.SuccessIconWithColor(iofmt.Red), cleanupMessage, release.TagName)
-	if !release.IsDraft && !mustCleanupTag {
+	if !release.IsDraft && !opts.CleanupTag {
 		fmt.Fprintf(opts.IO.ErrOut, "%s Note that the %s git tag still remains in the repository\n", iofmt.WarningIcon(), release.TagName)
 	}
 


Test patch:
diff --git a/git/client_test.go b/git/client_test.go
index 5e96bb1b7..841a496da 100644
--- a/git/client_test.go
+++ b/git/client_test.go
@@ -558,6 +558,45 @@ func TestClientReadBranchConfig(t *testing.T) {
 	}
 }
 
+func TestClientDeleteLocalTag(t *testing.T) {
+	tests := []struct {
+		name          string
+		cmdExitStatus int
+		cmdStdout     string
+		cmdStderr     string
+		wantCmdArgs   string
+		wantErrorMsg  string
+	}{
+		{
+			name:        "delete local tag",
+			wantCmdArgs: `path/to/git tag -d v1.0`,
+		},
+		{
+			name:          "git error",
+			cmdExitStatus: 1,
+			cmdStderr:     "git error message",
+			wantCmdArgs:   `path/to/git tag -d v1.0`,
+			wantErrorMsg:  "failed to run git: git error message",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			cmd, cmdCtx := createCommandContext(t, tt.cmdExitStatus, tt.cmdStdout, tt.cmdStderr)
+			client := Client{
+				GitPath:        "path/to/git",
+				commandContext: cmdCtx,
+			}
+			err := client.DeleteLocalTag(context.Background(), "v1.0")
+			assert.Equal(t, tt.wantCmdArgs, strings.Join(cmd.Args[3:], " "))
+			if tt.wantErrorMsg == "" {
+				assert.NoError(t, err)
+			} else {
+				assert.EqualError(t, err, tt.wantErrorMsg)
+			}
+		})
+	}
+}
+
 func TestClientDeleteLocalBranch(t *testing.T) {
 	tests := []struct {
 		name          string
diff --git a/pkg/cmd/release/delete/delete_test.go b/pkg/cmd/release/delete/delete_test.go
index c0c43f990..2787f247b 100644
--- a/pkg/cmd/release/delete/delete_test.go
+++ b/pkg/cmd/release/delete/delete_test.go
@@ -7,8 +7,10 @@ import (
 	"testing"
 
 	"github.com/MakeNowJust/heredoc"
+	"github.com/cli/cli/v2/git"
 	"github.com/cli/cli/v2/internal/ghrepo"
 	"github.com/cli/cli/v2/internal/prompter"
+	"github.com/cli/cli/v2/internal/run"
 	"github.com/cli/cli/v2/pkg/cmd/release/shared"
 	"github.com/cli/cli/v2/pkg/cmdutil"
 	"github.com/cli/cli/v2/pkg/httpmock"
@@ -110,6 +112,7 @@ func Test_deleteRun(t *testing.T) {
 		isTTY         bool
 		opts          DeleteOptions
 		prompterStubs func(*prompter.PrompterMock)
+		runStubs      func(*run.CommandStubber)
 		wantErr       string
 		wantStdout    string
 		wantStderr    string
@@ -164,6 +167,9 @@ func Test_deleteRun(t *testing.T) {
 				SkipConfirm: true,
 				CleanupTag:  true,
 			},
+			runStubs: func(rs *run.CommandStubber) {
+				rs.Register(`git tag -d v1.2.3`, 0, "")
+			},
 			wantStdout: ``,
 			wantStderr: heredoc.Doc(`
 				✓ Deleted release and tag v1.2.3
@@ -177,6 +183,9 @@ func Test_deleteRun(t *testing.T) {
 				SkipConfirm: false,
 				CleanupTag:  true,
 			},
+			runStubs: func(rs *run.CommandStubber) {
+				rs.Register(`git tag -d v1.2.3`, 0, "")
+			},
 			wantStdout: ``,
 			wantStderr: ``,
 		},
@@ -203,6 +212,12 @@ func Test_deleteRun(t *testing.T) {
 			fakeHTTP.Register(httpmock.REST("DELETE", "repos/OWNER/REPO/releases/23456"), httpmock.StatusStringResponse(204, ""))
 			fakeHTTP.Register(httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/tags/v1.2.3"), httpmock.StatusStringResponse(204, ""))
 
+			rs, teardown := run.Stub()
+			defer teardown(t)
+			if tt.runStubs != nil {
+				tt.runStubs(rs)
+			}
+
 			tt.opts.IO = ios
 			tt.opts.Prompter = pm
 			tt.opts.HttpClient = func() (*http.Client, error) {
@@ -211,6 +226,7 @@ func Test_deleteRun(t *testing.T) {
 			tt.opts.BaseRepo = func() (ghrepo.Interface, error) {
 				return ghrepo.FromFullName("OWNER/REPO")
 			}
+			tt.opts.GitClient = &git.Client{GitPath: "some/path/git"}
 
 			err := deleteRun(&tt.opts)
 			if tt.wantErr != "" {