Skip to content

Commit cdadcbd

Browse files
yahaviyahavi
authored
Fix code review comments (#23)
1 parent c32b982 commit cdadcbd

17 files changed

+74
-73
lines changed

Diff for: ‎.github/workflows/frogbot.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: "Frogbot"
22
on:
3-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
4-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
3+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
4+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
55
pull_request_target:
66
types: [labeled, opened]
77
jobs:

Diff for: ‎README.md

+4-4
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,17 @@ For a super quick start, we created [GitHub Actions templates](templates/github-
3232
#### How does it work?
3333

3434
1. User opens a pull request
35-
1. If missing, Frogbot creates a label `🐸 frogbot scan pr` in the repository
36-
1. Maintainer reviewes the pull request and assigns `🐸 frogbot scan pr`
35+
1. If missing, Frogbot creates a label `🐸 frogbot scan` in the repository
36+
1. Maintainer reviewes the pull request and assigns `🐸 frogbot scan`
3737
1. Frogbot gets triggered by the label, unlabels it, and executes the pull request scanning
3838

3939
Here's a recommanded structure of a `frogbot.yml` workflow file:
4040

4141
```yml
4242
name: "Frogbot"
4343
on:
44-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
45-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
44+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
45+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
4646
pull_request_target:
4747
types: [opened, labeled]
4848
jobs:

Diff for: ‎commands/scanpullrequest.go

+20-17
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ func ScanPullRequest(c *clitool.Context) error {
2424
}
2525

2626
if c.Bool("use-labels") {
27-
if err = beforeScan(params, client); err != nil {
27+
if shouldScan, err := handleFrogbotLabel(params, client); err != nil || !shouldScan {
2828
return err
2929
}
3030
}
@@ -36,7 +36,7 @@ func GetScanPullRequestFlags() []clitool.Flag {
3636
return []clitool.Flag{
3737
&clitool.BoolFlag{
3838
Name: "use-labels",
39-
Usage: "Set to true if scan-pull-request is triggered by adding '🐸 frogbot scan pr' label to a pull request.",
39+
Usage: "Set to true if scan-pull-request is triggered by adding '🐸 frogbot scan' label to a pull request.",
4040
EnvVars: []string{"JF_USE_LABELS"},
4141
},
4242
}
@@ -48,10 +48,10 @@ func GetScanPullRequestFlags() []clitool.Flag {
4848
// If pr is labeled - remove label and allow running Xray scan (return nil)
4949
// params - Frogbot parameters retreived from the environment variables
5050
// client - The VCS client
51-
func beforeScan(params *utils.FrogbotParams, client vcsclient.VcsClient) error {
51+
func handleFrogbotLabel(params *utils.FrogbotParams, client vcsclient.VcsClient) (bool, error) {
5252
labelInfo, err := client.GetLabel(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName))
5353
if err != nil {
54-
return err
54+
return false, err
5555
}
5656
if labelInfo == nil {
5757
clientLog.Info("Creating label " + string(utils.LabelName))
@@ -61,29 +61,32 @@ func beforeScan(params *utils.FrogbotParams, client vcsclient.VcsClient) error {
6161
Color: string(utils.LabelColor),
6262
})
6363
if err != nil {
64-
return err
64+
return false, err
6565
}
66-
return utils.ErrLabelCreated
66+
clientLog.Info(fmt.Sprintf("label '%s' was created. Please label this pull request to trigger an Xray scan", string(utils.LabelName)))
67+
return false, nil
6768
}
6869

6970
labels, err := client.ListPullRequestLabels(context.Background(), params.RepoOwner, params.Repo, params.PullRequestID)
7071
if err != nil {
71-
return err
72+
return false, err
7273
}
73-
clientLog.Debug("The following labels found in the pull request: ", labels)
74+
clientLog.Debug("The following labels were found in the pull request: ", labels)
7475
for _, label := range labels {
75-
if label == string(utils.LabelName) {
76-
clientLog.Info("Unlabeling '"+utils.LabelName+"' from pull request", params.PullRequestID)
77-
err = client.UnlabelPullRequest(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName), params.PullRequestID)
78-
// Trigger scan or return err
79-
return err
76+
if label != string(utils.LabelName) {
77+
continue
8078
}
79+
clientLog.Info("Unlabeling '"+utils.LabelName+"' from pull request", params.PullRequestID)
80+
err := client.UnlabelPullRequest(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName), params.PullRequestID)
81+
return err == nil, err
8182
}
82-
return utils.ErrUnlabel
83+
clientLog.Info(fmt.Sprintf("please add the '%s' label to trigger an Xray scan", string(utils.LabelName)))
84+
return false, nil
8385
}
8486

85-
// Scan a pull request by auditing the source and the target branches.
86-
// If errors were added in the source branch, print them in a comment.
87+
// Scan a pull request by as follows:
88+
// a. Audit the depedencies of the source and the target branches.
89+
// b. Compare the vulenrabilities found in source and target branches, and show only the new vulnerabilities added by the pull request.
8790
func scanPullRequest(params *utils.FrogbotParams, client vcsclient.VcsClient) error {
8891
// Audit PR code
8992
xrayScanParams := createXrayScanParams(params.Watches, params.Project)
@@ -249,7 +252,7 @@ func createPullRequestMessage(vulnerabilitiesRows []xrayutils.VulnerabilityRow)
249252
if len(vulnerability.Cves) > 0 {
250253
cve = vulnerability.Cves[0].Id
251254
}
252-
tableContent += fmt.Sprintf("\n| %s | %s | %s | %s | %s | %s | %s ", utils.GetSeverityTag(vulnerability.Severity)+" "+vulnerability.Severity, vulnerability.ImpactedPackageName,
255+
tableContent += fmt.Sprintf("\n| %s | %s | %s | %s | %s | %s | %s ", utils.GetSeverityTag(utils.IconName(vulnerability.Severity))+" "+vulnerability.Severity, vulnerability.ImpactedPackageName,
253256
vulnerability.ImpactedPackageVersion, vulnerability.FixedVersions, componentName, componentVersion, cve)
254257
}
255258
return utils.GetBanner(utils.VulnerabilitiesBannerSource) + utils.WhatIsFrogbotMd + utils.TableHeder + tableContent

Diff for: ‎commands/scanpullrequest_test.go

+26-20
Original file line numberDiff line numberDiff line change
@@ -270,7 +270,7 @@ var params = &utils.FrogbotParams{
270270
},
271271
}
272272

273-
func TestBeforeScan(t *testing.T) {
273+
func TestHandleFrogbotLabel(t *testing.T) {
274274
// Init mock
275275
client, finish := mockVcsClient(t)
276276
defer finish()
@@ -284,12 +284,13 @@ func TestBeforeScan(t *testing.T) {
284284
client.EXPECT().ListPullRequestLabels(context.Background(), params.RepoOwner, params.Repo, params.PullRequestID).Return([]string{string(utils.LabelName)}, nil)
285285
client.EXPECT().UnlabelPullRequest(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName), 5).Return(nil)
286286

287-
// Run beforeScan
288-
err := beforeScan(params, client)
287+
// Run handleFrogbotLabel
288+
shouldScan, err := handleFrogbotLabel(params, client)
289289
assert.NoError(t, err)
290+
assert.True(t, shouldScan)
290291
}
291292

292-
func TestBeforeScanGetLabelErr(t *testing.T) {
293+
func TestHandleFrogbotLabelGetLabelErr(t *testing.T) {
293294
// Init mock
294295
client, finish := mockVcsClient(t)
295296
defer finish()
@@ -298,12 +299,13 @@ func TestBeforeScanGetLabelErr(t *testing.T) {
298299
expectedError := errors.New("Couldn't get label")
299300
client.EXPECT().GetLabel(context.Background(), params.RepoOwner, params.Repo, string(utils.LabelName)).Return(&vcsclient.LabelInfo{}, expectedError)
300301

301-
// Run beforeScan
302-
err := beforeScan(params, client)
302+
// Run handleFrogbotLabel
303+
shouldScan, err := handleFrogbotLabel(params, client)
303304
assert.ErrorIs(t, err, expectedError)
305+
assert.False(t, shouldScan)
304306
}
305307

306-
func TestBeforeScanLabelNotExist(t *testing.T) {
308+
func TestHandleFrogbotLabelLabelNotExist(t *testing.T) {
307309
// Init mock
308310
client, finish := mockVcsClient(t)
309311
defer finish()
@@ -316,12 +318,13 @@ func TestBeforeScanLabelNotExist(t *testing.T) {
316318
Color: string(utils.LabelColor),
317319
}).Return(nil)
318320

319-
// Run beforeScan
320-
err := beforeScan(params, client)
321-
assert.ErrorIs(t, err, utils.ErrLabelCreated)
321+
// Run handleFrogbotLabel
322+
shouldScan, err := handleFrogbotLabel(params, client)
323+
assert.NoError(t, err)
324+
assert.False(t, shouldScan)
322325
}
323326

324-
func TestBeforeScanCreateLabelErr(t *testing.T) {
327+
func TestHandleFrogbotLabelCreateLabelErr(t *testing.T) {
325328
// Init mock
326329
client, finish := mockVcsClient(t)
327330
defer finish()
@@ -335,12 +338,13 @@ func TestBeforeScanCreateLabelErr(t *testing.T) {
335338
Color: string(utils.LabelColor),
336339
}).Return(expectedError)
337340

338-
// Run beforeScan
339-
err := beforeScan(params, client)
341+
// Run handleFrogbotLabel
342+
shouldScan, err := handleFrogbotLabel(params, client)
340343
assert.ErrorIs(t, err, expectedError)
344+
assert.False(t, shouldScan)
341345
}
342346

343-
func TestBeforeScanUnlabeled(t *testing.T) {
347+
func TestHandleFrogbotLabelUnlabeled(t *testing.T) {
344348
// Init mock
345349
client, finish := mockVcsClient(t)
346350
defer finish()
@@ -353,12 +357,13 @@ func TestBeforeScanUnlabeled(t *testing.T) {
353357
}, nil)
354358
client.EXPECT().ListPullRequestLabels(context.Background(), params.RepoOwner, params.Repo, params.PullRequestID).Return([]string{}, nil)
355359

356-
// Run beforeScan
357-
err := beforeScan(params, client)
358-
assert.ErrorIs(t, err, utils.ErrUnlabel)
360+
// Run handleFrogbotLabel
361+
shouldScan, err := handleFrogbotLabel(params, client)
362+
assert.NoError(t, err)
363+
assert.False(t, shouldScan)
359364
}
360365

361-
func TestBeforeScanCreateListLabelsErr(t *testing.T) {
366+
func TestHandleFrogbotLabelCreateListLabelsErr(t *testing.T) {
362367
// Init mock
363368
client, finish := mockVcsClient(t)
364369
defer finish()
@@ -377,9 +382,10 @@ func TestBeforeScanCreateListLabelsErr(t *testing.T) {
377382
}).Return(nil)
378383
client.EXPECT().ListPullRequestLabels(context.Background(), params.RepoOwner, params.Repo, params.PullRequestID).Return([]string{}, expectedError)
379384

380-
// Run beforeScan
381-
err := beforeScan(params, client)
385+
// Run handleFrogbotLabel
386+
shouldScan, err := handleFrogbotLabel(params, client)
382387
assert.ErrorIs(t, err, expectedError)
388+
assert.False(t, shouldScan)
383389
}
384390

385391
func mockVcsClient(t *testing.T) (*testdata.MockVcsClient, func()) {

Diff for: ‎commands/utils/consts.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ const (
2020
gitLab vcsProvider = "gitlab"
2121

2222
// Frogbot label
23-
LabelName frogbotLabel = "🐸 frogbot scan pr"
23+
LabelName frogbotLabel = "🐸 frogbot scan"
2424
LabelDescription frogbotLabel = "triggers frogbot scan"
2525
LabelColor frogbotLabel = "4AB548"
2626

Diff for: ‎commands/utils/icons.go

+4-2
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,10 @@ import (
55
"strings"
66
)
77

8-
func GetSeverityTag(iconName string) string {
9-
switch strings.ToLower(iconName) {
8+
type IconName string
9+
10+
func GetSeverityTag(iconName IconName) string {
11+
switch strings.ToLower(string(iconName)) {
1012
case "critical":
1113
return getIconTag(criticalSeveritySource)
1214
case "high":

Diff for: ‎commands/utils/params.go

+1
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ type JFrogEnvParams struct {
2323
Project string
2424
Watches string
2525
}
26+
2627
type GitParam struct {
2728
GitProvider vcsutils.VcsProvider
2829
RepoOwner string

Diff for: ‎commands/utils/utils.go

-3
Original file line numberDiff line numberDiff line change
@@ -5,9 +5,6 @@ import (
55
"os"
66
)
77

8-
var ErrLabelCreated = fmt.Errorf("label '%s' was created. Please label this pull request to trigger an Xray scan", string(LabelName))
9-
var ErrUnlabel = fmt.Errorf("please add the '%s' label to trigger an Xray scan", string(LabelName))
10-
118
type errMissingEnv struct {
129
variableName string
1310
}

Diff for: ‎main.go

-8
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,9 @@
11
package main
22

33
import (
4-
"errors"
54
"os"
65

76
"github.com/jfrog/frogbot/commands"
8-
"github.com/jfrog/frogbot/commands/utils"
97
"github.com/jfrog/jfrog-cli-core/v2/utils/coreutils"
108
"github.com/jfrog/jfrog-cli-core/v2/utils/log"
119
"github.com/jfrog/jfrog-client-go/utils/io/fileutils"
@@ -30,12 +28,6 @@ func execMain() error {
3028
Usage: "See https://github.com/jfrog/frogbot for usage instructions.",
3129
Commands: getCommands(),
3230
Version: frogbotVersion,
33-
ExitErrHandler: func(context *clitool.Context, err error) {
34-
if errors.Is(err, utils.ErrLabelCreated) || errors.Is(err, utils.ErrUnlabel) {
35-
clientLog.Info("Scan wasn't triggered: " + err.Error())
36-
os.Exit(0)
37-
}
38-
},
3931
}
4032

4133
err := app.Run(os.Args)

Diff for: ‎templates/github-actions/frogbot-dotnet.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: "Frogbot"
22
on:
3-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
4-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
3+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
4+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
55
pull_request_target:
66
types: [labeled, opened]
77
jobs:

Diff for: ‎templates/github-actions/frogbot-go.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: "Frogbot"
22
on:
3-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
4-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
3+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
4+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
55
pull_request_target:
66
types: [labeled, opened]
77
jobs:

Diff for: ‎templates/github-actions/frogbot-gradle.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: "Frogbot"
22
on:
3-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
4-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
3+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
4+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
55
pull_request_target:
66
types: [labeled, opened]
77
jobs:

Diff for: ‎templates/github-actions/frogbot-maven.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: "Frogbot"
22
on:
3-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
4-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
3+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
4+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
55
pull_request_target:
66
types: [labeled, opened]
77
jobs:

Diff for: ‎templates/github-actions/frogbot-npm.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: "Frogbot"
22
on:
3-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
4-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
3+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
4+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
55
pull_request_target:
66
types: [labeled, opened]
77
jobs:

Diff for: ‎templates/github-actions/frogbot-nuget.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: "Frogbot"
22
on:
3-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
4-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
3+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
4+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
55
pull_request_target:
66
types: [labeled, opened]
77
jobs:

Diff for: ‎templates/github-actions/frogbot-pip.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: "Frogbot"
22
on:
3-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
4-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
3+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
4+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
55
pull_request_target:
66
types: [labeled, opened]
77
jobs:

Diff for: ‎templates/github-actions/frogbot-pipenv.yml

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
name: "Frogbot"
22
on:
3-
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan pr" label if needed.
4-
# After "🐸 frogbot scan pr" label was added to a pull request, Frogbot scans the pull request.
3+
# After a pull request opened, Frogbot automatically creates the "🐸 frogbot scan" label if needed.
4+
# After "🐸 frogbot scan" label was added to a pull request, Frogbot scans the pull request.
55
pull_request_target:
66
types: [labeled, opened]
77
jobs:

0 commit comments

Comments
 (0)