Repository: jenkins-x/lighthouse (go)

Issue:
When keeper fails to merge a PR it would be nice to know why
We had a recent situation where lighthouse was failing to merge a PR on a new repo (due to github config) but it would be nice to surface the error message somewhere so the user can take action.

The error inside keeper was:

```{"base-sha":"cb2bcdde269d75c1e37046226a66e43f9d5a4039","branch":"master","component":"keeper","controller":"sync","error":"Merge commits are not allowed on this repository.","file":"/workspace/source/pkg/keeper/keeper.go:976","func":"github.com/jenkins-x/lighthouse/pkg/keeper.(*DefaultController).mergePRs","level":"error","merge-targets":[24],"msg":"Merge failed.","org":"jenkins-x","pr":24,"repo":"jx-api","sha":"59090827b78bba840f60cf0491c5fae8cd34c36c","time":"2020-06-23T15:55:59Z"}
{"action":"MERGE","base-sha":"cb2bcdde269d75c1e37046226a66e43f9d5a4039","branch":"master","component":"keeper","controller":"sync","file":"/workspace/source/pkg/keeper/keeper.go:1314","func":"github.com/jenkins-x/lighthouse/pkg/keeper.(*DefaultController).syncSubpool","level":"info","msg":"Subpool synced.","org":"jenkins-x","repo":"jx-api","targets":[24],"time":"2020-06-23T15:55:59Z"}
```

Could this be added as a status message, or a comment on the PR?

Pull request:
feat: Comment on PRs that failed to merge
fixes #824

Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

Patch:
diff --git a/dependency-matrix/matrix.md b/dependency-matrix/matrix.md
index 475001a9..eecfe641 100644
--- a/dependency-matrix/matrix.md
+++ b/dependency-matrix/matrix.md
@@ -2,6 +2,6 @@
 
 Dependency | Sources | Version | Mismatched versions
 ---------- | ------- | ------- | -------------------
-[jenkins-x/go-scm](https://github.com/jenkins-x/go-scm) |  | [1.5.154]() | 
+[jenkins-x/go-scm](https://github.com/jenkins-x/go-scm) |  | [1.5.152]() | 
 [jenkins-x/jx](https://github.com/jenkins-x/jx) |  | [2.1.114](https://github.com/jenkins-x/jx/releases/tag/v2.1.114) | 
 [jenkins-x/lighthouse-config](https://github.com/jenkins-x/lighthouse-config) |  | [0.0.9]() | 
diff --git a/dependency-matrix/matrix.yaml b/dependency-matrix/matrix.yaml
index fa3bbc4c..78ad5f98 100644
--- a/dependency-matrix/matrix.yaml
+++ b/dependency-matrix/matrix.yaml
@@ -3,7 +3,7 @@ dependencies:
   owner: jenkins-x
   repo: go-scm
   url: https://github.com/jenkins-x/go-scm
-  version: 1.5.154
+  version: 1.5.152
   versionURL: ""
 - host: github.com
   owner: jenkins-x
diff --git a/go.mod b/go.mod
index 2aa3104c..883e9a20 100644
--- a/go.mod
+++ b/go.mod
@@ -8,7 +8,7 @@ require (
 	github.com/google/go-cmp v0.4.1
 	github.com/gorilla/sessions v1.2.0
 	github.com/hashicorp/go-multierror v1.1.0
-	github.com/jenkins-x/go-scm v1.5.154
+	github.com/jenkins-x/go-scm v1.5.153
 	github.com/mattn/go-zglob v0.0.1
 	github.com/onsi/ginkgo v1.11.0
 	github.com/onsi/gomega v1.8.1
diff --git a/go.sum b/go.sum
index 6ccd1efd..e31e847e 100644
--- a/go.sum
+++ b/go.sum
@@ -627,8 +627,10 @@ github.com/jcmturner/gofork v1.0.0/go.mod h1:MK8+TM0La+2rjBD4jE12Kj1pCCxK7d2LK/U
 github.com/jenkins-x/go-scm v1.5.65/go.mod h1:MgGRkJScE/rJ30J/bXYqduN5sDPZqZFITJopsnZmTOw=
 github.com/jenkins-x/go-scm v1.5.79/go.mod h1:PCT338UhP/pQ0IeEeMEf/hoLTYKcH7qjGEKd7jPkeYg=
 github.com/jenkins-x/go-scm v1.5.117/go.mod h1:PCT338UhP/pQ0IeEeMEf/hoLTYKcH7qjGEKd7jPkeYg=
-github.com/jenkins-x/go-scm v1.5.154 h1:LD/Pdx4O8JFTPKhFF/rrvWrw40+LPSA1V0w9eOLvuNk=
-github.com/jenkins-x/go-scm v1.5.154/go.mod h1:PCT338UhP/pQ0IeEeMEf/hoLTYKcH7qjGEKd7jPkeYg=
+github.com/jenkins-x/go-scm v1.5.152 h1:Zn/5ZOpFJMkCe0l6e4+my1E+uOBkkyRsVdWAaHn29Xg=
+github.com/jenkins-x/go-scm v1.5.152/go.mod h1:PCT338UhP/pQ0IeEeMEf/hoLTYKcH7qjGEKd7jPkeYg=
+github.com/jenkins-x/go-scm v1.5.153 h1:FJmGMbCkByuOMNCcCyPa7gMLeqNyuQQB3azBVmiTdMI=
+github.com/jenkins-x/go-scm v1.5.153/go.mod h1:PCT338UhP/pQ0IeEeMEf/hoLTYKcH7qjGEKd7jPkeYg=
 github.com/jenkins-x/logrus-stackdriver-formatter v0.1.1-0.20200408213659-1dcf20c371bb h1:woC0LbYpL9rgwZn13Z0KQBEGAmi9ugpWgWtQezHqsBM=
 github.com/jenkins-x/logrus-stackdriver-formatter v0.1.1-0.20200408213659-1dcf20c371bb/go.mod h1:Erk5rrGYkvFlFAoVbz/7obMDEIUFCEbhK1odoR0Msqo=
 github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI=
diff --git a/pkg/keeper/keeper.go b/pkg/keeper/keeper.go
index a0153782..db6b823e 100644
--- a/pkg/keeper/keeper.go
+++ b/pkg/keeper/keeper.go
@@ -69,6 +69,7 @@ type scmProviderClient interface {
 	ProviderType() string
 	GetRepositoryByFullName(string) (*scm.Repository, error)
 	ListAllPullRequestsForFullNameRepo(string, scm.PullRequestListOptions) ([]*scm.PullRequest, error)
+	CreateComment(owner, repo string, number int, isPR bool, comment string) error
 }
 
 type contextChecker interface {
@@ -938,6 +939,7 @@ func (c *DefaultController) prepareMergeDetails(commitTemplates config.KeeperMer
 
 func (c *DefaultController) mergePRs(sp subpool, prs []PullRequest) error {
 	var merged, failed []int
+	var failedPRs []PullRequest
 	defer func() {
 		if len(merged) == 0 {
 			return
@@ -961,6 +963,7 @@ func (c *DefaultController) mergePRs(sp subpool, prs []PullRequest) error {
 				log.WithError(err).Error("Merge failed.")
 				errs = append(errs, err)
 				failed = append(failed, int(pr.Number))
+				failedPRs = append(failedPRs, pr)
 				continue
 			}
 		}
@@ -973,6 +976,7 @@ func (c *DefaultController) mergePRs(sp subpool, prs []PullRequest) error {
 			log.WithError(err).Error("Merge failed.")
 			errs = append(errs, err)
 			failed = append(failed, int(pr.Number))
+			failedPRs = append(failedPRs, pr)
 		} else {
 			log.Info("Merged.")
 			merged = append(merged, int(pr.Number))
@@ -991,6 +995,18 @@ func (c *DefaultController) mergePRs(sp subpool, prs []PullRequest) error {
 		return nil
 	}
 
+	finalErr := rollupMergeErrors(prs, failed, merged, errs)
+	reportErr := c.commentOnPRsWithFailedMerge(failedPRs, finalErr.Error())
+	if reportErr != nil {
+		sp.log.WithFields(logrus.Fields{
+			"targets": prNumbers(failedPRs),
+		}).WithError(reportErr).Error("error reporting back to PRs upon failed merge")
+	}
+
+	return finalErr
+}
+
+func rollupMergeErrors(prs []PullRequest, failed []int, merged []int, errs []error) error {
 	// Construct a more informative error.
 	var batch string
 	if len(prs) > 1 {
@@ -999,9 +1015,27 @@ func (c *DefaultController) mergePRs(sp subpool, prs []PullRequest) error {
 			batch = fmt.Sprintf("%s, partial merge %v", batch, merged)
 		}
 	}
+
 	return fmt.Errorf("failed merging %v%s: %v", failed, batch, errorutil.NewAggregate(errs...))
 }
 
+func mergeErrorDetail(origErr error) error {
+	switch origErr.(type) {
+	case scmprovider.ModifiedHeadError:
+		return fmt.Errorf("PR was modified: %v", origErr)
+	case scmprovider.UnmergablePRBaseChangedError:
+		return fmt.Errorf("base branch was modified: %v", origErr)
+	case scmprovider.UnauthorizedToPushError:
+		return fmt.Errorf("branch needs to be configured to allow this robot to push: %v", origErr)
+	case scmprovider.MergeCommitsForbiddenError:
+		return fmt.Errorf("keeper needs to be configured to use the 'rebase' merge method for this repo or the repo needs to allow merge commits: %v", origErr)
+	case scmprovider.UnmergablePRError:
+		return fmt.Errorf("PR is unmergable. Do the Keeper merge requirements match the SCM provider settings for the repo? %v", origErr)
+	default:
+		return origErr
+	}
+}
+
 // tryMerge attempts 1 merge and returns a bool indicating if we should try
 // to merge the remaining PRs and possibly an error.
 func tryMerge(mergeFunc func() error) (bool, error) {
@@ -1013,6 +1047,7 @@ func tryMerge(mergeFunc func() error) (bool, error) {
 			// Successful merge!
 			return true, nil
 		}
+		detailedErr := mergeErrorDetail(err)
 		// TODO: Add a config option to abort batches if a PR in the batch
 		// cannot be merged for any reason. This would skip merging
 		// not just the changed PR, but also the other PRs in the batch.
@@ -1026,7 +1061,7 @@ func tryMerge(mergeFunc func() error) (bool, error) {
 			// modifies their PR as we try to merge it in a batch then we
 			// end up in an untested state. This is unlikely to cause any
 			// real problems.
-			return true, fmt.Errorf("PR was modified: %v", err)
+			return true, detailedErr
 		} else if _, ok = err.(scmprovider.UnmergablePRBaseChangedError); ok {
 			//  complained that the base branch was modified. This is a
 			// strange error because the API doesn't even allow the request to
@@ -1036,7 +1071,7 @@ func tryMerge(mergeFunc func() error) (bool, error) {
 			// in time. https://gitprovider.com/kubernetes/test-infra/issues/5171
 			// We handle this by sleeping for a few seconds before trying to
 			// merge again.
-			err = fmt.Errorf("base branch was modified: %v", err)
+			err = detailedErr
 			if retry+1 < maxRetries {
 				sleep(backoff)
 				backoff *= 2
@@ -1047,15 +1082,15 @@ func tryMerge(mergeFunc func() error) (bool, error) {
 			// overzealous branch protection setting will not allow the robot to
 			// push to a specific branch.
 			// We won't be able to merge the other PRs.
-			return false, fmt.Errorf("branch needs to be configured to allow this robot to push: %v", err)
+			return false, detailedErr
 		} else if _, ok = err.(scmprovider.MergeCommitsForbiddenError); ok {
 			// GitHub let us know that the merge method configured for this repo
 			// is not allowed by other repo settings, so we should let the admins
 			// know that the configuration needs to be updated.
 			// We won't be able to merge the other PRs.
-			return false, fmt.Errorf("Keeper needs to be configured to use the 'rebase' merge method for this repo or the repo needs to allow merge commits: %v", err)
+			return false, detailedErr
 		} else if _, ok = err.(scmprovider.UnmergablePRError); ok {
-			return true, fmt.Errorf("PR is unmergable. Do the Keeper merge requirements match the GitHub settings for the repo? %v", err)
+			return true, detailedErr
 		}
 		return true, err
 	}
@@ -1258,6 +1293,24 @@ func (c *DefaultController) presubmitsByPull(sp *subpool) (map[int][]config.Pres
 	return presubmits, nil
 }
 
+func (c *DefaultController) commentOnPRsWithFailedMerge(prs []PullRequest, errorString string) error {
+
+	commentBody := fmt.Sprintf("Failed to merge this PR due to:\n>%s\n", errorString)
+
+	var errs []error
+	for _, pr := range prs {
+		err := c.spc.CreateComment(string(pr.Repository.Owner.Login), string(pr.Repository.Name), int(pr.Number), true, commentBody)
+		if err != nil {
+			errs = append(errs, err)
+		}
+	}
+
+	if len(errs) > 0 {
+		return errors.Wrap(errorutil.NewAggregate(errs...), "error reporting on failed merges")
+	}
+	return nil
+}
+
 func (c *DefaultController) syncSubpool(sp subpool, blocks []blockers.Blocker) (Pool, error) {
 	sp.log.Infof("Syncing subpool: %d PRs, %d PJs.", len(sp.prs), len(sp.ljs))
 	successes, pendings, missings, missingSerialTests := accumulate(sp.presubmits, sp.prs, sp.ljs, sp.log)


Test patch:
diff --git a/pkg/keeper/keeper_test.go b/pkg/keeper/keeper_test.go
index eb609ff6..11e92b60 100644
--- a/pkg/keeper/keeper_test.go
+++ b/pkg/keeper/keeper_test.go
@@ -491,11 +491,12 @@ func TestAccumulate(t *testing.T) {
 }
 
 type fgc struct {
-	prs       []PullRequest
-	refs      map[string]string
-	merged    int
-	setStatus bool
-	mergeErrs map[int]error
+	prs              []PullRequest
+	refs             map[string]string
+	merged           int
+	setStatus        bool
+	mergeErrs        map[int]error
+	mergeErrComments map[int]string
 
 	expectedSHA    string
 	ignoreExpected bool
@@ -550,6 +551,14 @@ func (f *fgc) Query(ctx context.Context, q interface{}, vars map[string]interfac
 	return nil
 }
 
+func (f *fgc) CreateComment(org, repo string, number int, isPR bool, commentBody string) error {
+	if f.mergeErrComments == nil {
+		f.mergeErrComments = make(map[int]string)
+	}
+	f.mergeErrComments[number] = commentBody
+	return nil
+}
+
 func (f *fgc) Merge(org, repo string, number int, details scmprovider.MergeDetails) error {
 	if err, ok := f.mergeErrs[number]; ok {
 		return err
@@ -1364,11 +1373,47 @@ func TestTakeAction(t *testing.T) {
 			if tc.batchPending {
 				batchPending = []PullRequest{{}}
 			}
+			successes := genPulls(tc.successes)
+			pendings := genPulls(tc.pendings)
+			nones := genPulls(tc.nones)
+			batchMerges := genPulls(tc.batchMerges)
 			t.Logf("Test case: %s", tc.name)
-			if act, _, err := c.takeAction(sp, batchPending, genPulls(tc.successes), genPulls(tc.pendings), genPulls(tc.nones), genPulls(tc.batchMerges), sp.presubmits); err != nil && !tc.expectErr {
+			if act, _, err := c.takeAction(sp, batchPending, successes, pendings, nones, batchMerges, sp.presubmits); err != nil && !tc.expectErr {
 				t.Fatalf("Unexpected error in takeAction: %v", err)
 			} else if err == nil && tc.expectErr {
 				t.Error("Missing expected error from takeAction.")
+			} else if err != nil && tc.expectErr {
+				var prsToMerge []PullRequest
+				var failed []int
+				var merged []int
+				if len(batchMerges) > 0 {
+					prsToMerge = append(prsToMerge, batchMerges...)
+					failedAndStopped := false
+					for _, pr := range batchMerges {
+						prNum := int(pr.Number)
+						if mergeErr, ok := tc.mergeErrs[prNum]; ok {
+							failed = append(failed, prNum)
+							switch mergeErr.(type) {
+							case scmprovider.MergeCommitsForbiddenError, scmprovider.UnauthorizedToPushError:
+								failedAndStopped = true
+							default:
+								continue
+							}
+						} else if !failedAndStopped {
+							merged = append(merged, prNum)
+						}
+					}
+				} else {
+					if ok, prToMerge := pickSmallestPassingNumber(sp.log, c.spc, successes, sp.cc); ok {
+						prsToMerge = append(prsToMerge, prToMerge)
+						failed = append(failed, int(prToMerge.Number))
+					}
+				}
+				for prNum, rawErr := range tc.mergeErrs {
+					detailedErr := mergeErrorDetail(rawErr)
+					expectedErr := rollupMergeErrors(prsToMerge, failed, merged, []error{detailedErr})
+					assert.Equal(t, fmt.Sprintf("Failed to merge this PR due to:\n>%s\n", expectedErr.Error()), fgc.mergeErrComments[prNum])
+				}
 			} else if act != tc.action {
 				t.Errorf("Wrong action. Got %v, wanted %v.", act, tc.action)
 			}