Repository: jenkins-x/lighthouse
Pull request #1042 (2020-09-11 09:25:01)

refactor: help plugin
This PR refactors the help 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"]} -->
abayer: /lgtm
jenkins-x-bot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *[abayer](https://github.com/jenkins-x/lighthouse/pull/1042#issuecomment-691075524 "LGTM")*

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

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

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

- ~~[OWNERS](https://github.com#issuecomment-691075524/jenkins-x/lighthouse/blob/master/OWNERS)~~ [abayer]

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":[]} -->

Patch:
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/label/label.go b/pkg/plugins/label/label.go
index f34539e6..ab242fdd 100644
--- a/pkg/plugins/label/label.go
+++ b/pkg/plugins/label/label.go
@@ -33,10 +33,8 @@ const pluginName = "label"
 
 var (
 	defaultLabels           = []string{"kind", "priority", "area"}
-	labelRegex              = regexp.MustCompile(`(?m)^/(?:lh-)?(area|committee|kind|language|priority|sig|triage|wg)\s*(.*)$`)
-	removeLabelRegex        = regexp.MustCompile(`(?m)^/(?:lh-)?remove-(area|committee|kind|language|priority|sig|triage|wg)\s*(.*)$`)
-	customLabelRegex        = regexp.MustCompile(`(?m)^/(?:lh-)?label\s*(.*)$`)
-	customRemoveLabelRegex  = regexp.MustCompile(`(?m)^/(?:lh-)?remove-label\s*(.*)$`)
+	labelRegex              = regexp.MustCompile(`(?m)^/(?:lh-)?(area|committee|kind|language|priority|sig|triage|wg|label)\s*(.*)$`)
+	removeLabelRegex        = regexp.MustCompile(`(?m)^/(?:lh-)?remove-(area|committee|kind|language|priority|sig|triage|wg|label)\s*(.*)$`)
 	nonExistentLabelOnIssue = "Those labels are not set on the issue: `%v`"
 )
 
@@ -45,10 +43,27 @@ var (
 		Description:        "The label plugin provides commands that add or remove certain types of labels. Labels of the following types can be manipulated: 'area/*', 'committee/*', 'kind/*', 'language/*', 'priority/*', 'sig/*', 'triage/*', and 'wg/*'. More labels can be configured to be used via the /label command.",
 		ConfigHelpProvider: configHelp,
 		Commands: []plugins.Command{{
-			GenericCommentHandler: handleGenericComment,
+			Filter: func(e scmprovider.GenericCommentEvent) bool { return e.Action == scm.ActionCreate },
+			Regex:  labelRegex,
+			GenericCommentHandler: func(match []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
+				return handle(false, match[1], match[2], pc.SCMProviderClient, pc.Logger, pc.PluginConfig.Label.AdditionalLabels, &e)
+			},
 			Help: []pluginhelp.Command{{
-				Usage:       "/[remove-](area|committee|kind|language|priority|sig|triage|wg|label) <target>",
-				Description: "Applies or removes a label from one of the recognized types of labels.",
+				Usage:       "/(area|committee|kind|language|priority|sig|triage|wg|label) <target>",
+				Description: "Applies a label from one of the recognized types of labels.",
+				Featured:    false,
+				WhoCanUse:   "Anyone can trigger this command on a PR.",
+				Examples:    []string{"/kind bug", "/remove-area prow", "/sig testing", "/language zh"},
+			}},
+		}, {
+			Filter: func(e scmprovider.GenericCommentEvent) bool { return e.Action == scm.ActionCreate },
+			Regex:  removeLabelRegex,
+			GenericCommentHandler: func(match []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
+				return handle(true, match[1], match[2], pc.SCMProviderClient, pc.Logger, pc.PluginConfig.Label.AdditionalLabels, &e)
+			},
+			Help: []pluginhelp.Command{{
+				Usage:       "/remove-(area|committee|kind|language|priority|sig|triage|wg|label) <target>",
+				Description: "Removes a label from one of the recognized types of labels.",
 				Featured:    false,
 				WhoCanUse:   "Anyone can trigger this command on a PR.",
 				Examples:    []string{"/kind bug", "/remove-area prow", "/sig testing", "/language zh"},
@@ -79,10 +94,6 @@ func configHelp(config *plugins.Configuration, enabledRepos []string) (map[strin
 		nil
 }
 
-func handleGenericComment(_ []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
-	return handle(pc.SCMProviderClient, pc.Logger, pc.PluginConfig.Label.AdditionalLabels, &e)
-}
-
 type scmProviderClient interface {
 	CreateComment(owner, repo string, number int, pr bool, comment string) error
 	AddLabel(owner, repo string, number int, label string, pr bool) error
@@ -93,46 +104,28 @@ type scmProviderClient interface {
 }
 
 // Get Labels from Regexp matches
-func getLabelsFromREMatches(matches [][]string) (labels []string) {
-	for _, match := range matches {
-		for _, label := range strings.Split(match[0], " ")[1:] {
-			label = strings.ToLower(match[1] + "/" + strings.TrimSpace(label))
-			labels = append(labels, label)
-		}
+func getLabelsFromREMatches(kind string, target string) []string {
+	var labels []string
+	for _, label := range strings.Split(target, " ") {
+		label = strings.ToLower(kind + "/" + strings.TrimSpace(label))
+		labels = append(labels, label)
 	}
-	return
+	return labels
 }
 
 // getLabelsFromGenericMatches returns label matches with extra labels if those
 // have been configured in the plugin config.
-func getLabelsFromGenericMatches(matches [][]string, additionalLabels []string) []string {
-	if len(additionalLabels) == 0 {
-		return nil
-	}
+func getLabelsFromGenericMatches(label string, additionalLabels []string) []string {
 	var labels []string
-	for _, match := range matches {
-		parts := strings.Split(match[0], " ")
-		if ((parts[0] != "/label") && (parts[0] != "/remove-label") && (parts[0] != "/lh-label") && (parts[0] != "/lh-remove-label")) || len(parts) != 2 {
-			continue
-		}
-		for _, l := range additionalLabels {
-			if l == parts[1] {
-				labels = append(labels, parts[1])
-			}
+	for _, l := range additionalLabels {
+		if l == label {
+			labels = append(labels, label)
 		}
 	}
 	return labels
 }
 
-func handle(spc scmProviderClient, log *logrus.Entry, additionalLabels []string, e *scmprovider.GenericCommentEvent) error {
-	labelMatches := labelRegex.FindAllStringSubmatch(e.Body, -1)
-	removeLabelMatches := removeLabelRegex.FindAllStringSubmatch(e.Body, -1)
-	customLabelMatches := customLabelRegex.FindAllStringSubmatch(e.Body, -1)
-	customRemoveLabelMatches := customRemoveLabelRegex.FindAllStringSubmatch(e.Body, -1)
-	if len(labelMatches) == 0 && len(removeLabelMatches) == 0 && len(customLabelMatches) == 0 && len(customRemoveLabelMatches) == 0 {
-		return nil
-	}
-
+func handle(remove bool, kind string, target string, spc scmProviderClient, log *logrus.Entry, additionalLabels []string, e *scmprovider.GenericCommentEvent) error {
 	org := e.Repo.Namespace
 	repo := e.Repo.Name
 
@@ -152,44 +145,39 @@ func handle(spc scmProviderClient, log *logrus.Entry, additionalLabels []string,
 	var (
 		nonexistent         []string
 		noSuchLabelsOnIssue []string
-		labelsToAdd         []string
-		labelsToRemove      []string
 	)
 
 	// Get labels to add and labels to remove from regexp matches
-	labelsToAdd = append(getLabelsFromREMatches(labelMatches), getLabelsFromGenericMatches(customLabelMatches, additionalLabels)...)
-	labelsToRemove = append(getLabelsFromREMatches(removeLabelMatches), getLabelsFromGenericMatches(customRemoveLabelMatches, additionalLabels)...)
-
-	// Add labels
-	for _, labelToAdd := range labelsToAdd {
-		if scmprovider.HasLabel(labelToAdd, labels) {
-			continue
-		}
-
-		if _, ok := RepoLabelsExisting[labelToAdd]; !ok {
-			nonexistent = append(nonexistent, labelToAdd)
-			continue
-		}
-
-		if err := spc.AddLabel(org, repo, e.Number, RepoLabelsExisting[labelToAdd], e.IsPR); err != nil {
-			log.WithError(err).Errorf("GitHub failed to add the following label: %s", labelToAdd)
-		}
+	var lbls []string
+	if kind == "label" {
+		lbls = append(lbls, getLabelsFromGenericMatches(target, additionalLabels)...)
+	} else {
+		lbls = append(lbls, getLabelsFromREMatches(kind, target)...)
 	}
 
-	// Remove labels
-	for _, labelToRemove := range labelsToRemove {
-		if !scmprovider.HasLabel(labelToRemove, labels) {
-			noSuchLabelsOnIssue = append(noSuchLabelsOnIssue, labelToRemove)
-			continue
-		}
-
-		if _, ok := RepoLabelsExisting[labelToRemove]; !ok {
-			nonexistent = append(nonexistent, labelToRemove)
-			continue
-		}
-
-		if err := spc.RemoveLabel(org, repo, e.Number, labelToRemove, e.IsPR); err != nil {
-			log.WithError(err).Errorf("GitHub failed to remove the following label: %s", labelToRemove)
+	for _, lbl := range lbls {
+		if remove {
+			if !scmprovider.HasLabel(lbl, labels) {
+				noSuchLabelsOnIssue = append(noSuchLabelsOnIssue, lbl)
+			} else {
+				if _, ok := RepoLabelsExisting[lbl]; !ok {
+					nonexistent = append(nonexistent, lbl)
+				} else {
+					if err := spc.RemoveLabel(org, repo, e.Number, lbl, e.IsPR); err != nil {
+						log.WithError(err).Errorf("Failed to remove the following label: %s", lbl)
+					}
+				}
+			}
+		} else {
+			if !scmprovider.HasLabel(lbl, labels) {
+				if _, ok := RepoLabelsExisting[lbl]; !ok {
+					nonexistent = append(nonexistent, lbl)
+				} else {
+					if err := spc.AddLabel(org, repo, e.Number, RepoLabelsExisting[lbl], e.IsPR); err != nil {
+						log.WithError(err).Errorf("GitHub failed to add the following label: %s", lbl)
+					}
+				}
+			}
 		}
 	}
 
@@ -201,6 +189,7 @@ func handle(spc scmProviderClient, log *logrus.Entry, additionalLabels []string,
 	// Tried to remove Labels that were not present on the Issue
 	if len(noSuchLabelsOnIssue) > 0 {
 		msg := fmt.Sprintf(nonExistentLabelOnIssue, strings.Join(noSuchLabelsOnIssue, ", "))
+		log.Info(msg)
 		return spc.CreateComment(org, repo, e.Number, e.IsPR, plugins.FormatResponseRaw(e.Body, e.Link, spc.QuoteAuthorForComment(e.Author.Login), msg))
 	}
 
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.
diff --git a/pkg/plugins/label/label_test.go b/pkg/plugins/label/label_test.go
index 44e5c582..cb2d9135 100644
--- a/pkg/plugins/label/label_test.go
+++ b/pkg/plugins/label/label_test.go
@@ -358,7 +358,6 @@ func TestLabel(t *testing.T) {
 			expectedNewLabels:     []string{},
 			expectedRemovedLabels: formatLabels("priority/low", "priority/high", "kind/api-server", "area/infra"),
 			commenter:             orgMember,
-			expectedBotComment:    true,
 		},
 		{
 			name:                  "Add and Remove Label at the same time",
@@ -488,7 +487,18 @@ func TestLabel(t *testing.T) {
 				Repo:   scm.Repository{Namespace: "org", Name: "repo"},
 				Author: scm.User{Login: tc.commenter},
 			}
-			err := handle(fakeClient, logrus.WithField("plugin", pluginName), tc.extraLabels, e)
+			agent := plugins.Agent{
+				SCMProviderClient: &fakeClient.Client,
+				Logger:            logrus.WithField("plugin", pluginName),
+				PluginConfig: &plugins.Configuration{
+					Label: plugins.Label{
+						AdditionalLabels: tc.extraLabels,
+					},
+				},
+			}
+			err := plugin.InvokeCommandHandler(e, func(handler plugins.GenericCommentHandler, e *scmprovider.GenericCommentEvent, match []string) error {
+				return handler(match, agent, *e)
+			})
 			if err != nil {
 				t.Fatalf("didn't expect error from label test: %v", err)
 			}