From 6f2dfd7eea1e4cfc004417a778ce2058c814d22a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 20 Jul 2021 19:34:32 +0200 Subject: [PATCH] Adjust conditions for switching between regular and auto merge Conditions prohibiting a regular merge: BLOCKED, BEHIND, DIRTY. Conditions triggering a regular merge even if `--auto` was set: CLEAN, HAS_HOOKS. Note that UNKNOWN status does not trigger either of the conditions. --- pkg/cmd/pr/merge/merge.go | 43 +++++++++++++++++++++++----------- pkg/cmd/pr/merge/merge_test.go | 33 ++++++++++++++++++++++++-- 2 files changed, 60 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index b9efda9fe..84fb23e06 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -156,7 +156,7 @@ func mergeRun(opts *MergeOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, - Fields: []string{"id", "number", "state", "title", "lastCommit", "mergeable", "mergeStateStatus", "headRepositoryOwner", "headRefName"}, + Fields: []string{"id", "number", "state", "title", "lastCommit", "mergeStateStatus", "headRepositoryOwner", "headRefName"}, } pr, baseRepo, err := opts.Finder.Find(findOptions) if err != nil { @@ -191,25 +191,17 @@ func mergeRun(opts *MergeOptions) error { } } - if pr.Mergeable == "CONFLICTING" { - fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d (%s) has conflicts and isn't mergeable\n", cs.Red("!"), pr.Number, pr.Title) - return cmdutil.SilentError - } - - approved := pr.MergeStateStatus == "CLEAN" || pr.MergeStateStatus == "HAS_HOOKS" || pr.MergeStateStatus == "UNSTABLE" - if !opts.AutoMergeEnable && !approved { - fmt.Fprintf( - opts.IO.ErrOut, - "%s Merging is blocked. Enable auto-merge to automatically merge it when all requirements are met with: gh pr merge %d --auto\n", - cs.FailureIcon(), pr.Number) + isPRAlreadyMerged := pr.State == "MERGED" + if blocked := blockedReason(pr.MergeStateStatus); !opts.AutoMergeEnable && !isPRAlreadyMerged && blocked != "" { + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is not mergeable: %s.\n", cs.FailureIcon(), pr.Number, blocked) + fmt.Fprintf(opts.IO.ErrOut, "To have the pull request merged after all the requirements have been met, add the `--auto` flag.\n") return cmdutil.SilentError } deleteBranch := opts.DeleteBranch crossRepoPR := pr.HeadRepositoryOwner.Login != baseRepo.RepoOwner() - autoMerge := opts.AutoMergeEnable && !approved + autoMerge := opts.AutoMergeEnable && !isImmediatelyMergeable(pr.MergeStateStatus) - isPRAlreadyMerged := pr.State == "MERGED" if !isPRAlreadyMerged { payload := mergePayload{ repo: baseRepo, @@ -461,3 +453,26 @@ func (e *userEditor) Edit(filename, startingText string) (string, error) { return surveyext.Edit(editorCommand, filename, startingText, e.io.In, e.io.Out, e.io.ErrOut, nil) } + +// blockedReason translates various MergeStateStatus GraphQL values into human-readable reason +func blockedReason(status string) string { + switch status { + case "BLOCKED": + return "the base branch policy prohibits the merge" + case "BEHIND": + return "the head branch is not up to date with the base branch" + case "DIRTY": + return "the merge commit cannot be cleanly created" + default: + return "" + } +} + +func isImmediatelyMergeable(status string) bool { + switch status { + case "CLEAN", "HAS_HOOKS": + return true + default: + return false + } +} diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 288cb203d..16aa89750 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -289,6 +289,35 @@ func TestPrMerge(t *testing.T) { } } +func TestPrMerge_blocked(t *testing.T) { + http := initFakeHTTP() + defer http.Verify(t) + + shared.RunCommandFinder( + "1", + &api.PullRequest{ + ID: "THE-ID", + Number: 1, + State: "OPEN", + Title: "The title of the PR", + MergeStateStatus: "BLOCKED", + }, + baseRepo("OWNER", "REPO", "master"), + ) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + output, err := runCommand(http, "master", true, "pr merge 1 --merge") + assert.EqualError(t, err, "SilentError") + + assert.Equal(t, "", output.String()) + assert.Equal(t, heredoc.Docf(` + X Pull request #1 is not mergeable: the base branch policy prohibits the merge. + To have the pull request merged after all the requirements have been met, add the %[1]s--auto%[1]s flag. + `, "`"), output.Stderr()) +} + func TestPrMerge_nontty(t *testing.T) { http := initFakeHTTP() defer http.Verify(t) @@ -471,7 +500,7 @@ func Test_nonDivergingPullRequest(t *testing.T) { stubCommit(pr, "COMMITSHA1") prFinder := shared.RunCommandFinder("", pr, baseRepo("OWNER", "REPO", "master")) - prFinder.ExpectFields([]string{"id", "number", "state", "title", "lastCommit", "mergeable", "mergeStateStatus", "headRepositoryOwner", "headRefName"}) + prFinder.ExpectFields([]string{"id", "number", "state", "title", "lastCommit", "mergeStateStatus", "headRepositoryOwner", "headRefName"}) http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`), @@ -510,7 +539,7 @@ func Test_divergingPullRequestWarning(t *testing.T) { stubCommit(pr, "COMMITSHA1") prFinder := shared.RunCommandFinder("", pr, baseRepo("OWNER", "REPO", "master")) - prFinder.ExpectFields([]string{"id", "number", "state", "title", "lastCommit", "mergeable", "mergeStateStatus", "headRepositoryOwner", "headRefName"}) + prFinder.ExpectFields([]string{"id", "number", "state", "title", "lastCommit", "mergeStateStatus", "headRepositoryOwner", "headRefName"}) http.Register( httpmock.GraphQL(`mutation PullRequestMerge\b`),