Replace gometalinter with Golangci lint #1797

silvin-lubecki · 2019-04-02T15:53:27Z

- What I did
gometalinter is deprecated in favour og golangci-lint which is faster and less memory hungry.
https://github.com/golangci/golangci-lint has more on the performance etc.

All the caught issues were fixed, commit by commit for better review. Happy to squash everything at the end of the review 👍

Moved the linter Dockerfile to Multistage build.

- A picture of a cute animal (not mandatory but encouraged)

…taticcheck) Signed-off-by: Silvin Lubecki <[email protected]>

…of append is never used, except maybe in other appends (staticcheck) Signed-off-by: Silvin Lubecki <[email protected]>

…the underlying ticker, consider using it only in endless functions, tests and the main package, and use time.NewTicker here (staticcheck) Signed-off-by: Silvin Lubecki <[email protected]>

…check) Signed-off-by: Silvin Lubecki <[email protected]>

…n: nil != nil (govet) Signed-off-by: Silvin Lubecki <[email protected]>

…dition: non-nil != nil (govet) Signed-off-by: Silvin Lubecki <[email protected]>

…pe assertion (govet) Signed-off-by: Silvin Lubecki <[email protected]>

…nil != nil (govet) Signed-off-by: Silvin Lubecki <[email protected]>

…,inline", json:"-"` not compatible with reflect.StructTag.Get: key:"value" pairs not separated by spaces (govet) Signed-off-by: Silvin Lubecki <[email protected]>

…/github.com/docker/go-units.Ulimit` composite literal uses unkeyed fields (govet) Signed-off-by: Silvin Lubecki <[email protected]>

…: nil != nil (govet) Signed-off-by: Silvin Lubecki <[email protected]>

Signed-off-by: Silvin Lubecki <[email protected]>

… `dir` always receives `""` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…- `perm` always receives `0777` (`511`) (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…r) is always nil (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

… 1 (error) is always nil (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…sed (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…ces` - `stackName` always receives `"test"` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…error) is always nil (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…rror) is always nil (unparam) cli/compose/convert/service.go:538:110: convertEndpointSpec - result 1 (error) is always nil (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…se function signatures cli/compose/loader/loader.go:756:66: transformServiceNetworkMap - result 1 (error) is always nil (unparam) cli/compose/loader/loader.go:767:67: transformStringOrNumberList - result 1 (error) is always nil (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

… `s` is unused (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…always receives `"argument"` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…sphrase` - `pwd` always receives `"foo"` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…ert) Signed-off-by: Silvin Lubecki <[email protected]>

Signed-off-by: Silvin Lubecki <[email protected]>

…nconvert) Signed-off-by: Silvin Lubecki <[email protected]>

…onvert) Signed-off-by: Silvin Lubecki <[email protected]>

… `prefix` always receives `"builder-context-test"` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

…lmost the same configuration. Signed-off-by: Silvin Lubecki <[email protected]>

codecov-io · 2019-04-02T16:00:17Z

Codecov Report

❗ No coverage uploaded for pull request base (master@5bbb56b). Click here to learn what that means.
The diff coverage is 33.33%.

@@           Coverage Diff            @@
##             master   #1797   +/-   ##
========================================
  Coverage          ?   56.3%           
========================================
  Files             ?     308           
  Lines             ?   21284           
  Branches          ?       0           
========================================
  Hits              ?   11983           
  Misses            ?    8430           
  Partials          ?     871

ijc · 2019-04-02T16:19:37Z

.golangci.yml

+  skip-files:
+    - cli/compose/schema/bindata.go
+    - .*generated.*
+    - parameter .* always receives


Is this in the right section?

vdemeester

Few question, otherwise looks good 👍

vdemeester · 2019-04-02T16:24:43Z

.golangci.yml

@@ -0,0 +1,35 @@
+run:
+  deadline: 3m


vdemeester · 2019-04-02T16:25:01Z

.golangci.yml

+  skip-files:
+    - cli/compose/schema/bindata.go
+    - .*generated.*
+    - parameter .* always receives


vdemeester · 2019-04-02T16:25:43Z

cli/command/container/attach.go

 	}

 	resp, errAttach := client.ContainerAttach(ctx, opts.container, options)
-	if errAttach != nil && errAttach != httputil.ErrPersistEOF {


I don't understand that change 😓

The httputil.ErrPersistEOF is no longer used, so should no longer be returned by anything.

That said; the cli could connect to an older daemon that still returns this error, so I think we should change these to an //nolint, which is what we did in the engine;

//nolint:staticcheck // ignore SA1019 for connecting to old (pre go1.8) daemons

moby/moby@fd65fed (updated in moby/moby@5f47cef)

Oh, actually; so that moby change is the exact path we're hitting here in the ContainerAttach, so the client won't return a httputil.ErrPersistEOF error, so we can safely remove that condition here.

vdemeester · 2019-04-03T08:10:33Z

cli/command/container/run.go

-		// ContainerAttach returns an ErrPersistEOF (connection closed)
-		// means server met an error and put it in Hijacked connection
-		// keep the error and read detailed error message from hijacked connection later
+	if errAttach != nil {


vdemeester · 2019-04-03T08:10:37Z

cli/command/container/start.go

-			// ContainerAttach return an ErrPersistEOF (connection closed)
-			// means server met an error and already put it in Hijacked connection,
-			// we would keep the error and read the detailed error message from hijacked connection
+		if errAttach != nil {


vdemeester · 2019-04-03T08:13:58Z

cli/command/image/build/context_test.go

 // When an error occurs, it terminates the test.
-func createTestTempDir(t *testing.T, dir, prefix string) (string, func()) {
-	path, err := ioutil.TempDir(dir, prefix)
+//nolint: unparam


Why is unparam needed ?

createTestTempDir - prefix always receives "builder-context-test"

I guess instead, we could change one test here as well

thaJeztah · 2019-04-11T11:08:11Z

dockerfiles/Dockerfile.lint

@@ -1,18 +1,15 @@
-FROM    golang:1.12.1-alpine
+FROM    golang:1.12.1 as build


alpine doesn't work for this one? 😢

nit: could you use capital AS ? Trying to keep Dockerfile keywords consistently cased

Suggested change

FROM golang:1.12.1 as build

FROM golang:1.12.1 AS build

thaJeztah · 2019-04-11T11:09:07Z

dockerfiles/Dockerfile.lint

-        go build -v -o /usr/local/bin/gometalinter . && \ 
-        gometalinter --install && \
-        rm -rf /go/src/* /go/pkg/*
+FROM golang:1.12.1-alpine as lint


Suggested change

FROM golang:1.12.1-alpine as lint

FROM golang:1.12.1-alpine AS lint

thaJeztah · 2019-04-11T20:20:34Z

cli/compose/loader/loader.go

 	return value, nil
 }

+// nolint: unparam


Wonder if it would be smart enough to accept it if we'd define a type for this;

cli/cli/compose/loader/loader.go

Lines 290 to 293 in 9a39a10

Func func(interface{}) (interface{}, error)

}

func createTransformHook(additionalTransformers ...Transformer) mapstructure.DecodeHookFuncType {

thaJeztah · 2019-04-11T20:23:56Z

e2e/image/push_test.go

 	return image
 }

+//nolint: unparam


withNotaryPassphrase - pwd always receives "foo"

perhaps we should change one test to use a different value 😅

thaJeztah · 2019-04-11T20:27:10Z

cli/command/image/build/context_test.go

 // When an error occurs, it terminates the test.
-func createTestTempDir(t *testing.T, dir, prefix string) (string, func()) {
-	path, err := ioutil.TempDir(dir, prefix)
+//nolint: unparam


createTestTempDir - prefix always receives "builder-context-test"

I guess instead, we could change one test here as well

thaJeztah · 2019-04-18T11:41:24Z

ping @silvin-lubecki 🤗 (also looks like it needs a rebase)

thaJeztah · 2019-05-14T01:14:45Z

ping @silvin-lubecki 🤗

silvin-lubecki added 28 commits April 2, 2019 17:46

opts/throttledevice.go:51:5: SA4003: unsigned values are never < 0 (s…

c38ca4c

…taticcheck) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/trust/inspect_pretty_test.go:399:24: SA4010: this result …

65808fc

…of append is never used, except maybe in other appends (staticcheck) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/container/stats.go:211:21: SA1015: using time.Tick leaks …

b2158b2

…the underlying ticker, consider using it only in endless functions, tests and the main package, and use time.NewTicker here (staticcheck) Signed-off-by: Silvin Lubecki <[email protected]>

SA1019: httputil.ErrPersistEOF is deprecated: No longer used. (static…

fcbd937

…check) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/trust/key_generate.go:112:9: nilness: impossible conditio…

63bf41f

…n: nil != nil (govet) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/stack/kubernetes/list.go:32:47: nilness: tautological con…

0d5c8d0

…dition: non-nil != nil (govet) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/container/start.go:157:20: nilness: nil dereference in ty…

27ebbbe

…pe assertion (govet) Signed-off-by: Silvin Lubecki <[email protected]>

cli/registry/client/fetcher.go:106:9: nilness: impossible condition: …

68a8b2c

…nil != nil (govet) Signed-off-by: Silvin Lubecki <[email protected]>

cli/compose/types/types.go:106:2: structtag: struct field tag `yaml:"…

31e233b

…,inline", json:"-"` not compatible with reflect.StructTag.Get: key:"value" pairs not separated by spaces (govet) Signed-off-by: Silvin Lubecki <[email protected]>

opts/ulimit_test.go:11:13: composites: `*github.com/docker/cli/vendor…

0850214

…/github.com/docker/go-units.Ulimit` composite literal uses unkeyed fields (govet) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/container/attach.go:141:15: nilness: impossible condition…

2fc15a6

…: nil != nil (govet) Signed-off-by: Silvin Lubecki <[email protected]>

unchecked errors

d5bbb94

Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/image/build/context_test.go:244:38: createTestTempDir -…

3c4788a

… `dir` always receives `""` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/image/build/context_test.go:252:71: createTestTempFile …

321aff9

…- `perm` always receives `0777` (`511`) (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/image/build_buildkit.go:450:56: parseSSH - result 1 (erro…

216bedc

…r) is always nil (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/image/build_session.go:133:45: getBuildSharedKey - result…

5abbbfc

… 1 (error) is always nil (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/plugin/list_test.go:61:31: TestList$1 - filter is unu…

65237e7

…sed (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/stack/kubernetes/deploy_test.go:65:68: `checkOwnerReferen…

8f98106

…ces` - `stackName` always receives `"test"` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/system/info.go:116:68: prettyPrintClientInfo - result 0 (…

d110697

…error) is always nil (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

cli/compose/convert/service.go:592:76: convertDNSConfig - result 1 (e…

c014ec1

…rror) is always nil (unparam) cli/compose/convert/service.go:538:110: convertEndpointSpec - result 1 (error) is always nil (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

cli/context/kubernetes/endpoint_test.go:122:38: checkClientConfig -…

2f09e17

… `s` is unused (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

Disable unparam linter: cli/required.go:102:16: pluralize - word …

f443ad3

…always receives `"argument"` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

Disable unparam linter: e2e/image/push_test.go:299:27: `withNotaryPas…

02091c1

…sphrase` - `pwd` always receives `"foo"` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/trust/sign_test.go:119:70: unnecessary conversion (unconv…

cd030a1

…ert) Signed-off-by: Silvin Lubecki <[email protected]>

File is not goimports-ed (goimports)

4a7a541

Signed-off-by: Silvin Lubecki <[email protected]>

cli/compose/convert/service_test.go:274:72: unnecessary conversion (u…

54cab47

…nconvert) Signed-off-by: Silvin Lubecki <[email protected]>

cli/command/registry/login_test.go:66:25: unnecessary conversion (unc…

06908e0

…onvert) Signed-off-by: Silvin Lubecki <[email protected]>

GordonTheTurtle added the status/0-triage label Apr 2, 2019

cli/command/image/build/context_test.go:244:38: createTestTempDir -…

e62ed7a

… `prefix` always receives `"builder-context-test"` (unparam) Signed-off-by: Silvin Lubecki <[email protected]>

Remove now obsolete gometalinter and use golangci-lint instead with a…

fbf90f3

…lmost the same configuration. Signed-off-by: Silvin Lubecki <[email protected]>

silvin-lubecki force-pushed the golangci-lint branch from 0bb59c4 to fbf90f3 Compare April 2, 2019 16:00

ijc reviewed Apr 2, 2019

View reviewed changes

vdemeester reviewed Apr 3, 2019

View reviewed changes

thaJeztah mentioned this pull request Apr 11, 2019

Bump Golang 1.12.3 #1818

Merged

thaJeztah reviewed Apr 11, 2019

View reviewed changes

thaJeztah added area/testing status/2-code-review and removed status/0-triage labels Apr 13, 2019

thaJeztah requested changes Apr 13, 2019

View reviewed changes

thaJeztah mentioned this pull request Sep 11, 2019

Replace gometalinter with golangci-lint moby/moby#39668

Merged

This was referenced Oct 24, 2019

Fix-up (gometalinter) linting config #2163

Merged

Replace gometalinter with Golangci lint [carry 1797] #2173

Merged

thaJeztah closed this in #2173 Oct 31, 2019

silvin-lubecki deleted the golangci-lint branch November 7, 2019 19:53

		@@ -1,18 +1,15 @@
		FROM golang:1.12.1-alpine
		FROM golang:1.12.1 as build

	FROM golang:1.12.1-alpine as lint
	FROM golang:1.12.1-alpine AS lint

	Func func(interface{}) (interface{}, error)
	}

	func createTransformHook(additionalTransformers ...Transformer) mapstructure.DecodeHookFuncType {

Replace gometalinter with Golangci lint #1797

Replace gometalinter with Golangci lint #1797

Uh oh!

Conversation

silvin-lubecki commented Apr 2, 2019

Uh oh!

codecov-io commented Apr 2, 2019

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Apr 18, 2019

Uh oh!

thaJeztah commented May 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants