Repository: jenkins-x/lighthouse
Pull request #1039 (2020-09-10 21:38:49)

refactor: label plugin
This PR refactors the label 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"]} -->
jenkins-x-bot: @eddycharly: The following test **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
gitlab | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/11) | `/test gitlab`

[View all Builds for this Pull Request](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039) 

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [jenkins-x/lighthouse](https://github.com/jenkins-x/lighthouse/issues/new?title=Command%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
jenkins-x-bot: @eddycharly: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
gitlab | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/11) | `/test gitlab`
github | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/9) | `/test github`
ghe | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/8) | `/test ghe`

[View all Builds for this Pull Request](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039) 

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [jenkins-x/lighthouse](https://github.com/jenkins-x/lighthouse/issues/new?title=Command%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
jenkins-x-bot: @eddycharly: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
gitlab | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/11) | `/test gitlab`
github | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/9) | `/test github`
ghe | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/8) | `/test ghe`
bbs | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/10) | `/test bbs`

[View all Builds for this Pull Request](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039) 

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [jenkins-x/lighthouse](https://github.com/jenkins-x/lighthouse/issues/new?title=Command%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
jenkins-x-bot: @eddycharly: The following tests **failed**, say `/retest` to rerun them all:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
gitlab | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/11) | `/test gitlab`
github | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/9) | `/test github`
ghe | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/8) | `/test ghe`
bbs | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/10) | `/test bbs`
tekton | 71d9b0f3caf036a2140ceb11f89b5ba7ed09378f | [link](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039/12) | `/test tekton`

[View all Builds for this Pull Request](https://dashboard-jx.jenkins-x.live/teams/jx/projects/jenkins-x/lighthouse/PR-1039) 

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [jenkins-x/lighthouse](https://github.com/jenkins-x/lighthouse/issues/new?title=Command%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
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/1039#issuecomment-691075448 "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-691075448/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/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))
 	}
 


Test patch:
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)
 			}