From 94d159da1a25dc091282aa94be0459a451b73f1c Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Wed, 28 Jun 2023 16:52:08 +0100 Subject: [PATCH] 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. --- cmd/wait-for-github/pr.go | 29 +++++++++++++-- cmd/wait-for-github/pr_test.go | 66 ++++++++++++++++++++++++---------- internal/github/github.go | 23 +++++++++++- internal/github/github_test.go | 32 +++++++++++++++++ 4 files changed, 128 insertions(+), 22 deletions(-) diff --git a/cmd/wait-for-github/pr.go b/cmd/wait-for-github/pr.go index 2111318..5e05609 100644 --- a/cmd/wait-for-github/pr.go +++ b/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, diff --git a/cmd/wait-for-github/pr_test.go b/cmd/wait-for-github/pr_test.go index 3ee6fff..f9c43a2 100644 --- a/cmd/wait-for-github/pr_test.go +++ b/cmd/wait-for-github/pr_test.go @@ -24,20 +24,34 @@ 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) { @@ -45,13 +59,12 @@ func TestPRCheck(t *testing.T) { tests := []struct { name string - fakeClient FakeGithubClientPRCheck - err error + fakeClient fakeGithubClientPRCheck expectedExitCode *int }{ { name: "PR is merged", - fakeClient: FakeGithubClientPRCheck{ + fakeClient: fakeGithubClientPRCheck{ MergedCommit: "abc123", MergedAt: 1234567890, }, @@ -59,25 +72,42 @@ func TestPRCheck(t *testing.T) { }, { 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, }, diff --git a/internal/github/github.go b/internal/github/github.go index e837b88..02ca5b1 100644 --- a/internal/github/github.go +++ b/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 ( diff --git a/internal/github/github_test.go b/internal/github/github_test.go index 023375b..86c46d2 100644 --- a/internal/github/github_test.go +++ b/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) +}