Migrate to golangci-lint #3023

sayboras · 2019-10-09T11:35:38Z

Currently, I keep the same set of linters we have from gometalinter.

Minor changes for below linting error

cmd/registry-api-descriptor-template/main.go:24                 goimports  File is not `goimports`-ed
registry/storage/driver/middleware/cloudfront/middleware.go:19  goimports  File is not `goimports`-ed
blobs.go:13                                                     goimports  File is not `goimports`-ed

GordonTheTurtle · 2019-10-09T11:35:40Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "feature/golangci-lint" git@github.com:sayboras/distribution.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

sayboras · 2019-10-09T11:39:18Z

@tariq1890 please help to take a look on this PR. Thanks

cappyzawa

I added some suggestions to pass CI.

cappyzawa · 2019-11-12T14:00:14Z

Makefile

@@ -50,7 +50,7 @@ version/version.go:

 check: ## run all linters (TODO: enable "unused", "varcheck", "ineffassign", "unconvert", "staticheck", "goimports", "structcheck")
 	@echo "$(WHALE) $@"
-	gometalinter --config .gometalinter.json ./...
+	@golangci-lint run


Suggested change

@golangci-lint run

@GO111MODULE=off golangci-lint run

cappyzawa · 2019-11-12T14:01:06Z

registry/registry.go

