pr: Fail when the CI status fails

In this case the PR will probably not be merged, but we can fail waiting
for it if its combined status is failed.

Author: iainlane

cmd/wait-for-github/pr.go

@@ -111,7 +111,7 @@ func parsePRArguments(c *cli.Context) (prConfig, error) {
 		repo:           repo,
 		pr:             n,
 		commitInfoFile: c.String("commit-info-file"),
-		writer: 	   osFileWriter{},
+		writer:         osFileWriter{},
 	}, nil
 }
 
@@ -122,10 +122,16 @@ type commitInfo struct {
 	MergedAt int64  `json:"mergedAt"`
 }
 
+type checkMergedAndOverallCI interface {
+	github.CheckPRMerged
+	github.GetPRHeadSHA
+	github.CheckOverallCIStatus
+}
+
 type prCheck struct {
 	prConfig
 
-	githubClient github.CheckPRMerged
+	githubClient checkMergedAndOverallCI
 }
 
 func (pr prCheck) Check(ctx context.Context, recheckInterval time.Duration) error {
@@ -161,11 +167,28 @@ func (pr prCheck) Check(ctx context.Context, recheckInterval time.Duration) erro
 		return cli.Exit("PR is closed", 1)
 	}
 
+	// not merged, not closed, let's see what the CI status is. If that's bad,
+	// we can exit early.
+	sha, err := pr.githubClient.GetPRHeadSHA(ctx, pr.owner, pr.repo, pr.pr)
+	if err != nil {
+		return err
+	}
+
+	status, err := pr.githubClient.GetCIStatus(ctx, pr.owner, pr.repo, sha)
+	if err != nil {
+		return err
+	}
+
+	if status == github.CIStatusFailed {
+		log.Info("CI failed, exiting")
+		return cli.Exit("CI failed", 1)
+	}
+
 	log.Infof("PR is not closed yet, rechecking in %s", recheckInterval)
 	return nil
 }
 
