Fix code review comments (jfrog#23)

jfrog-tamirb · Mar 31, 2022 · cdadcbd · cdadcbd
1 parent c32b982
commit cdadcbd
Show file tree

Hide file tree

Showing 17 changed files with 74 additions and 73 deletions.
diff --git a/.github/workflows/frogbot.yml b/.github/workflows/frogbot.yml
@@ -1,7 +1,7 @@
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [labeled, opened]
 jobs:

diff --git a/README.md b/README.md
@@ -32,17 +32,17 @@ For a super quick start, we created [GitHub Actions templates](templates/github-
 #### How does it work?
 
 1. User opens a pull request
-1. If missing, Frogbot creates a label `🐸 frogbot scan pr` in the repository
-1. Maintainer reviewes the pull request and assigns `🐸 frogbot scan pr`
+1. If missing, Frogbot creates a label `🐸 frogbot scan` in the repository
+1. Maintainer reviewes the pull request and assigns `🐸 frogbot scan`
 1. Frogbot gets triggered by the label, unlabels it, and executes the pull request scanning
 
 Here's a recommanded structure of a `frogbot.yml` workflow file:
 
 ```yml
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [opened, labeled]
 jobs:

diff --git a/commands/scanpullrequest.go b/commands/scanpullrequest.go
@@ -24,7 +24,7 @@ func ScanPullRequest(c *clitool.Context) error {
 	}
 
 	if c.Bool("use-labels") {
-		if err = beforeScan(params, client); err != nil {
+		if shouldScan, err := handleFrogbotLabel(params, client); err != nil || !shouldScan {
 			return err
 		}
 	}
@@ -36,7 +36,7 @@ func GetScanPullRequestFlags() []clitool.Flag {
 	return []clitool.Flag{
 		&clitool.BoolFlag{
 			Name:    "use-labels",
-			Usage:   "Set to true if scan-pull-request is triggered by adding '🐸 frogbot scan pr' label to a pull request.",
+			Usage:   "Set to true if scan-pull-request is triggered by adding '🐸 frogbot scan' label to a pull request.",
 			EnvVars: []string{"JF_USE_LABELS"},
 		},
 	}
@@ -48,10 +48,10 @@ func GetScanPullRequestFlags() []clitool.Flag {
 // If pr is labeled - remove label and allow running Xray scan (return nil)
 // params - Frogbot parameters retreived from the environment variables
 // client - The VCS client
-func beforeScan(params *utils.FrogbotParams, client vcsclient.VcsClient) error {
+func handleFrogbotLabel(params *utils.FrogbotParams, client vcsclient.VcsClient) (bool, error) {
 	labelInfo, err := client.GetLabel(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName))
 	if err != nil {
-		return err
+		return false, err
 	}
 	if labelInfo == nil {
 		clientLog.Info("Creating label " + string(utils.LabelName))
@@ -61,29 +61,32 @@ func beforeScan(params *utils.FrogbotParams, client vcsclient.VcsClient) error {
 			Color:       string(utils.LabelColor),
 		})
 		if err != nil {
-			return err
+			return false, err
 		}
-		return utils.ErrLabelCreated
+		clientLog.Info(fmt.Sprintf("label '%s' was created. Please label this pull request to trigger an Xray scan", string(utils.LabelName)))
+		return false, nil
 	}
 
 	labels, err := client.ListPullRequestLabels(context.Background(), params.RepoOwner, params.Repo, params.PullRequestID)
 	if err != nil {
-		return err
+		return false, err
 	}
-	clientLog.Debug("The following labels found in the pull request: ", labels)
+	clientLog.Debug("The following labels were found in the pull request: ", labels)
 	for _, label := range labels {
-		if label == string(utils.LabelName) {
-			clientLog.Info("Unlabeling '"+utils.LabelName+"' from pull request", params.PullRequestID)
-			err = client.UnlabelPullRequest(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName), params.PullRequestID)
-			// Trigger scan or return err
-			return err
+		if label != string(utils.LabelName) {
+			continue
 		}
+		clientLog.Info("Unlabeling '"+utils.LabelName+"' from pull request", params.PullRequestID)
+		err := client.UnlabelPullRequest(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName), params.PullRequestID)
+		return err == nil, err
 	}
-	return utils.ErrUnlabel
+	clientLog.Info(fmt.Sprintf("please add the '%s' label to trigger an Xray scan", string(utils.LabelName)))
+	return false, nil
 }
 
-// Scan a pull request by auditing the source and the target branches.
-// If errors were added in the source branch, print them in a comment.
+// Scan a pull request by as follows:
+// a. Audit the depedencies of the source and the target branches.
+// b. Compare the vulenrabilities found in source and target branches, and show only the new vulnerabilities added by the pull request.
 func scanPullRequest(params *utils.FrogbotParams, client vcsclient.VcsClient) error {
 	// Audit PR code
 	xrayScanParams := createXrayScanParams(params.Watches, params.Project)
@@ -249,7 +252,7 @@ func createPullRequestMessage(vulnerabilitiesRows []xrayutils.VulnerabilityRow)
 		if len(vulnerability.Cves) > 0 {
 			cve = vulnerability.Cves[0].Id
 		}
-		tableContent += fmt.Sprintf("\n| %s | %s | %s | %s | %s | %s | %s ", utils.GetSeverityTag(vulnerability.Severity)+" "+vulnerability.Severity, vulnerability.ImpactedPackageName,
+		tableContent += fmt.Sprintf("\n| %s | %s | %s | %s | %s | %s | %s ", utils.GetSeverityTag(utils.IconName(vulnerability.Severity))+" "+vulnerability.Severity, vulnerability.ImpactedPackageName,
 			vulnerability.ImpactedPackageVersion, vulnerability.FixedVersions, componentName, componentVersion, cve)
 	}
 	return utils.GetBanner(utils.VulnerabilitiesBannerSource) + utils.WhatIsFrogbotMd + utils.TableHeder + tableContent

diff --git a/commands/scanpullrequest_test.go b/commands/scanpullrequest_test.go
@@ -270,7 +270,7 @@ var params = &utils.FrogbotParams{
 	},
 }
 
-func TestBeforeScan(t *testing.T) {
+func TestHandleFrogbotLabel(t *testing.T) {
 	// Init mock
 	client, finish := mockVcsClient(t)
 	defer finish()
@@ -284,12 +284,13 @@ func TestBeforeScan(t *testing.T) {
 	client.EXPECT().ListPullRequestLabels(context.Background(), params.RepoOwner, params.Repo, params.PullRequestID).Return([]string{string(utils.LabelName)}, nil)
 	client.EXPECT().UnlabelPullRequest(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName), 5).Return(nil)
 
-	// Run beforeScan
-	err := beforeScan(params, client)
+	// Run handleFrogbotLabel
+	shouldScan, err := handleFrogbotLabel(params, client)
 	assert.NoError(t, err)
+	assert.True(t, shouldScan)
 }
 
-func TestBeforeScanGetLabelErr(t *testing.T) {
+func TestHandleFrogbotLabelGetLabelErr(t *testing.T) {
 	// Init mock
 	client, finish := mockVcsClient(t)
 	defer finish()
@@ -298,12 +299,13 @@ func TestBeforeScanGetLabelErr(t *testing.T) {
 	expectedError := errors.New("Couldn't get label")
 	client.EXPECT().GetLabel(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName)).Return(&vcsclient.LabelInfo{}, expectedError)
 
-	// Run beforeScan
-	err := beforeScan(params, client)
+	// Run handleFrogbotLabel
+	shouldScan, err := handleFrogbotLabel(params, client)
 	assert.ErrorIs(t, err, expectedError)
+	assert.False(t, shouldScan)
 }
 
-func TestBeforeScanLabelNotExist(t *testing.T) {
+func TestHandleFrogbotLabelLabelNotExist(t *testing.T) {
 	// Init mock
 	client, finish := mockVcsClient(t)
 	defer finish()
@@ -316,12 +318,13 @@ func TestBeforeScanLabelNotExist(t *testing.T) {
 		Color:       string(utils.LabelColor),
 	}).Return(nil)
 
-	// Run beforeScan
-	err := beforeScan(params, client)
-	assert.ErrorIs(t, err, utils.ErrLabelCreated)
+	// Run handleFrogbotLabel
+	shouldScan, err := handleFrogbotLabel(params, client)
+	assert.NoError(t, err)
+	assert.False(t, shouldScan)
 }
 
-func TestBeforeScanCreateLabelErr(t *testing.T) {
+func TestHandleFrogbotLabelCreateLabelErr(t *testing.T) {
 	// Init mock
 	client, finish := mockVcsClient(t)
 	defer finish()
@@ -335,12 +338,13 @@ func TestBeforeScanCreateLabelErr(t *testing.T) {
 		Color:       string(utils.LabelColor),
 	}).Return(expectedError)
 
-	// Run beforeScan
-	err := beforeScan(params, client)
+	// Run handleFrogbotLabel
+	shouldScan, err := handleFrogbotLabel(params, client)
 	assert.ErrorIs(t, err, expectedError)
+	assert.False(t, shouldScan)
 }
 
-func TestBeforeScanUnlabeled(t *testing.T) {
+func TestHandleFrogbotLabelUnlabeled(t *testing.T) {
 	// Init mock
 	client, finish := mockVcsClient(t)
 	defer finish()
@@ -353,12 +357,13 @@ func TestBeforeScanUnlabeled(t *testing.T) {
 	}, nil)
 	client.EXPECT().ListPullRequestLabels(context.Background(), params.RepoOwner, params.Repo, params.PullRequestID).Return([]string{}, nil)
 
-	// Run beforeScan
-	err := beforeScan(params, client)
-	assert.ErrorIs(t, err, utils.ErrUnlabel)
+	// Run handleFrogbotLabel
+	shouldScan, err := handleFrogbotLabel(params, client)
+	assert.NoError(t, err)
+	assert.False(t, shouldScan)
 }
 
-func TestBeforeScanCreateListLabelsErr(t *testing.T) {
+func TestHandleFrogbotLabelCreateListLabelsErr(t *testing.T) {
 	// Init mock
 	client, finish := mockVcsClient(t)
 	defer finish()
@@ -377,9 +382,10 @@ func TestBeforeScanCreateListLabelsErr(t *testing.T) {
 	}).Return(nil)
 	client.EXPECT().ListPullRequestLabels(context.Background(), params.RepoOwner, params.Repo, params.PullRequestID).Return([]string{}, expectedError)
 
-	// Run beforeScan
-	err := beforeScan(params, client)
+	// Run handleFrogbotLabel
+	shouldScan, err := handleFrogbotLabel(params, client)
 	assert.ErrorIs(t, err, expectedError)
+	assert.False(t, shouldScan)
 }
 
 func mockVcsClient(t *testing.T) (*testdata.MockVcsClient, func()) {

diff --git a/commands/utils/consts.go b/commands/utils/consts.go
@@ -20,7 +20,7 @@ const (
 	gitLab vcsProvider = "gitlab"
 
 	// Frogbot label
-	LabelName        frogbotLabel = "🐸 frogbot scan pr"
+	LabelName        frogbotLabel = "🐸 frogbot scan"
 	LabelDescription frogbotLabel = "triggers frogbot scan"
 	LabelColor       frogbotLabel = "4AB548"
 

diff --git a/commands/utils/icons.go b/commands/utils/icons.go
@@ -5,8 +5,10 @@ import (
 	"strings"
 )
 
-func GetSeverityTag(iconName string) string {
-	switch strings.ToLower(iconName) {
+type IconName string
+
+func GetSeverityTag(iconName IconName) string {
+	switch strings.ToLower(string(iconName)) {
 	case "critical":
 		return getIconTag(criticalSeveritySource)
 	case "high":

diff --git a/commands/utils/params.go b/commands/utils/params.go
@@ -23,6 +23,7 @@ type JFrogEnvParams struct {
 	Project string
 	Watches string
 }
+
 type GitParam struct {
 	GitProvider   vcsutils.VcsProvider
 	RepoOwner     string

diff --git a/commands/utils/utils.go b/commands/utils/utils.go
@@ -5,9 +5,6 @@ import (
 	"os"
 )
 
-var ErrLabelCreated = fmt.Errorf("label '%s' was created. Please label this pull request to trigger an Xray scan", string(LabelName))
-var ErrUnlabel = fmt.Errorf("please add the '%s' label to trigger an Xray scan", string(LabelName))
-
 type errMissingEnv struct {
 	variableName string
 }

diff --git a/main.go b/main.go
@@ -1,11 +1,9 @@
 package main
 
 import (
-	"errors"
 	"os"
 
 	"github.com/jfrog/frogbot/commands"
-	"github.com/jfrog/frogbot/commands/utils"
 	"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
 	"github.com/jfrog/jfrog-cli-core/v2/utils/log"
 	"github.com/jfrog/jfrog-client-go/utils/io/fileutils"
@@ -30,12 +28,6 @@ func execMain() error {
 		Usage:    "See https://github.com/jfrog/frogbot for usage instructions.",
 		Commands: getCommands(),
 		Version:  frogbotVersion,
-		ExitErrHandler: func(context *clitool.Context, err error) {
-			if errors.Is(err, utils.ErrLabelCreated) || errors.Is(err, utils.ErrUnlabel) {
-				clientLog.Info("Scan wasn't triggered: " + err.Error())
-				os.Exit(0)
-			}
-		},
 	}
 
 	err := app.Run(os.Args)

diff --git a/templates/github-actions/frogbot-dotnet.yml b/templates/github-actions/frogbot-dotnet.yml
@@ -1,7 +1,7 @@
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [labeled, opened]
 jobs:

diff --git a/templates/github-actions/frogbot-go.yml b/templates/github-actions/frogbot-go.yml
@@ -1,7 +1,7 @@
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [labeled, opened]
 jobs:

diff --git a/templates/github-actions/frogbot-gradle.yml b/templates/github-actions/frogbot-gradle.yml
@@ -1,7 +1,7 @@
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [labeled, opened]
 jobs:

diff --git a/templates/github-actions/frogbot-maven.yml b/templates/github-actions/frogbot-maven.yml
@@ -1,7 +1,7 @@
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [labeled, opened]
 jobs:

diff --git a/templates/github-actions/frogbot-npm.yml b/templates/github-actions/frogbot-npm.yml
@@ -1,7 +1,7 @@
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [labeled, opened]
 jobs:

diff --git a/templates/github-actions/frogbot-nuget.yml b/templates/github-actions/frogbot-nuget.yml
@@ -1,7 +1,7 @@
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [labeled, opened]
 jobs:

diff --git a/templates/github-actions/frogbot-pip.yml b/templates/github-actions/frogbot-pip.yml
@@ -1,7 +1,7 @@
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [labeled, opened]
 jobs:

diff --git a/templates/github-actions/frogbot-pipenv.yml b/templates/github-actions/frogbot-pipenv.yml
@@ -1,7 +1,7 @@
 name: "Frogbot"
 on:
-  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
-  # After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
+  # After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
+  # After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
   pull_request_target:
     types: [labeled, opened]
 jobs: