Add branch protection option to block merge on requested changes. (#9592)
* Add branch protection option to block merge on requested changes. * Add migration step * Fix check to correct negation * Apply suggestions from code review Language improvement. Co-Authored-By: John Olheiser <42128690+jolheiser@users.noreply.github.com> * Copyright year. Co-authored-by: John Olheiser <42128690+jolheiser@users.noreply.github.com> Co-authored-by: Lauris BH <lauris@nix.lv>
This commit is contained in:
parent
b39fab41c8
commit
ea707f5a77
10 changed files with 65 additions and 2 deletions
|
@ -44,6 +44,7 @@ type ProtectedBranch struct {
|
||||||
ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"`
|
ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"`
|
||||||
ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
|
ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
|
||||||
RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"`
|
RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"`
|
||||||
|
BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"`
|
||||||
CreatedUnix timeutil.TimeStamp `xorm:"created"`
|
CreatedUnix timeutil.TimeStamp `xorm:"created"`
|
||||||
UpdatedUnix timeutil.TimeStamp `xorm:"updated"`
|
UpdatedUnix timeutil.TimeStamp `xorm:"updated"`
|
||||||
}
|
}
|
||||||
|
@ -166,6 +167,23 @@ func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest)
|
||||||
return approvals
|
return approvals
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// MergeBlockedByRejectedReview returns true if merge is blocked by rejected reviews
|
||||||
|
func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullRequest) bool {
|
||||||
|
if !protectBranch.BlockOnRejectedReviews {
|
||||||
|
return false
|
||||||
|
}
|
||||||
|
rejectExist, err := x.Where("issue_id = ?", pr.IssueID).
|
||||||
|
And("type = ?", ReviewTypeReject).
|
||||||
|
And("official = ?", true).
|
||||||
|
Exist(new(Review))
|
||||||
|
if err != nil {
|
||||||
|
log.Error("MergeBlockedByRejectedReview: %v", err)
|
||||||
|
return true
|
||||||
|
}
|
||||||
|
|
||||||
|
return rejectExist
|
||||||
|
}
|
||||||
|
|
||||||
// GetProtectedBranchByRepoID getting protected branch by repo ID
|
// GetProtectedBranchByRepoID getting protected branch by repo ID
|
||||||
func GetProtectedBranchByRepoID(repoID int64) ([]*ProtectedBranch, error) {
|
func GetProtectedBranchByRepoID(repoID int64) ([]*ProtectedBranch, error) {
|
||||||
protectedBranches := make([]*ProtectedBranch, 0)
|
protectedBranches := make([]*ProtectedBranch, 0)
|
||||||
|
@ -340,7 +358,7 @@ func (repo *Repository) IsProtectedBranchForMerging(pr *PullRequest, branchName
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return true, err
|
return true, err
|
||||||
} else if has {
|
} else if has {
|
||||||
return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr), nil
|
return !protectedBranch.CanUserMerge(doer.ID) || !protectedBranch.HasEnoughApprovals(pr) || protectedBranch.MergeBlockedByRejectedReview(pr), nil
|
||||||
}
|
}
|
||||||
|
|
||||||
return false, nil
|
return false, nil
|
||||||
|
|
|
@ -288,6 +288,8 @@ var migrations = []Migration{
|
||||||
NewMigration("add user_id prefix to existing user avatar name", renameExistingUserAvatarName),
|
NewMigration("add user_id prefix to existing user avatar name", renameExistingUserAvatarName),
|
||||||
// v116 -> v117
|
// v116 -> v117
|
||||||
NewMigration("Extend TrackedTimes", extendTrackedTimes),
|
NewMigration("Extend TrackedTimes", extendTrackedTimes),
|
||||||
|
// v117 -> v118
|
||||||
|
NewMigration("Add block on rejected reviews branch protection", addBlockOnRejectedReviews),
|
||||||
}
|
}
|
||||||
|
|
||||||
// Migrate database to current version
|
// Migrate database to current version
|
||||||
|
|
17
models/migrations/v117.go
Normal file
17
models/migrations/v117.go
Normal file
|
@ -0,0 +1,17 @@
|
||||||
|
// Copyright 2020 The Gitea Authors. All rights reserved.
|
||||||
|
// Use of this source code is governed by a MIT-style
|
||||||
|
// license that can be found in the LICENSE file.
|
||||||
|
|
||||||
|
package migrations
|
||||||
|
|
||||||
|
import (
|
||||||
|
"xorm.io/xorm"
|
||||||
|
)
|
||||||
|
|
||||||
|
func addBlockOnRejectedReviews(x *xorm.Engine) error {
|
||||||
|
type ProtectedBranch struct {
|
||||||
|
BlockOnRejectedReviews bool `xorm:"NOT NULL DEFAULT false"`
|
||||||
|
}
|
||||||
|
|
||||||
|
return x.Sync2(new(ProtectedBranch))
|
||||||
|
}
|
|
@ -171,6 +171,7 @@ type ProtectBranchForm struct {
|
||||||
EnableApprovalsWhitelist bool
|
EnableApprovalsWhitelist bool
|
||||||
ApprovalsWhitelistUsers string
|
ApprovalsWhitelistUsers string
|
||||||
ApprovalsWhitelistTeams string
|
ApprovalsWhitelistTeams string
|
||||||
|
BlockOnRejectedReviews bool
|
||||||
}
|
}
|
||||||
|
|
||||||
// Validate validates the fields
|
// Validate validates the fields
|
||||||
|
|
|
@ -1054,6 +1054,7 @@ pulls.is_checking = "Merge conflict checking is in progress. Try again in few mo
|
||||||
pulls.required_status_check_failed = Some required checks were not successful.
|
pulls.required_status_check_failed = Some required checks were not successful.
|
||||||
pulls.required_status_check_administrator = As an administrator, you may still merge this pull request.
|
pulls.required_status_check_administrator = As an administrator, you may still merge this pull request.
|
||||||
pulls.blocked_by_approvals = "This Pull Request doesn't have enough approvals yet. %d of %d approvals granted."
|
pulls.blocked_by_approvals = "This Pull Request doesn't have enough approvals yet. %d of %d approvals granted."
|
||||||
|
pulls.blocked_by_rejection = "This Pull Request has changes requested by an official reviewer."
|
||||||
pulls.can_auto_merge_desc = This pull request can be merged automatically.
|
pulls.can_auto_merge_desc = This pull request can be merged automatically.
|
||||||
pulls.cannot_auto_merge_desc = This pull request cannot be merged automatically due to conflicts.
|
pulls.cannot_auto_merge_desc = This pull request cannot be merged automatically due to conflicts.
|
||||||
pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts.
|
pulls.cannot_auto_merge_helper = Merge manually to resolve the conflicts.
|
||||||
|
@ -1417,6 +1418,8 @@ settings.update_protect_branch_success = Branch protection for branch '%s' has b
|
||||||
settings.remove_protected_branch_success = Branch protection for branch '%s' has been disabled.
|
settings.remove_protected_branch_success = Branch protection for branch '%s' has been disabled.
|
||||||
settings.protected_branch_deletion = Disable Branch Protection
|
settings.protected_branch_deletion = Disable Branch Protection
|
||||||
settings.protected_branch_deletion_desc = Disabling branch protection allows users with write permission to push to the branch. Continue?
|
settings.protected_branch_deletion_desc = Disabling branch protection allows users with write permission to push to the branch. Continue?
|
||||||
|
settings.block_rejected_reviews = Block merge on rejected reviews
|
||||||
|
settings.block_rejected_reviews_desc = Merging will not be possible when changes are requested by official reviewers, even if there are enough approvals.
|
||||||
settings.default_branch_desc = Select a default repository branch for pull requests and code commits:
|
settings.default_branch_desc = Select a default repository branch for pull requests and code commits:
|
||||||
settings.choose_branch = Choose a branch…
|
settings.choose_branch = Choose a branch…
|
||||||
settings.no_protected_branch = There are no protected branches.
|
settings.no_protected_branch = There are no protected branches.
|
||||||
|
|
|
@ -113,6 +113,13 @@ func HookPreReceive(ctx *macaron.Context, opts private.HookOptions) {
|
||||||
})
|
})
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
if protectBranch.MergeBlockedByRejectedReview(pr) {
|
||||||
|
log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v and pr #%d has requested changes", opts.UserID, branchName, repo, pr.Index)
|
||||||
|
ctx.JSON(http.StatusForbidden, map[string]interface{}{
|
||||||
|
"err": fmt.Sprintf("protected branch %s can not be pushed to and pr #%d has requested changes", branchName, opts.ProtectedBranchID),
|
||||||
|
})
|
||||||
|
return
|
||||||
|
}
|
||||||
} else if !canPush {
|
} else if !canPush {
|
||||||
log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v", opts.UserID, branchName, repo)
|
log.Warn("Forbidden: User %d cannot push to protected branch: %s in %-v", opts.UserID, branchName, repo)
|
||||||
ctx.JSON(http.StatusForbidden, map[string]interface{}{
|
ctx.JSON(http.StatusForbidden, map[string]interface{}{
|
||||||
|
|
|
@ -962,7 +962,8 @@ func ViewIssue(ctx *context.Context) {
|
||||||
}
|
}
|
||||||
if pull.ProtectedBranch != nil {
|
if pull.ProtectedBranch != nil {
|
||||||
cnt := pull.ProtectedBranch.GetGrantedApprovalsCount(pull)
|
cnt := pull.ProtectedBranch.GetGrantedApprovalsCount(pull)
|
||||||
ctx.Data["IsBlockedByApprovals"] = pull.ProtectedBranch.RequiredApprovals > 0 && cnt < pull.ProtectedBranch.RequiredApprovals
|
ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull)
|
||||||
|
ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull)
|
||||||
ctx.Data["GrantedApprovals"] = cnt
|
ctx.Data["GrantedApprovals"] = cnt
|
||||||
}
|
}
|
||||||
ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch)
|
ctx.Data["IsPullBranchDeletable"] = canDelete && pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch)
|
||||||
|
|
|
@ -244,6 +244,7 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
|
||||||
approvalsWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.ApprovalsWhitelistTeams, ","))
|
approvalsWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.ApprovalsWhitelistTeams, ","))
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
protectBranch.BlockOnRejectedReviews = f.BlockOnRejectedReviews
|
||||||
|
|
||||||
err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{
|
err = models.UpdateProtectBranch(ctx.Repo.Repository, protectBranch, models.WhitelistOptions{
|
||||||
UserIDs: whitelistUsers,
|
UserIDs: whitelistUsers,
|
||||||
|
|
|
@ -41,6 +41,7 @@
|
||||||
{{else if .IsFilesConflicted}}grey
|
{{else if .IsFilesConflicted}}grey
|
||||||
{{else if .IsPullRequestBroken}}red
|
{{else if .IsPullRequestBroken}}red
|
||||||
{{else if .IsBlockedByApprovals}}red
|
{{else if .IsBlockedByApprovals}}red
|
||||||
|
{{else if .IsBlockedByRejection}}red
|
||||||
{{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}red
|
{{else if and .EnableStatusCheck (not .IsRequiredStatusCheckSuccess)}}red
|
||||||
{{else if .Issue.PullRequest.IsChecking}}yellow
|
{{else if .Issue.PullRequest.IsChecking}}yellow
|
||||||
{{else if .Issue.PullRequest.CanAutoMerge}}green
|
{{else if .Issue.PullRequest.CanAutoMerge}}green
|
||||||
|
@ -100,6 +101,11 @@
|
||||||
<span class="octicon octicon-x"></span>
|
<span class="octicon octicon-x"></span>
|
||||||
{{$.i18n.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}}
|
{{$.i18n.Tr "repo.pulls.blocked_by_approvals" .GrantedApprovals .Issue.PullRequest.ProtectedBranch.RequiredApprovals}}
|
||||||
</div>
|
</div>
|
||||||
|
{{else if .IsBlockedByRejection}}
|
||||||
|
<div class="item text red">
|
||||||
|
<span class="octicon octicon-x"></span>
|
||||||
|
{{$.i18n.Tr "repo.pulls.blocked_by_rejection"}}
|
||||||
|
</div>
|
||||||
{{else if .Issue.PullRequest.IsChecking}}
|
{{else if .Issue.PullRequest.IsChecking}}
|
||||||
<div class="item text yellow">
|
<div class="item text yellow">
|
||||||
<span class="octicon octicon-sync"></span>
|
<span class="octicon octicon-sync"></span>
|
||||||
|
|
|
@ -204,6 +204,13 @@
|
||||||
</div>
|
</div>
|
||||||
{{end}}
|
{{end}}
|
||||||
</div>
|
</div>
|
||||||
|
<div class="field">
|
||||||
|
<div class="ui checkbox">
|
||||||
|
<input name="block_on_rejected_reviews" type="checkbox" {{if .Branch.BlockOnRejectedReviews}}checked{{end}}>
|
||||||
|
<label for="block_on_rejected_reviews">{{.i18n.Tr "repo.settings.block_rejected_reviews"}}</label>
|
||||||
|
<p class="help">{{.i18n.Tr "repo.settings.block_rejected_reviews_desc"}}</p>
|
||||||
|
</div>
|
||||||
|
</div>
|
||||||
</div>
|
</div>
|
||||||
|
|
||||||
<div class="ui divider"></div>
|
<div class="ui divider"></div>
|
||||||
|
|
Reference in a new issue