Add linter check support #679

rahul2393 · 2020-10-11T12:05:08Z

codecov · 2020-10-11T12:49:08Z

Codecov Report

Merging #679 into master will increase coverage by 7.94%.
The diff coverage is 61.53%.

@@            Coverage Diff             @@
##           master     #679      +/-   ##
==========================================
+ Coverage   55.69%   63.64%   +7.94%     
==========================================
  Files          48       48              
  Lines        2027     2027              
==========================================
+ Hits         1129     1290     +161     
+ Misses        766      605     -161     
  Partials      132      132

Impacted Files	Coverage Δ
internal/client/config/config.go	`80.64% <ø> (ø)`
internal/config/artifact.go	`55.00% <0.00%> (ø)`
internal/config/db.go	`100.00% <ø> (ø)`
internal/config/global.go	`84.61% <ø> (ø)`
internal/config/image.go	`100.00% <ø> (ø)`
internal/config/report.go	`60.60% <ø> (ø)`
pkg/detector/library/advisory.go	`90.62% <ø> (ø)`
pkg/detector/library/bundler/advisory.go	`64.70% <ø> (ø)`
pkg/detector/library/detect.go	`0.00% <ø> (ø)`
pkg/detector/ospkg/alpine/alpine.go	`94.73% <ø> (ø)`
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c3bfb8...eec01fb. Read the comment docs.

.golangci.yaml

pkg/vulnerability/vulnerability.go

.github/workflows/test.yaml

go.mod

integration/docker/docker.go

internal/app.go

pkg/types/vulnerability.go

pkg/scanner/local/scan.go

pkg/report/writer.go

internal/client/run.go

internal/artifact/run.go

knqyf263 · 2020-10-14T09:49:51Z

.golangci.yaml

+  misspell:
+    locale: US
+  goimports:
+    local-prefixes: github.com/rahul23/trivy


Suggested change

local-prefixes: github.com/rahul23/trivy

local-prefixes: github.com/aquasecurity/trivy

It was for running the linter check in workflow here, reverted

Reverted to rahul2393?

internal/artifact/run.go

internal/client/run.go

knqyf263 · 2020-10-14T14:17:45Z

pkg/detector/ospkg/detect.go

 	}
-	return d
+	log.Logger.Warnf("unsupported os : %s", osFamily)


Hmm... I prefer the previous one. If we follow the linter, return xxx.NewScanner() looks better.

knqyf263 · 2020-10-15T07:01:37Z

.golangci.yaml

@@ -65,6 +65,9 @@ issues:
    - linters:
        - golint
      text: "a blank import should be only in a main or test package"
+    - linters:
+        - govet
+      text: "shadow: declaration of"


Doesn't it work as follow?
https://github.com/golangci/golangci-lint/blob/58234f055cd8a4ba71ce6dafb95d7fa665bc8d39/.golangci.example.yml#L242-L243

knqyf263 · 2020-10-15T07:03:01Z

.golangci.yaml

+  misspell:
+    locale: US
+  goimports:
+    local-prefixes: github.com/rahul23/trivy


Reverted to rahul2393?

rahul2393 · 2020-10-15T07:40:56Z

@knqyf263 Done

knqyf263 · 2020-10-15T07:45:45Z

@rahul2393 The linter test seems to be failing.

rahul2393 · 2020-10-15T07:54:18Z

As I said it will fail, but once merged it will succeed
#679 (comment)
@knqyf263

rahul2393 · 2020-10-15T08:01:31Z

Let me know what are next action items now

knqyf263 · 2020-10-15T10:04:47Z

As I said it will fail

Could you clarify it? I don't get your point yet.

rahul2393 · 2020-10-15T10:13:13Z

@knqyf263 see the last commit linters passed here 601d126 so once I revert it, linters will fail but once merged it will pass

knqyf263 · 2020-10-15T10:14:40Z

No, I meant why it needs to be rahul2393.

rahul2393 · 2020-10-15T10:19:03Z

Maybe because the workflow run my forked repo

knqyf263 · 2020-10-15T10:21:40Z

I also thought so, but I didn't find the clue. And if so, it means PR always fails the linter test.

rahul2393 · 2020-10-15T10:22:57Z

@knqyf263 removed that config now I think its good.

knqyf263 · 2020-10-15T10:26:38Z

We should find out what effect the option has and make sure it's okay to remove it. We have to avoid "we don't know why it fails, but it looks good after removing it" as much as possible.

rahul2393 · 2020-10-15T10:37:23Z

@knqyf263 this is to grouping the import in separate lines and disabling local prefixes will not cause issue.
https://stackoverflow.com/a/59964885