@@ -12,7 +12,6 @@ import (
 	"syscall"
 	"time"

-	"github.com/Shopify/logrus-bugsnag"


Suggested change

"github.com/Shopify/logrus-bugsnag"

logrus_bugsnag "github.com/Shopify/logrus-bugsnag"

thanks a lot 💯

codecov · 2019-11-13T13:08:35Z

Codecov Report

Merging #3023 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3023   +/-   ##
=======================================
  Coverage   60.74%   60.74%           
=======================================
  Files         102      102           
  Lines        8077     8077           
=======================================
  Hits         4906     4906           
  Misses       2519     2519           
  Partials      652      652

Flag	Coverage Δ
#linux	`60.74% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
registry/handlers/manifests.go	`52.82% <ø> (ø)`	⬆️
registry/client/repository.go	`59.76% <ø> (ø)`	⬆️
registry/handlers/tags.go	`73.07% <ø> (ø)`	⬆️
registry/handlers/blob.go	`72.54% <ø> (ø)`	⬆️
registry/storage/manifeststore.go	`75.36% <ø> (ø)`	⬆️
registry/handlers/app.go	`48.16% <ø> (ø)`	⬆️
registry/storage/ocimanifesthandler.go	`68.85% <ø> (ø)`	⬆️
manifest/ocischema/builder.go	`66.66% <ø> (ø)`	⬆️
registry/storage/schema2manifesthandler.go	`65.57% <ø> (ø)`	⬆️
registry/handlers/context.go	`80.76% <ø> (ø)`	⬆️
... and 3 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 a837179...6680964. Read the comment docs.

sayboras · 2019-12-09T11:36:34Z

@dmcgowan @manishtomar @tariq1890 any input for this PR ? Thanks

dmcgowan · 2019-12-13T19:15:44Z

@sayboras I'll take a look

dmcgowan · 2019-12-13T19:18:23Z

.golangci.yml

@@ -0,0 +1,150 @@
+# options for analysis running


Where did this version of the lint config come from?

In general, I would like to see this converge with containerd's configuration, see https://github.com/containerd/containerd/blob/master/.golangci.yml

If there are changes here I would like to see them applied in both places

Oh this one is coming from example config from golangci-lint.

However, it's making sense to keep the same as other repo (e.g. containerd). Let me make an update.

dmcgowan · 2019-12-13T19:19:50Z

go.mod

-	google.golang.org/grpc v0.0.0-20160317175043-d3ddb4469d5a // indirect
-	gopkg.in/check.v1 v1.0.0-20141024133853-64131543e789
-	gopkg.in/yaml.v2 v2.2.2
+	google.golang.org/grpc v1.21.0 // indirect


where is this grpc version change from?

This is unintentional changes, I have reverted go.mod

sayboras · 2019-12-19T02:55:54Z

@dmcgowan the changes are cleaner now. Please help to take another look.

sayboras · 2020-01-06T13:09:48Z

@dmcgowan please let me know if you have any comments, happy to address.

thaJeztah

thanks! overall these changes look good, but some cleaning up might be good to not break git bisect; currently, the first commit switches to golangci-lint, and follow-up commits fix linking issues. That means that if someone would try a git bisect, the commit that switches to golangci-lint would "fail".

Ideally (but I'm not a maintainer on this repo);

rebase and re-order commits to first have all the fixes (those fixes should already work with the old linter)
move the "switch to golangci-lint" to be the last commit

I haven't checked all commits, but some of the "linting fix" commits could possibly be squashed (e.g. master already switched to Go 1.12.x)

.golangci.yml

sayboras · 2020-02-13T21:04:41Z

thanks! overall these changes look good, but some cleaning up might be good to not break git bisect; currently, the first commit switches to golangci-lint, and follow-up commits fix linking issues. That means that if someone would try a git bisect, the commit that switches to golangci-lint would "fail".

Just curious if it's ok for me to squash all commits into one :)

Signed-off-by: Tam Mach <sayboras@yahoo.com>

dmcgowan

LGTM

GordonTheTurtle added the dco/no label Oct 9, 2019

sayboras force-pushed the feature/golangci-lint branch from 14c3972 to ff12894 Compare October 9, 2019 11:37

GordonTheTurtle removed the dco/no label Oct 9, 2019

GordonTheTurtle added the dco/no label Oct 9, 2019

sayboras force-pushed the feature/golangci-lint branch from d4615c2 to d3bbfda Compare October 9, 2019 11:50

GordonTheTurtle removed the dco/no label Oct 9, 2019

sayboras force-pushed the feature/golangci-lint branch 3 times, most recently from dde82ee to e6e8064 Compare October 9, 2019 12:02

cappyzawa reviewed Nov 12, 2019

View reviewed changes

GordonTheTurtle added the dco/no label Nov 13, 2019

sayboras force-pushed the feature/golangci-lint branch 2 times, most recently from d91fb6e to db09756 Compare November 13, 2019 12:29

GordonTheTurtle removed the dco/no label Nov 13, 2019

sayboras force-pushed the feature/golangci-lint branch from 15881a9 to 2d19491 Compare November 13, 2019 12:42

cappyzawa approved these changes Nov 14, 2019

View reviewed changes

sayboras force-pushed the feature/golangci-lint branch from 0426d8d to 31f7ec9 Compare December 9, 2019 11:33

dmcgowan reviewed Dec 13, 2019

View reviewed changes

GordonTheTurtle added the dco/no label Dec 16, 2019

sayboras force-pushed the feature/golangci-lint branch 4 times, most recently from d995aa4 to 521e48d Compare December 16, 2019 13:14

GordonTheTurtle removed the dco/no label Dec 16, 2019

sayboras requested a review from dmcgowan December 16, 2019 13:16

thaJeztah mentioned this pull request Jan 29, 2020

Update Golang 1.13.7, golang.org/x/crypto (CVE-2020-0601, CVE-2020-7919) #3089

Merged

thaJeztah requested changes Jan 29, 2020

View reviewed changes

.golangci.yml Outdated Show resolved Hide resolved

GordonTheTurtle added the dco/no label Feb 13, 2020

Migrate to golangci-lint

6680964

Signed-off-by: Tam Mach <sayboras@yahoo.com>

sayboras force-pushed the feature/golangci-lint branch from 6623bd1 to 6680964 Compare February 13, 2020 21:11

GordonTheTurtle removed the dco/no label Feb 13, 2020

sayboras requested a review from thaJeztah February 13, 2020 21:12

dmcgowan approved these changes Feb 21, 2020

View reviewed changes

dmcgowan merged commit 244d524 into distribution:master Feb 21, 2020

	"github.com/Shopify/logrus-bugsnag"
	logrus_bugsnag "github.com/Shopify/logrus-bugsnag"

Migrate to golangci-lint #3023

Migrate to golangci-lint #3023

Uh oh!

Conversation

sayboras commented Oct 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GordonTheTurtle commented Oct 9, 2019

Uh oh!

sayboras commented Oct 9, 2019

Uh oh!

cappyzawa left a comment

Choose a reason for hiding this comment

Uh oh!

cappyzawa Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

cappyzawa Nov 12, 2019

Choose a reason for hiding this comment

Uh oh!

sayboras Nov 13, 2019

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sayboras commented Dec 9, 2019

Uh oh!

dmcgowan commented Dec 13, 2019

Uh oh!

dmcgowan Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

sayboras Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

dmcgowan Dec 13, 2019

Choose a reason for hiding this comment

Uh oh!

sayboras Dec 16, 2019

Choose a reason for hiding this comment

Uh oh!

sayboras commented Dec 19, 2019

Uh oh!

sayboras commented Jan 6, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sayboras commented Feb 13, 2020

Uh oh!

dmcgowan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sayboras commented Oct 9, 2019 •

edited

Loading

codecov bot commented Nov 13, 2019 •

edited

Loading