-func checkPRMerged(timeoutCtx context.Context, githubClient github.CheckPRMerged, cfg *config, prConf *prConfig) error {
+func checkPRMerged(timeoutCtx context.Context, githubClient checkMergedAndOverallCI, cfg *config, prConf *prConfig) error {
 	checkPRMergedOrClosed := prCheck{
 		githubClient: githubClient,
 		prConfig:     *prConf,

cmd/wait-for-github/pr_test.go

@@ -24,60 +24,90 @@ import (
 	"io/fs"
 	"testing"
 
+	"github.com/grafana/wait-for-github/internal/github"
 	"github.com/stretchr/testify/require"
 	"github.com/urfave/cli/v2"
 )
 
-// FakeGithubClientPRCheck implements CheckPRMerged
-type FakeGithubClientPRCheck struct {
+// fakeGithubClientPRCheck implements the checkMergedAndOverallCI interface
+type fakeGithubClientPRCheck struct {
 	MergedCommit string
 	Closed       bool
 	MergedAt     int64
-	Error        error
+
+	isPRMergedError   error
+	getPRHeadSHAError error
+	getCIStatusError  error
+
+	CIStatus github.CIStatus
+}
+
+func (fg *fakeGithubClientPRCheck) IsPRMergedOrClosed(ctx context.Context, owner, repo string, pr int) (string, bool, int64, error) {
+	return fg.MergedCommit, fg.Closed, fg.MergedAt, fg.isPRMergedError
 }
 
-func (fg *FakeGithubClientPRCheck) IsPRMergedOrClosed(ctx context.Context, owner, repo string, pr int) (string, bool, int64, error) {
-	return fg.MergedCommit, fg.Closed, fg.MergedAt, fg.Error
+func (fg *fakeGithubClientPRCheck) GetPRHeadSHA(ctx context.Context, owner, repo string, pr int) (string, error) {
+	return fg.MergedCommit, fg.getPRHeadSHAError
+}
+
+func (fg *fakeGithubClientPRCheck) GetCIStatus(ctx context.Context, owner, repo string, commitHash string) (github.CIStatus, error) {
+	return fg.CIStatus, fg.getCIStatusError
 }
 
 func TestPRCheck(t *testing.T) {
 	t.Parallel()
 
 	tests := []struct {
 		name             string
-		fakeClient       FakeGithubClientPRCheck
-		err              error
+		fakeClient       fakeGithubClientPRCheck
 		expectedExitCode *int
 	}{
 		{
 			name: "PR is merged",
-			fakeClient: FakeGithubClientPRCheck{
+			fakeClient: fakeGithubClientPRCheck{
 				MergedCommit: "abc123",
 				MergedAt:     1234567890,
 			},
 			expectedExitCode: &zero,
 		},
 		{
 			name: "PR is closed",
-			fakeClient: FakeGithubClientPRCheck{
+			fakeClient: fakeGithubClientPRCheck{
 				Closed: true,
 			},
 			expectedExitCode: &one,
 		},
 		{
 			name: "PR is open",
-			fakeClient: FakeGithubClientPRCheck{
-				MergedCommit: "",
-				Closed:       false,
+			fakeClient: fakeGithubClientPRCheck{
+				Closed: false,
 			},
 			expectedExitCode: &one,
 		},
 		{
 			name: "Error from IsPRMergedOrClosed",
-			fakeClient: FakeGithubClientPRCheck{
-				Error: fmt.Errorf("an error occurred"),
+			fakeClient: fakeGithubClientPRCheck{
+				isPRMergedError: fmt.Errorf("an error occurred"),
+			},
+		},
+		{
+			name: "Not merged, but CI failed",
+			fakeClient: fakeGithubClientPRCheck{
+				CIStatus: github.CIStatusFailed,
+			},
+			expectedExitCode: &one,
+		},
+		{
+			name: "Not merged, getting PR head SHA failed",
+			fakeClient: fakeGithubClientPRCheck{
+				getPRHeadSHAError: fmt.Errorf("an error occurred"),
+			},
+		},
+		{
+			name: "Not merged, getting CI status failed",
+			fakeClient: fakeGithubClientPRCheck{
+				getCIStatusError: fmt.Errorf("an error occurred"),
 			},
-			err: nil,
 		},
 	}
 
@@ -103,7 +133,7 @@ func TestPRCheck(t *testing.T) {
 				require.ErrorAs(t, err, &exitErr)
 				require.Equal(t, *tt.expectedExitCode, exitErr.ExitCode())
 			} else if err != nil {
-				require.ErrorAs(t, err, &tt.err)
+				require.NotNil(t, err)
 			}
 		})
 	}
@@ -138,7 +168,7 @@ func TestWriteCommitInfoFile(t *testing.T) {
 	prCheck := &prCheck{
 		prConfig: prConfig,
 
-		githubClient: &FakeGithubClientPRCheck{
+		githubClient: &fakeGithubClientPRCheck{
 			MergedCommit: "abc123",
 			MergedAt:     1234567890,
 		},
@@ -182,7 +212,7 @@ func TestWriteCommitInfoFileError(t *testing.T) {
 	prCheck := &prCheck{
 		prConfig: prConfig,
 
-		githubClient: &FakeGithubClientPRCheck{
+		githubClient: &fakeGithubClientPRCheck{
 			MergedCommit: "abc123",
 			MergedAt:     1234567890,
 		},

internal/github/github.go

@@ -34,11 +34,23 @@ type CheckPRMerged interface {
 	IsPRMergedOrClosed(ctx context.Context, owner, repo string, pr int) (string, bool, int64, error)
 }
 
-type CheckCIStatus interface {
+type GetPRHeadSHA interface {
+	GetPRHeadSHA(ctx context.Context, owner, repo string, pr int) (string, error)
+}
+
+type CheckOverallCIStatus interface {
 	GetCIStatus(ctx context.Context, owner, repo string, commitHash string) (CIStatus, error)
+}
+
+type CheckCIStatusForChecks interface {
 	GetCIStatusForChecks(ctx context.Context, owner, repo string, commitHash string, checkNames []string) (CIStatus, []string, error)
 }
 
+type CheckCIStatus interface {
+	CheckOverallCIStatus
+	CheckCIStatusForChecks
+}
+
 type AuthInfo struct {
 	InstallationID int64
 	AppID          int64
@@ -117,6 +129,15 @@ func (c GHClient) IsPRMergedOrClosed(ctx context.Context, owner, repo string, pr
 	return sha, pr.GetState() == "closed", mergedAt, nil
 }
 
+func (c GHClient) GetPRHeadSHA(ctx context.Context, owner, repo string, prNumber int) (string, error) {
+	pr, _, err := c.client.PullRequests.Get(ctx, owner, repo, prNumber)
+	if err != nil {
+		return "", fmt.Errorf("failed to query GitHub for PR HEAD SHA: %w", err)
+	}
+
+	return pr.GetHead().GetSHA(), nil
+}
+
 type CIStatus uint
 
 const (

internal/github/github_test.go

@@ -644,3 +644,35 @@ func TestGetCIStatusForChecks_ErrorListStatuses(t *testing.T) {
 	require.Error(t, err)
 	require.ErrorContains(t, err, "failed to query GitHub")
 }
+
+func TestGetPRHeadSHA(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+
+	mockedHTTPClient := mock.NewMockedHTTPClient(
+		mock.WithRequestMatch(
+			mock.GetReposPullsByOwnerByRepoByPullNumber,
+			github.PullRequest{
+				Head: &github.PullRequestBranch{
+					SHA: github.String("abcdef12345"),
+				},
+			},
+		),
+	)
+
+	ghClient := newClientFromMock(t, mockedHTTPClient)
+
+	sha, err := ghClient.GetPRHeadSHA(ctx, "owner", "repo", 1)
+	require.NoError(t, err)
+	require.Equal(t, "abcdef12345", sha)
+}
+
+func TestGetPRHeadSHA_Error(t *testing.T) {
+	t.Parallel()
+
+	ctx := context.Background()
+	ghClient := newErrorReturningClient(t)
+	_, err := ghClient.GetPRHeadSHA(ctx, "owner", "repo", 1)
+	require.Error(t, err)
+}