feat: add DeleteBranch() and ClosePullRequest() (#2232)

test: update system test to clean up branch and close the pull request.

Followup to #2210 that cleans up the branch we created and closes the
pull request.
Towards #300

Signed-off-by: Jeff Ching <[email protected]>

Author: chingor13

internal/github/github.go

@@ -332,3 +332,13 @@ func (c *Client) CreateTag(ctx context.Context, tagName, commitSHA string) error
 	_, _, err := c.Git.CreateRef(ctx, c.repo.Owner, c.repo.Name, tagRef)
 	return err
 }
+
+// ClosePullRequest closes the pull request specified by pull request number.
+func (c *Client) ClosePullRequest(ctx context.Context, number int) error {
+	slog.Info("Closing pull request", slog.Int("number", number))
+	state := "closed"
+	_, _, err := c.PullRequests.Edit(ctx, c.repo.Owner, c.repo.Name, number, &github.PullRequest{
+		State: &state,
+	})
+	return err
+}

internal/gitrepo/gitrepo.go

@@ -46,6 +46,7 @@ type Repository interface {
 	CreateBranchAndCheckout(name string) error
 	Push(branchName string) error
 	Restore(paths []string) error
+	pushRefSpec(refSpec string) error
 }
 
 // LocalRepository represents a git repository.