rahul2393 · 2020-10-15T14:50:48Z

@knqyf263 Check now, added back the local-prefixes goimport and linter issues fixed

knqyf263 · 2020-10-18T09:29:32Z

Hmm. Please allow me to check that my understanding is correct or not. What you said as follows is not correct, right? Even after it is merged, the grouping still violates the policy, so the test will fail.
#679 (comment)

Also, can we specify github.com/aquasecurity, not github.com/aquasecurity/trivy? The repositories under github.com/aquasecurity are also developed by us, so we can call it local.

rahul2393 · 2020-10-19T08:48:46Z

@knqyf263 Done, make sense to add "github.com/aquasecurity" as local prefix.

knqyf263 · 2020-10-19T08:53:31Z

@rahul2393 What about the first question? Is my understanding correct? I'd like to figure out what was wrong.

rahul2393 · 2020-10-19T08:56:22Z

Yes, I was wrong before, if its merged now it will work fine because in all the files the grouping is fine, e.g

go internal packages
third party packages
acquasecurity libs
all 3 separated by single space, earlier I was wrong because I put forked local prefix although it was passing it was wrong because it was considering "github.com/rahul2393" as local prefix

knqyf263 · 2020-10-19T09:04:26Z

It makes sense. Thanks! I'll take a look at last.

pkg/detector/ospkg/debian/debian.go

pkg/detector/ospkg/oracle/oracle.go

pkg/detector/ospkg/redhat/redhat.go

knqyf263 · 2020-10-20T05:38:22Z

pkg/detector/library/python/advisory.go

 	"golang.org/x/xerrors"

 	"github.com/Masterminds/semver/v3"


Could you group them?

knqyf263 · 2020-10-20T05:38:37Z

pkg/detector/library/driver.go

+	"golang.org/x/xerrors"
+
 	"github.com/Masterminds/semver/v3"


knqyf263 · 2020-10-20T05:38:47Z

pkg/detector/library/detect.go

+	"golang.org/x/xerrors"

 	"github.com/google/wire"

 	"github.com/Masterminds/semver/v3"


knqyf263 · 2020-10-20T05:39:02Z

pkg/detector/library/cargo/advisory.go

+	"golang.org/x/xerrors"

 	"github.com/Masterminds/semver/v3"


knqyf263 · 2020-10-20T05:39:43Z

internal/server/config/config.go

 	"github.com/urfave/cli/v2"
+
+	"github.com/aquasecurity/trivy/internal/config"


knqyf263 · 2020-10-20T05:45:37Z

pkg/detector/ospkg/detect.go

@@ -56,31 +62,31 @@ func (d Detector) Detect(_, osFamily, osName string, _ time.Time, pkgs []ftypes.
 	return vulns, eosl, nil
 }

+// nolint: gocyclo
+// TODO: fix cyclometic complexity by removing default


I'd like to know why gocyclo complains default. Is it only about default? Or, even if we remove default and add a new family, will gocyclo complain again?

even if you replace the default with a different case, it will complain because by default cyclomatic complexity of 10, and for this function, default branch is adding up to 11

So, removing default doesn't address the root issue. Let's consider it carefully later.

knqyf263 · 2020-10-20T05:46:09Z

pkg/detector/ospkg/photon/photon.go

 	version "github.com/knqyf263/go-rpm-version"

 	"golang.org/x/xerrors"

+	"k8s.io/utils/clock"


knqyf263 · 2020-10-20T05:46:45Z

pkg/github/github.go

 	"golang.org/x/oauth2"
 	"golang.org/x/xerrors"
+
+	"github.com/google/go-github/v28/github"


knqyf263 · 2020-10-20T05:47:04Z

pkg/report/writer.go

 	"golang.org/x/xerrors"

+	"github.com/olekukonko/tablewriter"


knqyf263 · 2020-10-20T05:47:17Z

pkg/rpc/retry.go

 	"github.com/cenkalti/backoff"
+
 	"github.com/twitchtv/twirp"


rahul2393 · 2020-10-20T06:32:59Z

@knqyf263 I tried fixing all these issues

knqyf263 · 2020-10-20T12:14:28Z

pkg/detector/ospkg/detect.go

@@ -56,31 +62,31 @@ func (d Detector) Detect(_, osFamily, osName string, _ time.Time, pkgs []ftypes.
 	return vulns, eosl, nil
 }

+// nolint: gocyclo
+// TODO: fix cyclometic complexity by removing default


So, removing default doesn't address the root issue. Let's consider it carefully later.

knqyf263 · 2020-10-20T12:20:39Z

@rahul2393 Thanks.

* add linter supports * add only minor version * use latest version * Fix println with format issue * Fix test * Fix tests * For slice with unknown length, preallocating the array * fix code-coverage * Removed linter rules * Reverting linter fixes, adding TODO for later * Ignore linter error for import * Remove another err var. * Ignore shadow error * Fixes * Fix issue * Add back goimports local-prefixes * Update local prefixes * Removed extra spaces and merge the imports * more refactoring * Update photon.go Co-authored-by: Teppei Fukuda <[email protected]>

rahul2393 added 6 commits October 11, 2020 17:33

add linter supports

7e60fcd

add only minor version

5815438

use latest version

4542dbc

Fix println with format issue

abca649

Fix test

3a2612c

Fix tests

24a09ae

knqyf263 reviewed Oct 11, 2020

View reviewed changes

.golangci.yaml Show resolved Hide resolved

pkg/vulnerability/vulnerability.go Outdated Show resolved Hide resolved

pkg/vulnerability/vulnerability.go Outdated Show resolved Hide resolved

rahul2393 added 3 commits October 11, 2020 20:25

For slice with unknown length, preallocating the array

179851c

fix code-coverage

4797359

Removed linter rules

292f2ed

knqyf263 reviewed Oct 13, 2020

View reviewed changes

rahul2393 added 3 commits October 13, 2020 17:08

Reverting linter fixes, adding TODO for later

c5294cd

Ignore linter error for import

02a78c2

Remove another err var.

4f27b60

knqyf263 reviewed Oct 15, 2020

View reviewed changes

Ignore shadow error

2fd520d

knqyf263 reviewed Oct 15, 2020

View reviewed changes

Fixes

0557627

Fix issue

f5c4858

Add back goimports local-prefixes

b6e2b23

Update local prefixes

ab29542

knqyf263 reviewed Oct 19, 2020

View reviewed changes

pkg/detector/ospkg/debian/debian.go Outdated Show resolved Hide resolved

pkg/detector/ospkg/oracle/oracle.go Outdated Show resolved Hide resolved

pkg/detector/ospkg/redhat/redhat.go Outdated Show resolved Hide resolved

Removed extra spaces and merge the imports

bfea540

knqyf263 reviewed Oct 20, 2020

View reviewed changes

more refactoring

afc83a0

Update photon.go

eec01fb

knqyf263 approved these changes Oct 20, 2020

View reviewed changes

knqyf263 merged commit 793a1aa into aquasecurity:master Oct 20, 2020

	local-prefixes: github.com/rahul23/trivy
	local-prefixes: github.com/aquasecurity/trivy

		"golang.org/x/xerrors"

		"github.com/Masterminds/semver/v3"

		"github.com/urfave/cli/v2"

		"github.com/aquasecurity/trivy/internal/config"

		"golang.org/x/xerrors"

		"github.com/olekukonko/tablewriter"

		"github.com/cenkalti/backoff"

		"github.com/twitchtv/twirp"

Add linter check support #679

Add linter check support #679

Conversation

rahul2393 commented Oct 11, 2020

codecov bot commented Oct 11, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

rahul2393 Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

knqyf263 Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knqyf263 Oct 15, 2020 • edited Loading

Choose a reason for hiding this comment

rahul2393 commented Oct 15, 2020

knqyf263 commented Oct 15, 2020

rahul2393 commented Oct 15, 2020

rahul2393 commented Oct 15, 2020

knqyf263 commented Oct 15, 2020 • edited Loading

rahul2393 commented Oct 15, 2020

knqyf263 commented Oct 15, 2020

rahul2393 commented Oct 15, 2020

knqyf263 commented Oct 15, 2020

rahul2393 commented Oct 15, 2020

knqyf263 commented Oct 15, 2020

rahul2393 commented Oct 15, 2020

rahul2393 commented Oct 15, 2020

knqyf263 commented Oct 18, 2020 • edited Loading

rahul2393 commented Oct 19, 2020

knqyf263 commented Oct 19, 2020

rahul2393 commented Oct 19, 2020 • edited Loading

knqyf263 commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rahul2393 commented Oct 20, 2020

Choose a reason for hiding this comment

knqyf263 commented Oct 20, 2020

codecov bot commented Oct 11, 2020 •

edited

Loading

rahul2393 Oct 15, 2020 •

edited

Loading

knqyf263 Oct 15, 2020 •

edited

Loading

knqyf263 Oct 15, 2020 •

edited

Loading

knqyf263 commented Oct 15, 2020 •

edited

Loading

knqyf263 commented Oct 18, 2020 •

edited

Loading

rahul2393 commented Oct 19, 2020 •

edited

Loading