Repository: jenkins-x/lighthouse
Pull request #1044 (2020-09-11 12:53:31)

refactor: approve plugin
This PR refactors approve plugin.

jenkins-x-bot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by:
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **eddycharly**
You can assign the PR to them by writing `/assign @eddycharly` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=jenkins-x%2Flighthouse).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/jenkins-x/lighthouse/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["eddycharly"]} -->
eddycharly: /hold

must come after https://github.com/jenkins-x/lighthouse/pull/1042

Patch:
diff --git a/dependency-matrix/matrix.md b/dependency-matrix/matrix.md
index bd3a738e..7fa12c91 100644
--- a/dependency-matrix/matrix.md
+++ b/dependency-matrix/matrix.md
@@ -2,4 +2,4 @@
 
 Dependency | Sources | Version | Mismatched versions
 ---------- | ------- | ------- | -------------------
-[jenkins-x/go-scm](https://github.com/jenkins-x/go-scm) |  | [1.5.163]() | 
+[jenkins-x/go-scm](https://github.com/jenkins-x/go-scm) |  | [1.5.164]() | 
diff --git a/dependency-matrix/matrix.yaml b/dependency-matrix/matrix.yaml
index 962352ce..334ed9f8 100644
--- a/dependency-matrix/matrix.yaml
+++ b/dependency-matrix/matrix.yaml
@@ -3,5 +3,5 @@ dependencies:
   owner: jenkins-x
   repo: go-scm
   url: https://github.com/jenkins-x/go-scm
-  version: 1.5.163
+  version: 1.5.164
   versionURL: ""
diff --git a/go.mod b/go.mod
index e0a51da4..d454cc57 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.163
+	github.com/jenkins-x/go-scm v1.5.164
 	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 0126f964..bea351ca 100644
--- a/go.sum
+++ b/go.sum
@@ -582,8 +582,8 @@ 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.163 h1:dTMrxOWHvpn/DgG2bWAY5L2h5i18DOfmjQu/ODBbL7Y=
-github.com/jenkins-x/go-scm v1.5.163/go.mod h1:wJgj7PfgsCs+YAXC0vEj43g48HKf94apSyza62ADxKY=
+github.com/jenkins-x/go-scm v1.5.164 h1:p+N22JDRRuWLg/MTBD58eWoJhTdYYTx3FejaXG+pUwo=
+github.com/jenkins-x/go-scm v1.5.164/go.mod h1:wJgj7PfgsCs+YAXC0vEj43g48HKf94apSyza62ADxKY=
 github.com/jessevdk/go-flags v1.4.0/go.mod h1:4FA24M0QyGHXBuZZK/XkWh8h0e1EYbRYJSGM75WSRxI=
 github.com/jinzhu/gorm v0.0.0-20170316141641-572d0a0ab1eb/go.mod h1:Vla75njaFJ8clLU1W44h34PjIkijhjHIYnZxMqCdxqo=
 github.com/jinzhu/gorm v1.9.12/go.mod h1:vhTjlKSJUTWNtcbQtrMBFCxy7eXTzeCAzfL5fBZT/Qs=
diff --git a/pkg/plugins/approve/approve.go b/pkg/plugins/approve/approve.go
index f17b1f44..6be8370c 100644
--- a/pkg/plugins/approve/approve.go
+++ b/pkg/plugins/approve/approve.go
@@ -50,7 +50,7 @@ const (
 
 var (
 	associatedIssueRegexFormat = `(?:%s/[^/]+/issues/|#)(\d+)`
-	commandRegex               = regexp.MustCompile(`(?m)^/([^\s]+)[\t ]*([^\n\r]*)`)
+	commandRegex               = regexp.MustCompile(`(?mi)^/(?:lh-)?(lgtm|approve)(?:\s+(no-issue|cancel))?.*$`)
 	notificationRegex          = regexp.MustCompile(`(?is)^\[` + approvers.ApprovalNotificationName + `\] *?([^\n]*)(?:\n\n(.*))?`)
 
 	// deprecatedBotNames are the names of the bots that previously handled approvals.
@@ -103,10 +103,11 @@ var (
 		ReviewEventHandler: handleReviewEvent,
 		PullRequestHandler: handlePullRequestEvent,
 		Commands: []plugins.Command{{
-			GenericCommentHandler: handleGenericCommentEvent,
 			Filter: func(e scmprovider.GenericCommentEvent) bool {
 				return !(e.Action != scm.ActionCreate || !e.IsPR || e.IssueState == "closed")
 			},
+			Regex:                 commandRegex,
+			GenericCommentHandler: handleGenericCommentEvent,
 			Help: []pluginhelp.Command{{
 				Usage:       "/approve [no-issue|cancel]",
 				Description: "Approves a pull request",
diff --git a/pkg/plugins/help/help.go b/pkg/plugins/help/help.go
index 03459f52..9772fb3d 100644
--- a/pkg/plugins/help/help.go
+++ b/pkg/plugins/help/help.go
@@ -31,13 +31,10 @@ import (
 const pluginName = "help"
 
 var (
-	helpRe                     = regexp.MustCompile(`(?mi)^/(?:lh-)?help\s*$`)
-	helpRemoveRe               = regexp.MustCompile(`(?mi)^/(?:lh-)?remove-help\s*$`)
-	helpGoodFirstIssueRe       = regexp.MustCompile(`(?mi)^/(?:lh-)?good-first-issue\s*$`)
-	helpGoodFirstIssueRemoveRe = regexp.MustCompile(`(?mi)^/(?:lh-)?remove-good-first-issue\s*$`)
-	helpGuidelinesURL          = "https://git.k8s.io/community/contributors/guide/help-wanted.md"
-	helpMsgPruneMatch          = "This request has been marked as needing help from a contributor."
-	helpMsg                    = `
+	helpRe            = regexp.MustCompile(`(?mi)^/(?:lh-)?(help|remove-help|good-first-issue|remove-good-first-issue)\s*$`)
+	helpGuidelinesURL = "https://git.k8s.io/community/contributors/guide/help-wanted.md"
+	helpMsgPruneMatch = "This request has been marked as needing help from a contributor."
+	helpMsg           = `
 	This request has been marked as needing help from a contributor.
 
 Please ensure the request meets the requirements listed [here](` + helpGuidelinesURL + `).
@@ -60,10 +57,17 @@ var (
 	plugin = plugins.Plugin{
 		Description: "The help plugin provides commands that add or remove the '" + labels.Help + "' and the '" + labels.GoodFirstIssue + "' labels from issues.",
 		Commands: []plugins.Command{{
-			GenericCommentHandler: handleGenericComment,
 			Filter: func(e scmprovider.GenericCommentEvent) bool {
 				return !(e.IsPR || e.IssueState != "open" || e.Action != scm.ActionCreate)
 			},
+			Regex: helpRe,
+			GenericCommentHandler: func(match []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
+				cp, err := pc.CommentPruner()
+				if err != nil {
+					return err
+				}
+				return handle(match[1], pc.SCMProviderClient, pc.Logger, cp, &e)
+			},
 			Help: []pluginhelp.Command{{
 				Usage:       "/[remove-](help|good-first-issue)",
 				Description: "Applies or removes the '" + labels.Help + "' and '" + labels.GoodFirstIssue + "' labels to an issue.",
@@ -92,15 +96,7 @@ type commentPruner interface {
 	PruneComments(pr bool, shouldPrune func(*scm.Comment) bool)
 }
 
-func handleGenericComment(match []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
-	cp, err := pc.CommentPruner()
-	if err != nil {
-		return err
-	}
-	return handle(match, pc.SCMProviderClient, pc.Logger, cp, &e)
-}
-
-func handle(_ []string, spc scmProviderClient, log *logrus.Entry, cp commentPruner, e *scmprovider.GenericCommentEvent) error {
+func handle(command string, spc scmProviderClient, log *logrus.Entry, cp commentPruner, e *scmprovider.GenericCommentEvent) error {
 	org := e.Repo.Namespace
 	repo := e.Repo.Name
 	commentAuthor := e.Author.Login
@@ -114,7 +110,7 @@ func handle(_ []string, spc scmProviderClient, log *logrus.Entry, cp commentPrun
 	hasGoodFirstIssue := scmprovider.HasLabel(labels.GoodFirstIssue, issueLabels)
 
 	// If PR has help label and we're asking for it to be removed, remove label
-	if hasHelp && helpRemoveRe.MatchString(e.Body) {
+	if hasHelp && command == "remove-help" {
 		if err := spc.RemoveLabel(org, repo, e.Number, labels.Help, e.IsPR); err != nil {
 			log.WithError(err).Errorf("GitHub failed to remove the following label: %s", labels.Help)
 		}
@@ -138,7 +134,7 @@ func handle(_ []string, spc scmProviderClient, log *logrus.Entry, cp commentPrun
 
 	// If PR does not have the good-first-issue label and we are asking for it to be added,
 	// add both the good-first-issue and help labels
-	if !hasGoodFirstIssue && helpGoodFirstIssueRe.MatchString(e.Body) {
+	if !hasGoodFirstIssue && command == "good-first-issue" {
 		if err := spc.CreateComment(org, repo, e.Number, e.IsPR, plugins.FormatResponseRaw(e.Body, e.IssueLink, spc.QuoteAuthorForComment(commentAuthor), goodFirstIssueMsg)); err != nil {
 			log.WithError(err).Errorf("Failed to create comment \"%s\".", goodFirstIssueMsg)
 		}
@@ -158,7 +154,7 @@ func handle(_ []string, spc scmProviderClient, log *logrus.Entry, cp commentPrun
 
 	// If PR does not have the help label and we're asking it to be added,
 	// add the label
-	if !hasHelp && helpRe.MatchString(e.Body) {
+	if !hasHelp && command == "help" {
 		if err := spc.CreateComment(org, repo, e.Number, e.IsPR, plugins.FormatResponseRaw(e.Body, e.IssueLink, spc.QuoteAuthorForComment(commentAuthor), helpMsg)); err != nil {
 			log.WithError(err).Errorf("Failed to create comment \"%s\".", helpMsg)
 		}
@@ -171,7 +167,7 @@ func handle(_ []string, spc scmProviderClient, log *logrus.Entry, cp commentPrun
 
 	// If PR has good-first-issue label and we are asking for it to be removed,
 	// remove just the good-first-issue label
-	if hasGoodFirstIssue && helpGoodFirstIssueRemoveRe.MatchString(e.Body) {
+	if hasGoodFirstIssue && command == "remove-good-first-issue" {
 		if err := spc.RemoveLabel(org, repo, e.Number, labels.GoodFirstIssue, e.IsPR); err != nil {
 			log.WithError(err).Errorf("GitHub failed to remove the following label: %s", labels.GoodFirstIssue)
 		}
diff --git a/pkg/plugins/plugins.go b/pkg/plugins/plugins.go
index 575b91a1..9b47a39e 100644
--- a/pkg/plugins/plugins.go
+++ b/pkg/plugins/plugins.go
@@ -95,6 +95,21 @@ func (cmd Command) InvokeCommandHandler(ce *scmprovider.GenericCommentEvent, han
 	return nil
 }
 
+// GetMatches returns command matches
+func (cmd Command) GetMatches(ce *scmprovider.GenericCommentEvent) ([][]string, error) {
+	if cmd.Filter != nil && !cmd.Filter(*ce) {
+		return nil, nil
+	}
+	if cmd.Regex != nil {
+		max := cmd.MaxMatches
+		if max == 0 {
+			max = -1
+		}
+		return cmd.Regex.FindAllStringSubmatch(ce.Body, max), nil
+	}
+	return nil, errors.New("regex cannot be nil")
+}
+
 // Plugin defines a plugin and its handlers
 type Plugin struct {
 	Description        string
@@ -213,7 +228,7 @@ type Agent struct {
 	Logger *logrus.Entry
 
 	// may be nil if not initialized
-	commentPruner *commentpruner.EventClient
+	Commentpruner *commentpruner.EventClient
 }
 
 // NewAgent bootstraps a new Agent struct from the passed dependencies.
@@ -246,7 +261,7 @@ func NewAgent(configAgent *config.Agent, pluginConfigAgent *ConfigAgent, clientA
 // InitializeCommentPruner attaches a commentpruner.EventClient to the agent to handle
 // pruning comments.
 func (a *Agent) InitializeCommentPruner(org, repo string, pr int) {
-	a.commentPruner = commentpruner.NewEventClient(
+	a.Commentpruner = commentpruner.NewEventClient(
 		a.SCMProviderClient, a.Logger.WithField("client", "commentpruner"),
 		org, repo, pr,
 	)
@@ -255,10 +270,10 @@ func (a *Agent) InitializeCommentPruner(org, repo string, pr int) {
 // CommentPruner will return the commentpruner.EventClient attached to the agent or an error
 // if one is not attached.
 func (a *Agent) CommentPruner() (*commentpruner.EventClient, error) {
-	if a.commentPruner == nil {
+	if a.Commentpruner == nil {
 		return nil, errors.New("comment pruner client never initialized")
 	}
-	return a.commentPruner, nil
+	return a.Commentpruner, nil
 }
 
 // ClientAgent contains the various clients that are attached to the Agent.


Test patch:
diff --git a/pkg/plugins/help/help_test.go b/pkg/plugins/help/help_test.go
index c8ce61b3..6bf3d8a0 100644
--- a/pkg/plugins/help/help_test.go
+++ b/pkg/plugins/help/help_test.go
@@ -215,11 +215,15 @@ func TestLabel(t *testing.T) {
 				Repo:       scm.Repository{Namespace: "org", Name: "repo"},
 				Author:     scm.User{Login: "Alice"},
 			}
-			err := plugin.InvokeCommand(e, func(match []string) error {
-				return handle(match, fakeSCMProviderClient, logrus.WithField("plugin", pluginName), &fakePruner{}, e)
-			})
+			cmd := plugin.Commands[0]
+			matches, err := cmd.GetMatches(e)
 			if err != nil {
-				t.Fatalf("For case %s, didn't expect error from label test: %v", tc.name, err)
+				t.Fatalf("(%s): Unexpected error from handle: %v.", tc.name, err)
+			}
+			for _, m := range matches {
+				if err := handle(m[1], &fakeSCMProviderClient.Client, logrus.WithField("plugin", pluginName), &fakePruner{}, e); err != nil {
+					t.Fatalf("For case %s, didn't expect error from label test: %v", tc.name, err)
+				}
 			}
 
 			// Check that all the correct labels (and only the correct labels) were added.