diff --git a/cmd/wait-for-github/pr.go b/cmd/wait-for-github/pr.go index fe367d27..bd311497 100644 --- a/cmd/wait-for-github/pr.go +++ b/cmd/wait-for-github/pr.go @@ -157,6 +157,7 @@ type checkMergedAndOverallCI interface { github.CheckPRMerged github.CheckPRMergeableState github.GetPRHeadSHA + github.CheckPRMergeable github.CheckOverallCIStatus github.RerunFailedWorkflows github.MergePR @@ -251,6 +252,15 @@ func (pr *prCheck) Check(ctx context.Context) error { } if pr.autoMerge && status == github.CIStatusPassed { + mergeable, mergeableState, err := pr.githubClient.IsPRMergeable(ctx, pr.owner, pr.repo, pr.pr) + if err != nil { + return err + } + if !mergeable { + pr.logger.InfoContext(ctx, "CI passed but PR is not mergeable yet", "mergeable_state", mergeableState) + return nil + } + pr.logger.InfoContext(ctx, "CI passed and auto-merge is enabled, merging PR", "method", pr.autoMergeMethod) // Pass sha to prevent merging a different commit than the one CI ran on. // Merge failures (conflicts, branch protection) are retried on each poll; diff --git a/cmd/wait-for-github/pr_test.go b/cmd/wait-for-github/pr_test.go index 14934817..e5933d8d 100644 --- a/cmd/wait-for-github/pr_test.go +++ b/cmd/wait-for-github/pr_test.go @@ -38,6 +38,7 @@ type fakeGithubClientPRCheck struct { isPRMergedError error getPRHeadSHAError error + checkPRMergeableError error getCIStatusError error rerunFailedWorkflowsError error mergePRError error @@ -46,6 +47,8 @@ type fakeGithubClientPRCheck struct { getMergeableStateError error CIStatus github.CIStatus + Mergeable bool + MergeableCalledCount int RerunCount int HasRunsInProgress bool RerunCalledCount int @@ -69,6 +72,11 @@ func (fg *fakeGithubClientPRCheck) GetPRHeadSHA(ctx context.Context, owner, repo return fg.MergedCommit, fg.getPRHeadSHAError } +func (fg *fakeGithubClientPRCheck) IsPRMergeable(ctx context.Context, owner, repo string, pr int) (bool, string, error) { + fg.MergeableCalledCount++ + return fg.Mergeable, fg.MergeableState, fg.checkPRMergeableError +} + func (fg *fakeGithubClientPRCheck) GetCIStatus(ctx context.Context, owner, repo string, commitHash string, excludes []string) (github.CIStatus, error) { return fg.CIStatus, fg.getCIStatusError } @@ -195,8 +203,9 @@ func TestPRCheck(t *testing.T) { { name: "CI passed with auto-merge squash, merge succeeds", fakeClient: fakeGithubClientPRCheck{ - HeadSHA: "abc123", - CIStatus: github.CIStatusPassed, + HeadSHA: "abc123", + CIStatus: github.CIStatusPassed, + Mergeable: true, }, autoMerge: true, autoMergeMethod: "squash", @@ -208,8 +217,9 @@ func TestPRCheck(t *testing.T) { { name: "CI passed with auto-merge rebase, merge succeeds", fakeClient: fakeGithubClientPRCheck{ - HeadSHA: "abc123", - CIStatus: github.CIStatusPassed, + HeadSHA: "abc123", + CIStatus: github.CIStatusPassed, + Mergeable: true, }, autoMerge: true, autoMergeMethod: "rebase", @@ -220,8 +230,9 @@ func TestPRCheck(t *testing.T) { { name: "CI passed with auto-merge, default method", fakeClient: fakeGithubClientPRCheck{ - HeadSHA: "abc123", - CIStatus: github.CIStatusPassed, + HeadSHA: "abc123", + CIStatus: github.CIStatusPassed, + Mergeable: true, }, autoMerge: true, autoMergeMethod: "merge", @@ -234,6 +245,7 @@ func TestPRCheck(t *testing.T) { fakeClient: fakeGithubClientPRCheck{ HeadSHA: "abc123", CIStatus: github.CIStatusPassed, + Mergeable: true, mergePRError: fmt.Errorf("merge failed"), }, autoMerge: true, @@ -243,6 +255,26 @@ func TestPRCheck(t *testing.T) { expectMergeMethod: "squash", // No exit code - continues polling and will retry }, + { + name: "CI passed with auto-merge, PR is blocked", + fakeClient: fakeGithubClientPRCheck{ + HeadSHA: "abc123", + CIStatus: github.CIStatusPassed, + MergeableState: "blocked", + }, + autoMerge: true, + autoMergeMethod: "merge", + }, + { + name: "CI passed with auto-merge, mergeability check fails", + fakeClient: fakeGithubClientPRCheck{ + HeadSHA: "abc123", + CIStatus: github.CIStatusPassed, + checkPRMergeableError: fmt.Errorf("mergeability check failed"), + }, + autoMerge: true, + autoMergeMethod: "merge", + }, { name: "CI pending with auto-merge, no merge attempt", fakeClient: fakeGithubClientPRCheck{ diff --git a/internal/github/github.go b/internal/github/github.go index 467ecf6d..010a6af5 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -60,6 +60,10 @@ type GetPRHeadSHA interface { GetPRHeadSHA(ctx context.Context, owner, repo string, pr int) (string, error) } +type CheckPRMergeable interface { + IsPRMergeable(ctx context.Context, owner, repo string, pr int) (bool, string, error) +} + type CheckOverallCIStatus interface { GetCIStatus(ctx context.Context, owner, repo string, commitHash string, excludes []string) (CIStatus, error) } @@ -459,6 +463,20 @@ func (c GHClient) GetPRHeadSHA(ctx context.Context, owner, repo string, prNumber return pr.GetHead().GetSHA(), nil } +func (c GHClient) IsPRMergeable(ctx context.Context, owner, repo string, prNumber int) (bool, string, error) { + pr, resp, err := c.client.PullRequests.Get(ctx, owner, repo, prNumber) + if err != nil { + return false, "", fmt.Errorf("failed to query GitHub for PR mergeability: %w", err) + } + + if respErr := c.handleResponseError(resp, "GetPullRequest", owner, repo); respErr != nil { + return false, "", respErr + } + + mergeableState := pr.GetMergeableState() + return pr.GetMergeable() && mergeableState == "clean", mergeableState, nil +} + func (c GHClient) MergePR(ctx context.Context, owner, repo string, prNumber int, sha, mergeMethod string) error { result, resp, err := c.client.PullRequests.Merge(ctx, owner, repo, prNumber, "", &github.PullRequestOptions{ SHA: sha, diff --git a/internal/github/github_test.go b/internal/github/github_test.go index dc49fc5d..8cf6e412 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -343,6 +343,63 @@ func TestIsPRMergedOrClosed_Error(t *testing.T) { require.Error(t, err) } +func TestIsPRMergeable(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + githubMergeable bool + mergeableState string + expectedMergeable bool + }{ + { + name: "clean", + githubMergeable: true, + mergeableState: "clean", + expectedMergeable: true, + }, + { + name: "blocked by branch protection", + githubMergeable: true, + mergeableState: "blocked", + }, + { + name: "merge conflict", + githubMergeable: false, + mergeableState: "dirty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mockedHTTPClient := mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposPullsByOwnerByRepoByPullNumber, + github.PullRequest{ + Mergeable: &tt.githubMergeable, + MergeableState: &tt.mergeableState, + }, + ), + ) + + ghClient := newClientFromMock(t, mockedHTTPClient, "") + mergeable, state, err := ghClient.IsPRMergeable(context.Background(), "owner", "repo", 1) + + require.NoError(t, err) + require.Equal(t, tt.expectedMergeable, mergeable) + require.Equal(t, tt.mergeableState, state) + }) + } +} + +func TestIsPRMergeable_Error(t *testing.T) { + t.Parallel() + + ghClient := newErrorReturningClient(t) + _, _, err := ghClient.IsPRMergeable(context.Background(), "owner", "repo", 1) + require.Error(t, err) +} + func TestGetCIStatus(t *testing.T) { tests := []struct { name string