Merge pull request #27 from capitalone/0.26.0-release

0.26.0 release

CHANGELOG.md

@@ -3,6 +3,20 @@ at 4cbae5. Checks-out uses [semantic versioning](http://semver.org/). The
 Checks-out configuration format is incompatible with the LGTM configuration
 format but the legacy format can be parsed.
 
+# 0.26.0
+
+* Reduce scope of 'authoraffirm' feature. Themessage will only appear
+when the pull request has multiple committers AND there are approvers
+who are also committers. If the approvers are not committers then no
+error message appears. When the error message appears, we have changed
+the wording of the message posted to the pull request.
+* Improvement to branch deletion protected that was added in 0.25.0.
+If deletion of the compare branch is enabled then the final approval
+policy in the policy array must either have a match of "off" or have
+deletion disabled in the policy.
+* Fix several 500-level http responses that should be 400-level
+responses.
+
 # 0.25.0
 
 * Add 'authoraffirm' feature when multiple committers on pull request.

model/commit.go

@@ -7,4 +7,5 @@ type Commit struct {
 	Committer string
 	Message   string
 	SHA       string
+	Parents   []string
 }

remote/github/github.go

@@ -1110,11 +1110,16 @@ func getPullRequestCommits(ctx context.Context, client *github.Client, r *model.
 	}
 	res := []model.Commit{}
 	for _, c := range commits {
+		parents := []string{}
+		for _, p := range c.Commit.Parents {
+			parents = append(parents, p.GetSHA())
+		}
 		res = append(res, model.Commit{
 			Author:    lowercase.Create(c.Author.GetLogin()),
 			Committer: c.Commit.Committer.GetName(),
 			Message:   c.Commit.GetMessage(),
 			SHA:       c.GetSHA(),
+			Parents:   parents,
 		})
 	}
 	return res, nil

snapshot/errors.go

@@ -25,5 +25,8 @@ import (
 )
 
 func badRequest(err error) error {
+	if err == nil {
+		return nil
+	}
 	return exterror.Create(http.StatusBadRequest, err)
 }

snapshot/snapshot.go

@@ -55,17 +55,26 @@ type OrgLazyLoad struct {
 }
 
 func findConfig(c context.Context, user *model.User, caps *model.Capabilities, repo *model.Repo) (*model.Config, error) {
+	var cfg *model.Config
 	var rcfile []byte
 	var exterr, err error
 	// look for configuration file in current repository
 	rcfile, exterr = remote.GetContents(c, user, repo, configFileName)
 	if exterr == nil {
-		return model.ParseConfig(rcfile, caps)
+		cfg, err = model.ParseConfig(rcfile, caps)
+		if err != nil {
+			return nil, badRequest(err)
+		}
+		return cfg, nil
 	}
 	// look for legacy file
 	rcfile, err = remote.GetContents(c, user, repo, ".lgtm")
 	if err == nil {
-		return model.ParseOldConfig(rcfile)
+		cfg, err = model.ParseOldConfig(rcfile)
+		if err != nil {
+			return nil, badRequest(err)
+		}
+		return cfg, nil
 	}
 	// look for template configuration file in org repository
 	if !repo.Org || len(orgRepoName) == 0 {
@@ -82,10 +91,14 @@ func findConfig(c context.Context, user *model.User, caps *model.Capabilities, r
 		return nil, multierror.Append(exterr, err)
 	}
 	rcfile, err = remote.GetContents(c, user, &orgRepo, ConfigTemplateName)
-	if err == nil {
-		return model.ParseConfig(rcfile, caps)
+	if err != nil {
+		return nil, multierror.Append(exterr, err)
+	}
+	cfg, err = model.ParseConfig(rcfile, caps)
+	if err != nil {
+		return nil, badRequest(err)
 	}
-	return nil, multierror.Append(exterr, err)
+	return cfg, nil
 }
 
 func GetConfig(c context.Context, user *model.User, caps *model.Capabilities, repo *model.Repo) (*model.Config, error) {

snapshot/validate.go

@@ -28,14 +28,14 @@ func validateSnapshot(config *model.Config, snapshot *model.MaintainerSnapshot)
 	var errs error
 	for _, approval := range config.Approvals {
 		err := approval.Match.Validate(snapshot)
-		errs = multierror.Append(errs, err)
+		errs = multierror.Append(errs, badRequest(err))
 		if approval.AntiMatch != nil {
 			err := approval.AntiMatch.Validate(snapshot)
-			errs = multierror.Append(errs, err)
+			errs = multierror.Append(errs, badRequest(err))
 		}
 		if approval.AuthorMatch != nil {
 			err := approval.AuthorMatch.Validate(snapshot)
-			errs = multierror.Append(errs, err)
+			errs = multierror.Append(errs, badRequest(err))
 		}
 	}
 	return errs

web/approval.go

@@ -30,6 +30,7 @@ import (
 	"github.com/capitalone/checks-out/model"
 	"github.com/capitalone/checks-out/remote"
 	"github.com/capitalone/checks-out/set"
+	"github.com/capitalone/checks-out/strings/lowercase"
 
 	log "github.com/Sirupsen/logrus"
 )
@@ -84,7 +85,9 @@ func approve(c context.Context, params HookParams, id int, setStatus bool) (*App
 	return approvePullRequest(c, params, id, &pullRequest, setStatus)
 }
 
-const authorAffirmMsg = "Multiple committers detected on PR branch. PR author must approve pull request. PR author must review the commits from the other committers."
+const authorAffirmMsg = "Someone besides the committers and the PR author should approve the pull request. " +
+	"If this is not possible, the PR author can approve to indicate they have reviewed the other commits in the PR. " +
+	"The PR author must approve with a comment directly on the PR (GitHub Reviews comments will not work)."
 
 var affirmMsgActions = set.New("opened", "reopened", "synchronized")
 
@@ -108,7 +111,7 @@ func approvePullRequest(c context.Context, params HookParams, id int, pullReques
 
 	if setStatus {
 		if !approval.AuthorAffirmed {
-			if params.Event == "pull_request" && affirmMsgActions.Contains(params.Action) {
+			if params.Approval != nil && params.Approval.IsApproval(&request) {
 				err = remote.WriteComment(c, user, repo, pullRequest.Number, authorAffirmMsg)
 				if err != nil {
 					return nil, err
@@ -259,7 +262,7 @@ func authorAffirm(request *model.ApprovalRequest, policy *model.ApprovalPolicy)
 		return true
 	}
 	for _, c := range request.Commits {
-		if systemAccounts.Contains(c.Committer) {
+		if len(c.Parents) == 2 && systemAccounts.Contains(c.Committer) {
 			continue
 		}
 		if c.Author != request.PullRequest.Author {
@@ -269,13 +272,23 @@ func authorAffirm(request *model.ApprovalRequest, policy *model.ApprovalPolicy)
 	return true
 }
 
+func removeAuthor(author lowercase.String, req *model.ApprovalRequest) {
+	var filter []model.Feedback
+	for _, fb := range req.ApprovalComments {
+		if fb.GetAuthor() != author {
+			filter = append(filter, fb)
+		}
+	}
+	req.ApprovalComments = filter
+}
+
 func calculateApprovalInfo(request *model.ApprovalRequest, policy *model.ApprovalPolicy, audit bool) (*ApprovalInfo, error) {
 	approvers := set.Empty()
 	disapprovers := set.Empty()
 	validAuthor := false
 	validTitle := false
 	validAudit := audit
-	authorAffirm := authorAffirm(request, policy)
+	authAffirm := false
 	approved, err := model.Approve(request, policy,
 		func(f model.Feedback, op model.ApprovalOp) {
 			author := f.GetAuthor().String()
@@ -297,15 +310,40 @@ func calculateApprovalInfo(request *model.ApprovalRequest, policy *model.Approva
 	if err != nil {
 		return nil, err
 	}
-	approved = approved && audit && authorAffirm
+	if !approved {
+		// only after sufficient approvals are received
+		// test the author affirm feature
+		authAffirm = true
+	} else {
+		authAffirm = authorAffirm(request, policy)
+		if !authAffirm {
+			clone := *request
+			for _, c := range request.Commits {
+				removeAuthor(c.Author, &clone)
+			}
+			approvers = set.Empty()
+			authAffirm, err = model.Approve(&clone, policy,
+				func(f model.Feedback, op model.ApprovalOp) {
+					author := f.GetAuthor().String()
+					switch op {
+					case model.Approval:
+						approvers.Add(author)
+					}
+				})
+			if err != nil {
+				return nil, err
+			}
+		}
+	}
+	approved = approved && audit && authAffirm
 
 	ai := ApprovalInfo{
 		Policy:         policy,
 		Approved:       approved,
 		AuthorApproved: validAuthor,
 		TitleApproved:  validTitle,
 		AuditApproved:  validAudit,
-		AuthorAffirmed: authorAffirm,
+		AuthorAffirmed: authAffirm,
 		Approvers:      approvers,
 		Disapprovers:   disapprovers,
 		CurCommentInfo: CurCommentInfo{
@@ -355,7 +393,7 @@ func generateStatus(info *ApprovalInfo) (string, string) {
 		}
 	} else if !info.AuthorAffirmed {
 		status = "error"
-		desc = "multiple committers. author must approve PR"
+		desc = "non-committer or PR author must approve"
 	} else if !info.AuditApproved {
 		status = "error"
 		desc = "audit chain must be manually approved"

web/approval_hook.go

@@ -24,11 +24,12 @@ import (
 	"github.com/capitalone/checks-out/notifier"
 )
 
-func doApprovalHook(c context.Context, hook *ApprovalHook) (*ApprovalOutput, error) {
+func doApprovalHook(c context.Context, hook *ApprovalHook, isApp IsApprover) (*ApprovalOutput, error) {
 	params, err := GetHookParameters(c, hook.HookCommon, hook.Repo.Slug)
 	if err != nil {
 		return nil, err
 	}
+	params.Approval = isApp
 	approvalInfo, err := approve(c, params, hook.Issue.Number, true)
 
 	if err != nil {

web/approval_test.go

@@ -87,7 +87,7 @@ func TestGenerateStatus(t *testing.T) {
 			AuthorAffirmed: false,
 		}: {
 			status: "error",
-			desc:   "multiple committers. author must approve PR",
+			desc:   "non-committer or PR author must approve",
 		},
 
 		&ApprovalInfo{

web/comment_hook.go

@@ -22,8 +22,9 @@ import (
 	"context"
 	"strings"
 
-	multierror "github.com/mspiegel/go-multierror"
 	"github.com/capitalone/checks-out/model"
+
+	multierror "github.com/mspiegel/go-multierror"
 )
 
 func (hook *CommentHook) Process(c context.Context) (interface{}, error) {
@@ -39,5 +40,5 @@ func doCommentHook(c context.Context, hook *CommentHook) (*ApprovalOutput, error
 	if strings.HasPrefix(hook.Comment, model.CommentPrefix) {
 		return nil, nil
 	}
-	return doApprovalHook(c, &hook.ApprovalHook)
+	return doApprovalHook(c, &hook.ApprovalHook, hook)
 }

web/github_create.go

@@ -113,6 +113,7 @@ func createReviewHook(body []byte) (Hook, error) {
 				Slug:  data.Repo.GetFullName(),
 			},
 		},
+		State: lowercase.Create(data.Review.GetState()),
 	}
 
 	return hook, nil

web/hook.go

@@ -22,6 +22,7 @@ import (
 	"context"
 
 	"github.com/capitalone/checks-out/model"
+	"github.com/capitalone/checks-out/strings/lowercase"
 	"github.com/gin-gonic/gin"
 )
 
@@ -30,6 +31,10 @@ type Hook interface {
 	SetEvent(event string)
 }
 
+type IsApprover interface {
+	IsApproval(req *model.ApprovalRequest) bool
+}
+
 type HookCommon struct {
 	Event  string
 	Action string
@@ -48,6 +53,7 @@ type CommentHook struct {
 
 type ReviewHook struct {
 	ApprovalHook
+	State lowercase.String
 }
 
 type PRHook struct {
@@ -77,6 +83,7 @@ type HookParams struct {
 	Snapshot *model.MaintainerSnapshot
 	Event    string
 	Action   string
+	Approval IsApprover
 }
 
 func ProcessHook(c *gin.Context) {
@@ -100,6 +107,20 @@ func ProcessHook(c *gin.Context) {
 	}
 }
 
+func (h *CommentHook) IsApproval(req *model.ApprovalRequest) bool {
+	c := model.Comment{
+		Body: h.Comment,
+	}
+	return c.IsApproval(req)
+}
+
+func (h *ReviewHook) IsApproval(req *model.ApprovalRequest) bool {
+	r := model.Review{
+		State: h.State,
+	}
+	return r.IsApproval(req)
+}
+
 func (h *HookCommon) SetEvent(event string) {
 	h.Event = event
 }

web/review_hook.go

@@ -25,7 +25,7 @@ import (
 )
 
 func (hook *ReviewHook) Process(c context.Context) (interface{}, error) {
-	approvalOutput, e1 := doApprovalHook(c, &hook.ApprovalHook)
+	approvalOutput, e1 := doApprovalHook(c, &hook.ApprovalHook, hook)
 	if e1 != nil {
 		e2 := sendErrorStatus(c, &hook.ApprovalHook, e1)
 		e1 = multierror.Append(e1, e2)

web/status_hook.go

@@ -210,6 +210,16 @@ func sendMessage(c context.Context, config *model.Config, mw *notifier.MessageWr
 
 const msgBlockDeletion = "Deletion of the merged branch %s has been blocked. "
 
+func finalPolicyEligible(p *model.ApprovalPolicy) bool {
+	if p.Match.Matcher.GetType() == "off" {
+		return true
+	}
+	if p.Merge != nil && !p.Merge.Delete {
+		return true
+	}
+	return false
+}
+
 func eligibleForDeletion(req *model.ApprovalRequest, mw *notifier.MessageWrapper) bool {
 	if req.PullRequest.Branch.CompareOwner != req.Repository.Owner {
 		mw.Messages = append(mw.Messages, notifier.MessageInfo{
@@ -223,10 +233,12 @@ func eligibleForDeletion(req *model.ApprovalRequest, mw *notifier.MessageWrapper
 	cfg := req.Config
 	for _, p := range cfg.Approvals {
 		if p.Scope.ValidateFinal() == nil {
-			if p.Match.Matcher.GetType() != "off" {
+			if !finalPolicyEligible(p) {
 				mw.Messages = append(mw.Messages, notifier.MessageInfo{
 					Message: fmt.Sprintf(
-						msgBlockDeletion+"Approval policy %s has a match that is not 'off'",
+						msgBlockDeletion+
+							"Approval policy %s must either have match policy "+
+							"'off' or merge deletion disabled",
 						req.PullRequest.Branch.CompareName, p.Name),
 					Type: model.CommentDelete,
 				})

web/status_hook_test.go

@@ -728,6 +728,13 @@ func TestEligibleForDeletion(t *testing.T) {
 	if !eligibleForDeletion(request, mw) {
 		t.Error("off policy should be eligible for deletion")
 	}
+	config.Approvals[0] = model.DefaultApprovalPolicy()
+	config.Approvals[0].Merge = &model.MergeConfig{
+		Delete: false,
+	}
+	if !eligibleForDeletion(request, mw) {
+		t.Error("disable deletion should allow other branches to be deleted")
+	}
 	config.Approvals = append(config.Approvals, model.DefaultApprovalPolicy())
 	config.Approvals[0].Scope.Branches = set.New("baz")
 	if eligibleForDeletion(request, mw) {