Repository: jenkins-x/lighthouse
Pull request #1031 (2020-09-09 20:57:50)

refactor: dog plugin
This PR refactors the dog plugin.

It adds separate commands for all commands regular expressions `/this-is-fine`, `/this-is-not-fine` and `/this-is-unbearable`.


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

Patch:
diff --git a/pkg/plugins/approve/approve.go b/pkg/plugins/approve/approve.go
index 5c5def04..f17b1f44 100644
--- a/pkg/plugins/approve/approve.go
+++ b/pkg/plugins/approve/approve.go
@@ -40,8 +40,7 @@ import (
 )
 
 const (
-	// PluginName defines this plugin's registered name.
-	PluginName = "approve"
+	pluginName = "approve"
 
 	approveCommand  = "APPROVE"
 	cancelArgument  = "cancel"
@@ -120,7 +119,7 @@ var (
 )
 
 func init() {
-	plugins.RegisterPlugin(PluginName, plugin)
+	plugins.RegisterPlugin(pluginName, plugin)
 }
 
 func configHelp(config *plugins.Configuration, enabledRepos []string) (map[string]string, error) {
diff --git a/pkg/plugins/blockade/blockade.go b/pkg/plugins/blockade/blockade.go
index 83d16742..5100f55d 100644
--- a/pkg/plugins/blockade/blockade.go
+++ b/pkg/plugins/blockade/blockade.go
@@ -36,8 +36,7 @@ import (
 )
 
 const (
-	// PluginName defines this plugin's registered name.
-	PluginName = "blockade"
+	pluginName = "blockade"
 )
 
 var blockedPathsBody = fmt.Sprintf("Adding label: `%s` because PR changes a protected file.", labels.BlockedPaths)
@@ -57,7 +56,7 @@ type pruneClient interface {
 
 func init() {
 	plugins.RegisterPlugin(
-		PluginName,
+		pluginName,
 		plugins.Plugin{
 			Description:        "The blockade plugin blocks pull requests from merging if they touch specific files. The plugin applies the '" + labels.BlockedPaths + "' label to pull requests that touch files that match a blockade's block regular expression and none of the corresponding exception regular expressions.",
 			ConfigHelpProvider: configHelp,
diff --git a/pkg/plugins/dog/dog.go b/pkg/plugins/dog/dog.go
index a480c243..375d9545 100644
--- a/pkg/plugins/dog/dog.go
+++ b/pkg/plugins/dog/dog.go
@@ -49,25 +49,71 @@ const (
 	pluginName    = "dog"
 )
 
-var (
-	plugin = plugins.Plugin{
+func createPlugin(p pack) plugins.Plugin {
+	return plugins.Plugin{
 		Description: "The dog plugin adds a dog image to an issue or PR in response to the `/woof` command.",
 		Commands: []plugins.Command{{
-			GenericCommentHandler: handleGenericComment,
-			Filter:                func(e scmprovider.GenericCommentEvent) bool { return e.Action == scm.ActionCreate },
+			Filter: func(e scmprovider.GenericCommentEvent) bool { return e.Action == scm.ActionCreate },
+			Regex:  match,
+			GenericCommentHandler: func(_ []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
+				return handle(pc.SCMProviderClient, pc.Logger, &e, p)
+			},
+			Help: []pluginhelp.Command{{
+				Usage:       "/(lh-)?(woof|bark)",
+				Description: "Add a dog image to the issue or PR",
+				Featured:    false,
+				WhoCanUse:   "Anyone",
+				Examples:    []string{"/woof", "/bark"},
+			}},
+		}, {
+			Filter: func(e scmprovider.GenericCommentEvent) bool { return e.Action == scm.ActionCreate },
+			Regex:  fineRegex,
+			GenericCommentHandler: func(_ []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
+				return formatURLAndSendResponse(pc.SCMProviderClient, &e, fineURL)
+			},
+			Help: []pluginhelp.Command{{
+				Usage:       "/(lh-)?this-is-fine",
+				Description: "Add a dog image to the issue or PR",
+				Featured:    false,
+				WhoCanUse:   "Anyone",
+				Examples:    []string{"/this-is-fine", "/lh-this-is-fine"},
+			}},
+		}, {
+			Filter: func(e scmprovider.GenericCommentEvent) bool { return e.Action == scm.ActionCreate },
+			Regex:  notFineRegex,
+			GenericCommentHandler: func(_ []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
+				return formatURLAndSendResponse(pc.SCMProviderClient, &e, notFineURL)
+			},
+			Help: []pluginhelp.Command{{
+				Usage:       "/(lh-)?this-is-not-fine",
+				Description: "Add a dog image to the issue or PR",
+				Featured:    false,
+				WhoCanUse:   "Anyone",
+				Examples:    []string{"/this-is-not-fine", "/lh-this-is-not-fine"},
+			}},
+		}, {
+			Filter: func(e scmprovider.GenericCommentEvent) bool { return e.Action == scm.ActionCreate },
+			Regex:  unbearableRegex,
+			GenericCommentHandler: func(_ []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
+				return formatURLAndSendResponse(pc.SCMProviderClient, &e, unbearableURL)
+			},
 			Help: []pluginhelp.Command{{
-				Usage:       "/(lh-)?(woof|bark|this-is-{fine|not-fine|unbearable})",
+				Usage:       "/(lh-)?this-is-unbearable",
 				Description: "Add a dog image to the issue or PR",
 				Featured:    false,
 				WhoCanUse:   "Anyone",
-				Examples:    []string{"/woof", "/bark", "this-is-{fine|not-fine|unbearable}"},
+				Examples:    []string{"/this-is-unbearable", "/lh-this-is-unbearable"},
 			}},
 		}},
 	}
-)
+}
 
 func init() {
-	plugins.RegisterPlugin(pluginName, plugin)
+	plugins.RegisterPlugin(pluginName, createPlugin(dogURL))
+}
+
+type agent interface {
+	ScmProviderClient() *scmprovider.Client
 }
 
 type scmProviderClient interface {
@@ -76,7 +122,7 @@ type scmProviderClient interface {
 }
 
 type pack interface {
-	readDog(dogURL string) (string, error)
+	readDog() (string, error)
 }
 
 type realPack string
@@ -87,8 +133,7 @@ type dogResult struct {
 	URL string `json:"url"`
 }
 
-// FormatURL will return the GH markdown to show the image for a specific dogURL.
-func FormatURL(dogURL string) (string, error) {
+func formatURL(dogURL string) (string, error) {
 	if dogURL == "" {
 		return "", errors.New("empty url")
 	}
@@ -99,75 +144,56 @@ func FormatURL(dogURL string) (string, error) {
 	return fmt.Sprintf("[![dog image](%s)](%s)", src, src), nil
 }
 
-func (u realPack) readDog(dogURL string) (string, error) {
-	if dogURL == "" {
-		uri := string(u)
-		req, err := http.NewRequest("GET", uri, nil)
-		if err != nil {
-			return "", fmt.Errorf("could not create request %s: %v", uri, err)
-		}
-		req.Header.Add("Accept", "application/json")
-		resp, err := client.Do(req)
-		if err != nil {
-			return "", fmt.Errorf("could not read dog from %s: %v", uri, err)
-		}
-		defer resp.Body.Close()
-		var a dogResult
-		if err = json.NewDecoder(resp.Body).Decode(&a); err != nil {
-			return "", err
-		}
-		dogURL = a.URL
+func (u realPack) readDog() (string, error) {
+	uri := string(u)
+	req, err := http.NewRequest("GET", uri, nil)
+	if err != nil {
+		return "", fmt.Errorf("could not create request %s: %v", uri, err)
+	}
+	req.Header.Add("Accept", "application/json")
+	resp, err := client.Do(req)
+	if err != nil {
+		return "", fmt.Errorf("could not read dog from %s: %v", uri, err)
+	}
+	defer resp.Body.Close()
+	var a dogResult
+	if err = json.NewDecoder(resp.Body).Decode(&a); err != nil {
+		return "", err
 	}
-
 	// GitHub doesn't support videos :(
-	if !filetypes.MatchString(dogURL) {
-		return "", errors.New("unsupported doggo :( unknown filetype: " + dogURL)
+	if !filetypes.MatchString(a.URL) {
+		return "", errors.New("unsupported doggo :( unknown filetype: " + a.URL)
 	}
 	// checking size, GitHub doesn't support big images
-	toobig, err := scmprovider.ImageTooBig(dogURL)
+	toobig, err := scmprovider.ImageTooBig(a.URL)
 	if err != nil {
 		return "", err
 	} else if toobig {
-		return "", errors.New("unsupported doggo :( size too big: " + dogURL)
+		return "", errors.New("unsupported doggo :( size too big: " + a.URL)
 	}
-	return FormatURL(dogURL)
-}
-
-func handleGenericComment(match []string, pc plugins.Agent, e scmprovider.GenericCommentEvent) error {
-	return handle(match, pc.SCMProviderClient, pc.Logger, &e, dogURL)
+	return a.URL, nil
 }
 
-func handle(_ []string, spc scmProviderClient, log *logrus.Entry, e *scmprovider.GenericCommentEvent, p pack) error {
-	// Make sure they are requesting a dog
-	mat := match.FindStringSubmatch(e.Body)
-	url := ""
-	if mat == nil {
-		// check is this one of the famous.dog
-		if fineRegex.FindStringSubmatch(e.Body) != nil {
-			url = fineURL
-		} else if notFineRegex.FindStringSubmatch(e.Body) != nil {
-			url = notFineURL
-		} else if unbearableRegex.FindStringSubmatch(e.Body) != nil {
-			url = unbearableURL
-		}
-
-		if url == "" {
-			return nil
-		}
-	}
-
-	org := e.Repo.Namespace
-	repo := e.Repo.Name
-	number := e.Number
-
+func handle(spc scmProviderClient, log *logrus.Entry, e *scmprovider.GenericCommentEvent, p pack) error {
 	for i := 0; i < 5; i++ {
-		resp, err := p.readDog(url)
+		resp, err := p.readDog()
 		if err != nil {
 			log.WithError(err).Println("Failed to get dog img")
 			continue
 		}
-		return spc.CreateComment(org, repo, number, e.IsPR, plugins.FormatResponseRaw(e.Body, e.Link, spc.QuoteAuthorForComment(e.Author.Login), resp))
+		return formatURLAndSendResponse(spc, e, resp)
 	}
-
 	return errors.New("could not find a valid dog image")
 }
+
+func formatURLAndSendResponse(spc scmProviderClient, e *scmprovider.GenericCommentEvent, url string) error {
+	msg, err := formatURL(url)
+	if err != nil {
+		return err
+	}
+	return sendResponse(spc, e, msg)
+}
+
+func sendResponse(spc scmProviderClient, e *scmprovider.GenericCommentEvent, msg string) error {
+	return spc.CreateComment(e.Repo.Namespace, e.Repo.Name, 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 df735a9e..575b91a1 100644
--- a/pkg/plugins/plugins.go
+++ b/pkg/plugins/plugins.go
@@ -53,7 +53,7 @@ type Command struct {
 	GenericCommentHandler GenericCommentHandler
 }
 
-// InvokeHandler performs command checks (filter, then regex if any) the calls the handler wiith the match (if any)
+// InvokeHandler performs command checks (filter, then regex if any) the calls the handler with the match (if any)
 func (cmd Command) InvokeHandler(ce *scmprovider.GenericCommentEvent, handler func([]string) error) error {
 	if cmd.GenericCommentHandler == nil || (cmd.Filter != nil && !cmd.Filter(*ce)) {
 		return nil
@@ -74,6 +74,27 @@ func (cmd Command) InvokeHandler(ce *scmprovider.GenericCommentEvent, handler fu
 	return nil
 }
 
+// InvokeCommandHandler performs command checks (filter, then regex if any) the calls the handler with the match (if any)
+func (cmd Command) InvokeCommandHandler(ce *scmprovider.GenericCommentEvent, handler func(GenericCommentHandler, *scmprovider.GenericCommentEvent, []string) error) error {
+	if cmd.GenericCommentHandler == nil || (cmd.Filter != nil && !cmd.Filter(*ce)) {
+		return nil
+	}
+	if cmd.Regex != nil {
+		max := cmd.MaxMatches
+		if max == 0 {
+			max = -1
+		}
+		for _, m := range cmd.Regex.FindAllStringSubmatch(ce.Body, max) {
+			if err := handler(cmd.GenericCommentHandler, ce, m); err != nil {
+				return err
+			}
+		}
+	} else {
+		return handler(cmd.GenericCommentHandler, ce, nil)
+	}
+	return nil
+}
+
 // Plugin defines a plugin and its handlers
 type Plugin struct {
 	Description        string
@@ -96,6 +117,16 @@ func (plugin Plugin) InvokeCommand(ce *scmprovider.GenericCommentEvent, handler
 	return nil
 }
 
+// InvokeCommandHandler calls InvokeHandler on all commands
+func (plugin Plugin) InvokeCommandHandler(ce *scmprovider.GenericCommentEvent, handler func(GenericCommentHandler, *scmprovider.GenericCommentEvent, []string) error) error {
+	for _, cmd := range plugin.Commands {
+		if err := cmd.InvokeCommandHandler(ce, handler); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // RegisterPlugin registers a plugin.
 func RegisterPlugin(name string, plugin Plugin) {
 	plugins[name] = plugin
diff --git a/pkg/plugins/wip/wip-label.go b/pkg/plugins/wip/wip-label.go
index c9ec7e20..dad6786d 100644
--- a/pkg/plugins/wip/wip-label.go
+++ b/pkg/plugins/wip/wip-label.go
@@ -33,7 +33,6 @@ import (
 )
 
 const (
-	// PluginName defines this plugin's registered name.
 	pluginName = "wip"
 )
 


Test patch:
diff --git a/pkg/plugins/blockade/blockade_test.go b/pkg/plugins/blockade/blockade_test.go
index 73bd56ba..82838894 100644
--- a/pkg/plugins/blockade/blockade_test.go
+++ b/pkg/plugins/blockade/blockade_test.go
@@ -150,7 +150,7 @@ func TestCalculateBlocks(t *testing.T) {
 	}
 
 	for _, tc := range tcs {
-		blockades := compileApplicableBlockades("org", "repo", logrus.WithField("plugin", PluginName), tc.config)
+		blockades := compileApplicableBlockades("org", "repo", logrus.WithField("plugin", pluginName), tc.config)
 		sum := calculateBlocks(tc.changes, blockades)
 		if !reflect.DeepEqual(sum, tc.expectedSummary) {
 			t.Errorf("[%s] Expected summary: %#v, actual summary: %#v.", tc.name, tc.expectedSummary, sum)
@@ -325,7 +325,7 @@ func TestHandle(t *testing.T) {
 				Number: 1,
 			},
 		}
-		if err := handle(fakeSCMProviderClient, logrus.WithField("plugin", PluginName), tc.config, &fakePruner{}, calcF, pre); err != nil {
+		if err := handle(fakeSCMProviderClient, logrus.WithField("plugin", pluginName), tc.config, &fakePruner{}, calcF, pre); err != nil {
 			t.Errorf("[%s] Unexpected error from handle: %v.", tc.name, err)
 			continue
 		}
diff --git a/pkg/plugins/dog/dog_test.go b/pkg/plugins/dog/dog_test.go
index cb3ba7a2..46f26575 100644
--- a/pkg/plugins/dog/dog_test.go
+++ b/pkg/plugins/dog/dog_test.go
@@ -29,6 +29,7 @@ import (
 
 	"github.com/jenkins-x/go-scm/scm"
 	"github.com/jenkins-x/go-scm/scm/driver/fake"
+	"github.com/jenkins-x/lighthouse/pkg/plugins"
 	"github.com/jenkins-x/lighthouse/pkg/scmprovider"
 	"github.com/sirupsen/logrus"
 )
@@ -37,10 +38,7 @@ type fakePack string
 
 var human = flag.Bool("human", false, "Enable to run additional manual tests")
 
-func (c fakePack) readDog(dogURL string) (string, error) {
-	if dogURL != "" {
-		return dogURL, nil
-	}
+func (c fakePack) readDog() (string, error) {
 	return string(c), nil
 }
 
@@ -48,7 +46,7 @@ func TestRealDog(t *testing.T) {
 	if !*human {
 		t.Skip("Real dogs disabled for automation. Manual users can add --human [--category=foo]")
 	}
-	if dog, err := dogURL.readDog(""); err != nil {
+	if dog, err := dogURL.readDog(); err != nil {
 		t.Errorf("Could not read dog from %s: %v", dogURL, err)
 	} else {
 		fmt.Println(dog)
@@ -80,7 +78,7 @@ func TestFormat(t *testing.T) {
 		},
 	}
 	for _, tc := range testcases {
-		ret, err := FormatURL(tc.url)
+		ret, err := formatURL(tc.url)
 		switch {
 		case tc.err:
 			if err == nil {
@@ -96,7 +94,6 @@ func TestFormat(t *testing.T) {
 
 // Medium integration test (depends on ability to open a TCP port)
 func TestHttpResponse(t *testing.T) {
-
 	// create test cases for handling content length of images
 	contentLength := make(map[string]string)
 	contentLength["/dog.jpg"] = "717987"
@@ -206,7 +203,7 @@ func TestHttpResponse(t *testing.T) {
 
 	// run test for each case
 	for _, testcase := range testcases {
-		dog, err := realPack(ts.URL + testcase.path).readDog("")
+		dog, err := realPack(ts.URL + testcase.path).readDog()
 		if testcase.isValid && err != nil {
 			t.Errorf("For case %s, didn't expect error: %v", testcase.name, err)
 		} else if !testcase.isValid && err == nil {
@@ -228,8 +225,13 @@ func TestHttpResponse(t *testing.T) {
 			Number:     5,
 			IssueState: "open",
 		}
-		err = plugin.InvokeCommand(e, func(match []string) error {
-			return handle(match, fakeClient, logrus.WithField("plugin", pluginName), e, realPack(ts.URL))
+		agent := plugins.Agent{
+			SCMProviderClient: &fakeClient.Client,
+			Logger:            logrus.WithField("plugin", pluginName),
+		}
+		plugin := createPlugin(realPack(ts.URL))
+		err = plugin.InvokeCommandHandler(e, func(handler plugins.GenericCommentHandler, e *scmprovider.GenericCommentEvent, match []string) error {
+			return handler(match, agent, *e)
 		})
 		if err != nil {
 			t.Errorf("tc %s: For comment %s, didn't expect error: %v", testcase.name, testcase.comment, err)
@@ -350,8 +352,13 @@ func TestDogs(t *testing.T) {
 				IssueState: tc.state,
 				IsPR:       tc.pr,
 			}
-			err := plugin.InvokeCommand(e, func(match []string) error {
-				return handle(match, fakeClient, logrus.WithField("plugin", pluginName), e, fakePack("doge"))
+			agent := plugins.Agent{
+				SCMProviderClient: &fakeClient.Client,
+				Logger:            logrus.WithField("plugin", pluginName),
+			}
+			plugin := createPlugin(fakePack("http://127.0.0.1"))
+			err := plugin.InvokeCommandHandler(e, func(handler plugins.GenericCommentHandler, e *scmprovider.GenericCommentEvent, match []string) error {
+				return handler(match, agent, *e)
 			})
 			if err != nil {
 				t.Errorf("For case %s, didn't expect error: %v", tc.name, err)