From baa18c164de838c9f6ffe9f04363fd5608e6b229 Mon Sep 17 00:00:00 2001 From: rsteube Date: Mon, 2 Aug 2021 12:10:13 +0200 Subject: [PATCH 1/2] pr merge: added `--admin` flag --- pkg/cmd/pr/merge/merge.go | 24 +++++++++++++++++------- pkg/cmd/pr/merge/merge_test.go | 3 ++- 2 files changed, 19 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 73145faba..4f66f0426 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -40,6 +40,7 @@ type MergeOptions struct { BodySet bool Editor editor + UseAdmin bool IsDeleteBranchIndicated bool CanDeleteLocalBranch bool InteractiveMode bool @@ -140,6 +141,7 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm }, } + cmd.Flags().BoolVar(&opts.UseAdmin, "admin", false, "Use implicit administrator privileges") cmd.Flags().BoolVarP(&opts.DeleteBranch, "delete-branch", "d", false, "Delete the local and remote branch after merge") cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Body `text` for the merge commit") cmd.Flags().StringVarP(&bodyFile, "body-file", "F", "", "Read body text from `file`") @@ -192,9 +194,12 @@ func mergeRun(opts *MergeOptions) error { } 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) + if blocked, reason := blockedReason(pr.MergeStateStatus, opts.UseAdmin); !opts.AutoMergeEnable && !isPRAlreadyMerged && blocked { + fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is not mergeable: %s.\n", cs.FailureIcon(), pr.Number, reason) fmt.Fprintf(opts.IO.ErrOut, "To have the pull request merged after all the requirements have been met, add the `--auto` flag.\n") + if !opts.UseAdmin && allowsAdminOverride(pr.MergeStateStatus) { + fmt.Fprintf(opts.IO.ErrOut, "To use administrator privileges to merge the pull request, add the `--admin` flag.\n") + } return cmdutil.SilentError } @@ -455,19 +460,24 @@ func (e *userEditor) Edit(filename, startingText string) (string, error) { } // blockedReason translates various MergeStateStatus GraphQL values into human-readable reason -func blockedReason(status string) string { +func blockedReason(status string, admin bool) (bool, string) { switch status { case "BLOCKED": - return "the base branch policy prohibits the merge" + return !admin, "the base branch policy prohibits the merge" case "BEHIND": - return "the head branch is not up to date with the base branch" + return !admin, "the head branch is not up to date with the base branch" case "DIRTY": - return "the merge commit cannot be cleanly created" + return true, "the merge commit cannot be cleanly created" default: - return "" + return false, "" } } +func allowsAdminOverride(status string) bool { + blocked, _ := blockedReason(status, true) + return !blocked +} + func isImmediatelyMergeable(status string) bool { switch status { case "CLEAN", "HAS_HOOKS", "UNSTABLE": diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 16aa89750..e5fb65476 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -315,7 +315,8 @@ func TestPrMerge_blocked(t *testing.T) { 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()) + To use administrator privileges to merge the pull request, add the %[1]s--admin%[1]s flag. + `, "`"), output.Stderr()) } func TestPrMerge_nontty(t *testing.T) { From 5d1d967c439cc1a8ffa6bbb6e0b1e3349ea646a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 3 Aug 2021 15:49:55 +0200 Subject: [PATCH 2/2] :nail_care: Clean up pr merge admin logic --- pkg/cmd/pr/merge/merge.go | 40 +++++++++++++++++++++++++--------- pkg/cmd/pr/merge/merge_test.go | 2 +- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 4f66f0426..3badf352c 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -110,6 +110,15 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm bodyProvided := cmd.Flags().Changed("body") bodyFileProvided := bodyFile != "" + if err := cmdutil.MutuallyExclusive( + "specify only one of `--auto`, `--disable-auto`, or `--admin`", + opts.AutoMergeEnable, + opts.AutoMergeDisable, + opts.UseAdmin, + ); err != nil { + return err + } + if err := cmdutil.MutuallyExclusive( "specify only one of `--body` or `--body-file`", bodyProvided, @@ -141,7 +150,7 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm }, } - cmd.Flags().BoolVar(&opts.UseAdmin, "admin", false, "Use implicit administrator privileges") + cmd.Flags().BoolVar(&opts.UseAdmin, "admin", false, "Use administrator privileges to merge a pull request that does not meet requirements") cmd.Flags().BoolVarP(&opts.DeleteBranch, "delete-branch", "d", false, "Delete the local and remote branch after merge") cmd.Flags().StringVarP(&opts.Body, "body", "b", "", "Body `text` for the merge commit") cmd.Flags().StringVarP(&bodyFile, "body-file", "F", "", "Read body text from `file`") @@ -194,11 +203,12 @@ func mergeRun(opts *MergeOptions) error { } isPRAlreadyMerged := pr.State == "MERGED" - if blocked, reason := blockedReason(pr.MergeStateStatus, opts.UseAdmin); !opts.AutoMergeEnable && !isPRAlreadyMerged && blocked { + if reason := blockedReason(pr.MergeStateStatus, opts.UseAdmin); !opts.AutoMergeEnable && !isPRAlreadyMerged && reason != "" { fmt.Fprintf(opts.IO.ErrOut, "%s Pull request #%d is not mergeable: %s.\n", cs.FailureIcon(), pr.Number, reason) fmt.Fprintf(opts.IO.ErrOut, "To have the pull request merged after all the requirements have been met, add the `--auto` flag.\n") if !opts.UseAdmin && allowsAdminOverride(pr.MergeStateStatus) { - fmt.Fprintf(opts.IO.ErrOut, "To use administrator privileges to merge the pull request, add the `--admin` flag.\n") + // TODO: show this flag only to repo admins + fmt.Fprintf(opts.IO.ErrOut, "To use administrator privileges to immediately merge the pull request, add the `--admin` flag.\n") } return cmdutil.SilentError } @@ -460,22 +470,32 @@ func (e *userEditor) Edit(filename, startingText string) (string, error) { } // blockedReason translates various MergeStateStatus GraphQL values into human-readable reason -func blockedReason(status string, admin bool) (bool, string) { +func blockedReason(status string, useAdmin bool) string { switch status { case "BLOCKED": - return !admin, "the base branch policy prohibits the merge" + if useAdmin { + return "" + } + return "the base branch policy prohibits the merge" case "BEHIND": - return !admin, "the head branch is not up to date with the base branch" + if useAdmin { + return "" + } + return "the head branch is not up to date with the base branch" case "DIRTY": - return true, "the merge commit cannot be cleanly created" + return "the merge commit cannot be cleanly created" default: - return false, "" + return "" } } func allowsAdminOverride(status string) bool { - blocked, _ := blockedReason(status, true) - return !blocked + switch status { + case "BLOCKED", "BEHIND": + return true + default: + return false + } } func isImmediatelyMergeable(status string) bool { diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index e5fb65476..cd0e61b68 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -315,7 +315,7 @@ func TestPrMerge_blocked(t *testing.T) { 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. - To use administrator privileges to merge the pull request, add the %[1]s--admin%[1]s flag. + To use administrator privileges to immediately merge the pull request, add the %[1]s--admin%[1]s flag. `, "`"), output.Stderr()) }