Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add force_merge to merge request and fix checking mergable (#23010) #23032

Merged
merged 5 commits into from
Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 11 additions & 4 deletions routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -767,11 +767,18 @@ func MergePullRequest(ctx *context.APIContext) {
}
}

manuallMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
force := form.ForceMerge != nil && *form.ForceMerge
manuallyMerged := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged

mergeCheckType := pull_service.MergeCheckTypeGeneral
if form.MergeWhenChecksSucceed {
mergeCheckType = pull_service.MergeCheckTypeAuto
}
if manuallyMerged {
mergeCheckType = pull_service.MergeCheckTypeManually
}

// start with merging by checking
if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manuallMerge, force); err != nil {
if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge); err != nil {
if errors.Is(err, pull_service.ErrIsClosed) {
ctx.NotFound()
} else if errors.Is(err, pull_service.ErrUserNotAllowedToMerge) {
Expand All @@ -793,7 +800,7 @@ func MergePullRequest(ctx *context.APIContext) {
}

// handle manually-merged mark
if manuallMerge {
if manuallyMerged {
if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
if models.IsErrInvalidMergeStyle(err) {
ctx.Error(http.StatusMethodNotAllowed, "Invalid merge style", fmt.Errorf("%s is not allowed an allowed merge style for this repository", repo_model.MergeStyle(form.Do)))
Expand Down
16 changes: 12 additions & 4 deletions routers/web/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -912,11 +912,19 @@ func MergePullRequest(ctx *context.Context) {
pr := issue.PullRequest
pr.Issue = issue
pr.Issue.Repo = ctx.Repo.Repository
manualMerge := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged
forceMerge := form.ForceMerge != nil && *form.ForceMerge

manuallyMerged := repo_model.MergeStyle(form.Do) == repo_model.MergeStyleManuallyMerged

mergeCheckType := pull_service.MergeCheckTypeGeneral
if form.MergeWhenChecksSucceed {
mergeCheckType = pull_service.MergeCheckTypeAuto
}
if manuallyMerged {
mergeCheckType = pull_service.MergeCheckTypeManually
}

// start with merging by checking
if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, manualMerge, forceMerge); err != nil {
if err := pull_service.CheckPullMergable(ctx, ctx.Doer, &ctx.Repo.Permission, pr, mergeCheckType, form.ForceMerge); err != nil {
switch {
case errors.Is(err, pull_service.ErrIsClosed):
if issue.IsPull {
Expand Down Expand Up @@ -948,7 +956,7 @@ func MergePullRequest(ctx *context.Context) {
}

// handle manually-merged mark
if manualMerge {
if manuallyMerged {
if err := pull_service.MergedManually(pr, ctx.Doer, ctx.Repo.GitRepo, form.MergeCommitID); err != nil {
switch {

Expand Down
2 changes: 1 addition & 1 deletion services/automerge/automerge.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,7 @@ func handlePull(pullID int64, sha string) {
return
}

if err := pull_service.CheckPullMergable(ctx, doer, &perm, pr, false, false); err != nil {
if err := pull_service.CheckPullMergable(ctx, doer, &perm, pr, pull_service.MergeCheckTypeGeneral, false); err != nil {
if errors.Is(pull_service.ErrUserNotAllowedToMerge, err) {
log.Info("%-v was scheduled to automerge by an unauthorized user", pr)
return
Expand Down
2 changes: 1 addition & 1 deletion services/forms/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -597,7 +597,7 @@ type MergePullRequestForm struct {
MergeMessageField string
MergeCommitID string // only used for manually-merged
HeadCommitID string `json:"head_commit_id,omitempty"`
ForceMerge *bool `json:"force_merge,omitempty"`
ForceMerge bool `json:"force_merge,omitempty"`
MergeWhenChecksSucceed bool `json:"merge_when_checks_succeed,omitempty"`
DeleteBranchAfterMerge bool `json:"delete_branch_after_merge,omitempty"`
}
Expand Down
37 changes: 28 additions & 9 deletions services/pull/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,16 @@ func AddToTaskQueue(pr *issues_model.PullRequest) {
}
}

type MergeCheckType int

const (
MergeCheckTypeGeneral MergeCheckType = iota // general merge checks for "merge", "rebase", "squash", etc
MergeCheckTypeManually // Manually Merged button (mark a PR as merged manually)
MergeCheckTypeAuto // Auto Merge (Scheduled Merge) After Checks Succeed
)

// CheckPullMergable check if the pull mergable based on all conditions (branch protection, merge options, ...)
func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, manuallMerge, force bool) error {
func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *access_model.Permission, pr *issues_model.PullRequest, mergeCheckType MergeCheckType, adminSkipProtectionCheck bool) error {
return db.WithTx(func(ctx context.Context) error {
if pr.HasMerged {
return ErrHasMerged
Expand All @@ -80,8 +88,8 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
return ErrUserNotAllowedToMerge
}

if manuallMerge {
// don't check rules to "auto merge", doer is going to mark this pull as merged manually
if mergeCheckType == MergeCheckTypeManually {
// if doer is doing "manually merge" (mark as merged manually), do not check anything
return nil
}

Expand All @@ -103,14 +111,25 @@ func CheckPullMergable(stdCtx context.Context, doer *user_model.User, perm *acce
return err
}

if !force {
return err
// Now the branch protection check failed, check whether the failure could be skipped (skip by setting err = nil)

// * when doing Auto Merge (Scheduled Merge After Checks Succeed), skip the branch protection check
if mergeCheckType == MergeCheckTypeAuto {
err = nil
}

// * if the doer is admin, they could skip the branch protection check
if adminSkipProtectionCheck {
if isRepoAdmin, errCheckAdmin := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); errCheckAdmin != nil {
log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, errCheckAdmin)
return errCheckAdmin
} else if isRepoAdmin {
err = nil // repo admin can skip the check, so clear the error
}
}

if isRepoAdmin, err2 := access_model.IsUserRepoAdmin(ctx, pr.BaseRepo, doer); err2 != nil {
log.Error("Unable to check if %-v is a repo admin in %-v: %v", doer, pr.BaseRepo, err2)
return err2
} else if !isRepoAdmin {
// If there is still a branch protection check error, return it
if err != nil {
return err
}
}
Expand Down
7 changes: 6 additions & 1 deletion web_src/js/components/PullRequestMergeForm.vue
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
<input type="hidden" name="_csrf" :value="csrfToken">
<input type="hidden" name="head_commit_id" v-model="mergeForm.pullHeadCommitID">
<input type="hidden" name="merge_when_checks_succeed" v-model="autoMergeWhenSucceed">
<input type="hidden" name="force_merge" v-model="forceMerge">

<template v-if="!mergeStyleDetail.hideMergeMessageTexts">
<div class="field">
Expand Down Expand Up @@ -123,6 +124,7 @@ export default {
textDoMerge: '',
mergeTitleFieldText: '',
mergeMessageFieldText: '',
hideAutoMerge: false,
},
mergeStyleAllowedCount: 0,

Expand All @@ -134,7 +136,10 @@ export default {
mergeButtonStyleClass() {
if (this.mergeForm.allOverridableChecksOk) return 'green';
return this.autoMergeWhenSucceed ? 'blue' : 'red';
}
},
forceMerge() {
return this.mergeForm.canMergeNow && !this.mergeForm.allOverridableChecksOk;
},
},

watch: {
Expand Down