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

Refuse merge until all required status checks success #7481

Merged
merged 19 commits into from
Sep 18, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ require (
github.com/nfnt/resize v0.0.0-20160724205520-891127d8d1b5
github.com/oliamb/cutter v0.2.2
github.com/philhofer/fwd v1.0.0 // indirect
github.com/pkg/errors v0.8.1
github.com/pquerna/otp v0.0.0-20160912161815-54653902c20e
github.com/prometheus/client_golang v1.1.0
github.com/prometheus/procfs v0.0.4 // indirect
Expand Down
2 changes: 2 additions & 0 deletions models/branches.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ type ProtectedBranch struct {
EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"`
MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"`
MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"`
StatusCheckContexts []string `xorm:"JSON TEXT"`
ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"`
ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"`
Expand Down
22 changes: 22 additions & 0 deletions models/commit_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"crypto/sha1"
"fmt"
"strings"
"time"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
Expand Down Expand Up @@ -205,6 +206,27 @@ func GetLatestCommitStatus(repo *Repository, sha string, page int) ([]*CommitSta
return statuses, x.In("id", ids).Find(&statuses)
}

// FindRepoRecentCommitStatusContexts returns repository's recent commit status contexts
func FindRepoRecentCommitStatusContexts(repoID int64, before time.Duration) ([]string, error) {
start := timeutil.TimeStampNow().AddDuration(-before)
ids := make([]int64, 0, 10)
if err := x.Table("commit_status").
Where("repo_id = ?", repoID).
And("updated_unix >= ?", start).
Select("max( id ) as id").
GroupBy("context_hash").OrderBy("max( id ) desc").
Find(&ids); err != nil {
return nil, err
}

var contexts = make([]string, 0, len(ids))
if len(ids) == 0 {
return contexts, nil
}
return contexts, x.Select("context").Table("commit_status").In("id", ids).Find(&contexts)

}

// NewCommitStatusOptions holds options for creating a CommitStatus
type NewCommitStatusOptions struct {
Repo *Repository
Expand Down
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,8 @@ var migrations = []Migration{
NewMigration("remove orphaned repository index statuses", removeLingeringIndexStatus),
// v93 -> v94
NewMigration("add email notification enabled preference to user", addEmailNotificationEnabledToUser),
// v94 -> v95
NewMigration("add enable_status_check, status_check_contexts to protected_branch", addStatusCheckColumnsForProtectedBranches),
}

// Migrate database to current version
Expand Down
24 changes: 24 additions & 0 deletions models/migrations/v94.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// Copyright 2019 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 "github.com/go-xorm/xorm"

func addStatusCheckColumnsForProtectedBranches(x *xorm.Engine) error {
type ProtectedBranch struct {
EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"`
StatusCheckContexts []string `xorm:"JSON TEXT"`
}

if err := x.Sync2(new(ProtectedBranch)); err != nil {
return err
}

_, err := x.Cols("enable_status_check", "status_check_contexts").Update(&ProtectedBranch{
EnableStatusCheck: false,
StatusCheckContexts: []string{},
})
return err
}
14 changes: 14 additions & 0 deletions models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,20 @@ func (pr *PullRequest) LoadAttributes() error {
return pr.loadAttributes(x)
}

// LoadBaseRepo loads pull request base repository from database
func (pr *PullRequest) LoadBaseRepo() error {
if pr.BaseRepo == nil {
var repo Repository
if has, err := x.ID(pr.BaseRepoID).Get(&repo); err != nil {
return err
} else if !has {
return ErrRepoNotExist{ID: pr.BaseRepoID}
}
pr.BaseRepo = &repo
}
return nil
}

// LoadIssue loads issue information from database
func (pr *PullRequest) LoadIssue() (err error) {
return pr.loadIssue(x)
Expand Down
2 changes: 2 additions & 0 deletions modules/auth/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ type ProtectBranchForm struct {
EnableMergeWhitelist bool
MergeWhitelistUsers string
MergeWhitelistTeams string
EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"`
StatusCheckContexts []string
RequiredApprovals int64
ApprovalsWhitelistUsers string
ApprovalsWhitelistTeams string
Expand Down
70 changes: 70 additions & 0 deletions modules/pull/commit_status.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
// Copyright 2019 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 pull

import (
"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/git"

"github.com/pkg/errors"
)

// IsCommitStatusContextSuccess returns true if all required status check contexts succeed.
func IsCommitStatusContextSuccess(commitStatuses []*models.CommitStatus, requiredContexts []string) bool {
for _, ctx := range requiredContexts {
var found bool
for _, commitStatus := range commitStatuses {
if commitStatus.Context == ctx {
if commitStatus.State != models.CommitStatusSuccess {
return false
}

found = true
break
}
}
if !found {
return false
}
}
return true
}

// IsPullCommitStatusPass returns if all required status checks PASS
func IsPullCommitStatusPass(pr *models.PullRequest) (bool, error) {
if err := pr.LoadProtectedBranch(); err != nil {
return false, errors.Wrap(err, "GetLatestCommitStatus")
}
if pr.ProtectedBranch == nil || !pr.ProtectedBranch.EnableStatusCheck {
return true, nil
}

// check if all required status checks are successful
headGitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath())
if err != nil {
return false, errors.Wrap(err, "OpenRepository")
}

if !headGitRepo.IsBranchExist(pr.HeadBranch) {
return false, errors.New("Head branch does not exist, can not merge")
}

sha, err := headGitRepo.GetBranchCommitID(pr.HeadBranch)
if err != nil {
return false, errors.Wrap(err, "GetBranchCommitID")
}

if err := pr.LoadBaseRepo(); err != nil {
return false, errors.Wrap(err, "LoadBaseRepo")
}

commitStatuses, err := models.GetLatestCommitStatus(pr.BaseRepo, sha, 0)
if err != nil {
return false, errors.Wrap(err, "GetLatestCommitStatus")
}

return IsCommitStatusContextSuccess(commitStatuses, pr.ProtectedBranch.StatusCheckContexts), nil
}
6 changes: 6 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -981,13 +981,16 @@ pulls.cannot_merge_work_in_progress = This pull request is marked as a work in p
pulls.data_broken = This pull request is broken due to missing fork information.
pulls.files_conflicted = This pull request has changes conflicting with the target branch.
pulls.is_checking = "Merge conflict checking is in progress. Try again in few moments."
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.blocked_by_approvals = "This Pull Request doesn't have enough approvals yet. %d of %d approvals granted."
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_helper = Merge manually to resolve the conflicts.
pulls.no_merge_desc = This pull request cannot be merged because all repository merge options are disabled.
pulls.no_merge_helper = Enable merge options in the repository settings or merge the pull request manually.
pulls.no_merge_wip = This pull request can not be merged because it is marked as being a work in progress.
pulls.no_merge_status_check = This pull request cannot be merged because not all required status checkes are successful.
lunny marked this conversation as resolved.
Show resolved Hide resolved
pulls.merge_pull_request = Merge Pull Request
pulls.rebase_merge_pull_request = Rebase and Merge
pulls.rebase_merge_commit_pull_request = Rebase and Merge (--no-ff)
Expand Down Expand Up @@ -1311,6 +1314,9 @@ settings.protect_merge_whitelist_committers = Enable Merge Whitelist
settings.protect_merge_whitelist_committers_desc = Allow only whitelisted users or teams to merge pull requests into this branch.
settings.protect_merge_whitelist_users = Whitelisted users for merging:
settings.protect_merge_whitelist_teams = Whitelisted teams for merging:
settings.protect_check_status_contexts = Enable Status Check
settings.protect_check_status_contexts_desc = Require status checks to pass before merging Choose which status checks must pass before branches can be merged into a branch that matches this rule. When enabled, commits must first be pushed to another branch, then merged or pushed directly to a branch that matches this rule after status checks have passed.
lafriks marked this conversation as resolved.
Show resolved Hide resolved
lunny marked this conversation as resolved.
Show resolved Hide resolved
settings.protect_check_status_contexts_list = Status checks found in the last week for this repository
settings.protect_required_approvals = Required approvals:
settings.protect_required_approvals_desc = Allow only to merge pull request with enough positive reviews of whitelisted users or teams.
settings.protect_approvals_whitelist_users = Whitelisted reviewers:
Expand Down
12 changes: 12 additions & 0 deletions routers/api/v1/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification"
"code.gitea.io/gitea/modules/pull"
pull_service "code.gitea.io/gitea/modules/pull"
api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/timeutil"
milestone_service "code.gitea.io/gitea/services/milestone"
Expand Down Expand Up @@ -571,6 +572,17 @@ func MergePullRequest(ctx *context.APIContext, form auth.MergePullRequestForm) {
return
}

isPass, err := pull_service.IsPullCommitStatusPass(pr)
if err != nil {
ctx.Error(500, "IsPullCommitStatusPass", err)
return
}

if !isPass && !ctx.IsUserRepoAdmin() {
ctx.Status(405)
return
}

if len(form.Do) == 0 {
form.Do = string(models.MergeStyleMerge)
}
Expand Down
30 changes: 30 additions & 0 deletions routers/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification"
"code.gitea.io/gitea/modules/pull"
pull_service "code.gitea.io/gitea/modules/pull"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/services/gitdiff"
Expand Down Expand Up @@ -322,6 +323,12 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare

setMergeTarget(ctx, pull)

if err = pull.LoadProtectedBranch(); err != nil {
ctx.ServerError("GetLatestCommitStatus", err)
return nil
}
ctx.Data["EnableStatusCheck"] = pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck

var headGitRepo *git.Repository
var headBranchExist bool
// HeadRepo may be missing
Expand Down Expand Up @@ -350,6 +357,18 @@ func PrepareViewPullInfo(ctx *context.Context, issue *models.Issue) *git.Compare
ctx.Data["LatestCommitStatuses"] = commitStatuses
ctx.Data["LatestCommitStatus"] = models.CalcCommitStatus(commitStatuses)
}

if pull.ProtectedBranch != nil && pull.ProtectedBranch.EnableStatusCheck {
ctx.Data["is_context_required"] = func(context string) bool {
for _, c := range pull.ProtectedBranch.StatusCheckContexts {
if c == context {
return true
}
}
return false
}
ctx.Data["IsRequiredStatusCheckSuccess"] = pull_service.IsCommitStatusContextSuccess(commitStatuses, pull.ProtectedBranch.StatusCheckContexts)
}
}
}

Expand Down Expand Up @@ -608,6 +627,17 @@ func MergePullRequest(ctx *context.Context, form auth.MergePullRequestForm) {
return
}

isPass, err := pull_service.IsPullCommitStatusPass(pr)
if err != nil {
ctx.ServerError("IsPullCommitStatusPass", err)
return
}
if !isPass && !ctx.IsUserRepoAdmin() {
ctx.Flash.Error(ctx.Tr("repo.pulls.no_merge_status_check"))
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
return
}

if ctx.HasError() {
ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index))
Expand Down
28 changes: 28 additions & 0 deletions routers/repo/setting_protected_branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package repo
import (
"fmt"
"strings"
"time"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/auth"
Expand Down Expand Up @@ -125,6 +126,29 @@ func SettingsProtectedBranch(c *context.Context) {
c.Data["whitelist_users"] = strings.Join(base.Int64sToStrings(protectBranch.WhitelistUserIDs), ",")
c.Data["merge_whitelist_users"] = strings.Join(base.Int64sToStrings(protectBranch.MergeWhitelistUserIDs), ",")
c.Data["approvals_whitelist_users"] = strings.Join(base.Int64sToStrings(protectBranch.ApprovalsWhitelistUserIDs), ",")
contexts, _ := models.FindRepoRecentCommitStatusContexts(c.Repo.Repository.ID, 7*24*time.Hour) // Find last week status check contexts
for _, context := range protectBranch.StatusCheckContexts {
var found bool
for _, ctx := range contexts {
if ctx == context {
found = true
break
}
}
if !found {
contexts = append(contexts, context)
}
}

c.Data["branch_status_check_contexts"] = contexts
c.Data["is_context_required"] = func(context string) bool {
for _, c := range protectBranch.StatusCheckContexts {
if c == context {
return true
}
}
return false
}

if c.Repo.Owner.IsOrganization() {
teams, err := c.Repo.Owner.TeamsWithAccessToRepo(c.Repo.Repository.ID, models.AccessModeRead)
Expand Down Expand Up @@ -186,6 +210,10 @@ func SettingsProtectedBranchPost(ctx *context.Context, f auth.ProtectBranchForm)
if strings.TrimSpace(f.MergeWhitelistTeams) != "" {
mergeWhitelistTeams, _ = base.StringsToInt64s(strings.Split(f.MergeWhitelistTeams, ","))
}
lafriks marked this conversation as resolved.
Show resolved Hide resolved

protectBranch.EnableStatusCheck = f.EnableStatusCheck
protectBranch.StatusCheckContexts = f.StatusCheckContexts

protectBranch.RequiredApprovals = f.RequiredApprovals
if strings.TrimSpace(f.ApprovalsWhitelistUsers) != "" {
approvalsWhitelistUsers, _ = base.StringsToInt64s(strings.Split(f.ApprovalsWhitelistUsers, ","))
Expand Down
Loading