@@ -429,8 +430,19 @@ func (r *LocalRepository) CreateBranchAndCheckout(name string) error {
 // Push pushes the local branch to the origin remote.
 func (r *LocalRepository) Push(branchName string) error {
 	// https://stackoverflow.com/a/75727620
-	refSpec := config.RefSpec(fmt.Sprintf("+refs/heads/%s:refs/heads/%s", branchName, branchName))
+	refSpec := fmt.Sprintf("+refs/heads/%s:refs/heads/%s", branchName, branchName)
 	slog.Info("Pushing changes", "branch name", branchName, slog.Any("refspec", refSpec))
+	return r.pushRefSpec(refSpec)
+}
+
+// DeleteBranch deletes a branch on the origin remote.
+func (r *LocalRepository) DeleteBranch(branchName string) error {
+	refSpec := fmt.Sprintf(":refs/heads/%s", branchName)
+	return r.pushRefSpec(refSpec)
+}
+
+func (r *LocalRepository) pushRefSpec(refSpec string) error {
+	slog.Info("Pushing changes", "refSpec", refSpec)
 	var auth *httpAuth.BasicAuth
 	if r.gitPassword != "" {
 		slog.Info("Authenticating with basic auth")
@@ -443,12 +455,12 @@ func (r *LocalRepository) Push(branchName string) error {
 	}
 	if err := r.repo.Push(&git.PushOptions{
 		RemoteName: "origin",
-		RefSpecs:   []config.RefSpec{refSpec},
+		RefSpecs:   []config.RefSpec{config.RefSpec(refSpec)},
 		Auth:       auth,
 	}); err != nil {
 		return err
 	}
-	slog.Info("Successfully pushed branch to remote 'origin", "branch", branchName)
+	slog.Info("Successfully pushed changes", "refSpec", refSpec)
 	return nil
 }
 

system_test.go

@@ -16,6 +16,8 @@ package librarian
 
 import (
 	"context"
+	"fmt"
+	"log/slog"
 	"os"
 	"path"
 	"strings"
@@ -105,6 +107,7 @@ func TestPullRequestSystem(t *testing.T) {
 	// Replace the issue labels
 	// Search for the pull request
 	// Fetch the pull request
+	// Close the pull request
 	if testToken == "" {
 		t.Skip("TEST_GITHUB_TOKEN not set, skipping GitHub integration test")
 	}
@@ -131,7 +134,8 @@ func TestPullRequestSystem(t *testing.T) {
 		t.Fatalf("unexpected error in ParseRemote() %s", err)
 	}
 
-	branchName := "integration-test"
+	now := time.Now()
+	branchName := fmt.Sprintf("integration-test-%s", now.Format("20060102150405"))
 	err = localRepository.CreateBranchAndCheckout(branchName)
 	if err != nil {
 		t.Fatalf("unexpected error in CreateBranchAndCheckout() %s", err)
@@ -155,23 +159,40 @@ func TestPullRequestSystem(t *testing.T) {
 		t.Fatalf("unexpected error in Push() %s", err)
 	}
 
+	cleanupBranch := func() {
+		slog.Info("cleaning up created branch", "branch", branchName)
+		err := localRepository.DeleteBranch(branchName)
+		if err != nil {
+			t.Fatalf("unexpected error in DeleteBranch() %s", err)
+		}
+	}
+	defer cleanupBranch()
+
 	// Create a pull request
 	client := github.NewClient(testToken, repo)
-	metadata, err := client.CreatePullRequest(t.Context(), repo, branchName, "main", "test: integration test", "do not merge")
+	createdPullRequest, err := client.CreatePullRequest(t.Context(), repo, branchName, "main", "test: integration test", "do not merge")
 	if err != nil {
 		t.Fatalf("unexpected error in CreatePullRequest() %s", err)
 	}
-	t.Logf("created pull request: %d", metadata.Number)
+	t.Logf("created pull request: %d", createdPullRequest.Number)
+
+	// Ensure we clean up the created PR. The pull request is closed later in the
+	// test, but this should make sure unless ClosePullRequest() is not working.
+	cleanupPR := func() {
+		slog.Info("cleaning up opened pull request")
+		client.ClosePullRequest(t.Context(), createdPullRequest.Number)
+	}
+	defer cleanupPR()
 
 	// Add a label to the pull request
 	labels := []string{"do not merge", "type: cleanup"}
-	err = client.AddLabelsToIssue(t.Context(), repo, metadata.Number, labels)
+	err = client.AddLabelsToIssue(t.Context(), repo, createdPullRequest.Number, labels)
 	if err != nil {
 		t.Fatalf("unexpected error in AddLabelsToIssue() %s", err)
 	}
 
 	// Get labels and verify
-	foundLabels, err := client.GetLabels(t.Context(), metadata.Number)
+	foundLabels, err := client.GetLabels(t.Context(), createdPullRequest.Number)
 	if err != nil {
 		t.Fatalf("unexpected error in GetLabels() %s", err)
 	}
@@ -181,13 +202,13 @@ func TestPullRequestSystem(t *testing.T) {
 
 	// Replace labels
 	labels = []string{"foo", "bar"}
-	err = client.ReplaceLabels(t.Context(), metadata.Number, labels)
+	err = client.ReplaceLabels(t.Context(), createdPullRequest.Number, labels)
 	if err != nil {
 		t.Fatalf("unexpected error in ReplaceLabels() %s", err)
 	}
 
 	// Get labels and verify
-	foundLabels, err = client.GetLabels(t.Context(), metadata.Number)
+	foundLabels, err = client.GetLabels(t.Context(), createdPullRequest.Number)
 	if err != nil {
 		t.Fatalf("unexpected error in GetLabels() %s", err)
 	}
@@ -196,14 +217,14 @@ func TestPullRequestSystem(t *testing.T) {
 	}
 
 	// Add label
-	err = client.AddLabelsToIssue(t.Context(), repo, metadata.Number, []string{"librarian-test", "asdf"})
+	err = client.AddLabelsToIssue(t.Context(), repo, createdPullRequest.Number, []string{"librarian-test", "asdf"})
 	if err != nil {
 		t.Fatalf("unexpected error in AddLabelsToIssue() %s", err)
 	}
 
 	// Get labels and verify
 	wantLabels := []string{"foo", "bar", "librarian-test", "asdf"}
-	foundLabels, err = client.GetLabels(t.Context(), metadata.Number)
+	foundLabels, err = client.GetLabels(t.Context(), createdPullRequest.Number)
 	if err != nil {
 		t.Fatalf("unexpected error in GetLabels() %s", err)
 	}
@@ -220,7 +241,7 @@ func TestPullRequestSystem(t *testing.T) {
 		}
 		for _, pullRequest := range foundPullRequests {
 			// Look for the PR we created
-			if *pullRequest.Number == metadata.Number {
+			if *pullRequest.Number == createdPullRequest.Number {
 				found = true
 				break
 			}
@@ -233,12 +254,21 @@ func TestPullRequestSystem(t *testing.T) {
 		t.Fatalf("failed to find pull request after 5 attempts")
 	}
 
+	// Close pull request
+	err = client.ClosePullRequest(t.Context(), createdPullRequest.Number)
+	if err != nil {
+		t.Fatalf("unexpected error in ClosePullRequest() %s", err)
+	}
+
 	// Get single pull request
-	foundPullRequest, err := client.GetPullRequest(t.Context(), metadata.Number)
+	foundPullRequest, err := client.GetPullRequest(t.Context(), createdPullRequest.Number)
 	if err != nil {
 		t.Fatalf("unexpected error in GetPullRequest() %s", err)
 	}
-	if diff := cmp.Diff(*foundPullRequest.Number, metadata.Number); diff != "" {
+	if diff := cmp.Diff(*foundPullRequest.Number, createdPullRequest.Number); diff != "" {
 		t.Fatalf("pull request number mismatch (-want + got):\n%s", diff)
 	}
+	if diff := cmp.Diff(*foundPullRequest.State, "closed"); diff != "" {
+		t.Fatalf("pull request state mismatch (-want + got):\n%s", diff)
+	}
 }