Skip to content

[testing] ~update~ fix linting issues found by golangci-lint v1.40.1#42445

Merged
thaJeztah merged 19 commits intomoby:masterfrom
thaJeztah:bump_golang_ci
Jun 16, 2021
Merged

[testing] ~update~ fix linting issues found by golangci-lint v1.40.1#42445
thaJeztah merged 19 commits intomoby:masterfrom
thaJeztah:bump_golang_ci

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented May 31, 2021

Splitting this from #42303, which needs some work (use a custom action instead of the default one; similar to buildkit CI)

This updates fixes linting issues found by the latest golangci-lint (v1.40.1)

@thaJeztah
Copy link
Member Author

Arf...

[2021-05-31T12:16:04.349Z] #49 [golangci_lint 1/1] RUN --mount=type=cache,target=/root/.cache/go-build     --mount=type=cache,target=/go/pkg/mod     --mount=type=bind,src=hack/dockerfile/install,target=/tmp/install         PREFIX=/build /tmp/install/install.sh golangci_lint
[2021-05-31T12:16:04.349Z] #49 sha256:349e1551451e60eb111e0b092c0002ff279617974c02cedd50f0cc7c74572fef
[2021-05-31T12:16:04.349Z] #49 85.93 # github.com/quasilyte/go-ruleguard/internal/gogrep
[2021-05-31T12:16:04.349Z] #49 85.93 /tmp/tmp.hsC4laHthw/pkg/mod/github.com/quasilyte/go-ruleguard@v0.3.4/internal/gogrep/match.go:677:13: undefined: strconv.ParseComplex
[2021-05-31T12:16:04.349Z] #49 85.93 note: module requires Go 1.15
[2021-05-31T12:16:04.606Z] #49 ...

No longer works on Go 1.13, so we'll have to wait for #40353

@thaJeztah
Copy link
Member Author

I'll remove the last two commits

@thaJeztah thaJeztah changed the title [testing] update golangci-lint to v1.40.1 [testing] ~update~ fix linting issues found by golangci-lint v1.40.1 May 31, 2021
@thaJeztah
Copy link
Member Author

@AkihiroSuda @cpuguy83 @tianon ptal

@thaJeztah
Copy link
Member Author

@samuelkarp updated; ptal 👍

Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Member Author

Rebased, and resolved the conflict in the golangci-lint config file (after #42262 was merged)

@thaJeztah
Copy link
Member Author

More flakiness in the libnetwork tests;

[2021-06-03T19:29:33.244Z] === Failed
[2021-06-03T19:29:33.244Z] === FAIL: libnetwork/networkdb TestNetworkDBIslands (123.95s)

opened #42459

@thaJeztah
Copy link
Member Author

https://ci-next.docker.com/public/job/moby/job/PR-42445/11/execution/node/333/log/

More flakiness?

TestFindNetworkUtil - opened #42482

=== RUN   TestFindNetworkUtil
time="2021-06-08T08:29:35Z" level=warning msg="bridge store not initialized. kv object docker/network/v1.0/bridge/b3f5da5d7faac2508f5265a089d36408a6bc2beb5946dd80602ac6e1e4f09ef1/ is not added to the store"
time="2021-06-08T08:29:35Z" level=warning msg="Failed to update store after ipam release for network network (b3f5da5d7faac2508f5265a089d36408a6bc2beb5946dd80602ac6e1e4f09ef1): failed to update store for object type *libnetwork.network: Key not found in store"
--- FAIL: TestFindNetworkUtil (0.16s)
    api_linux_test.go:1099: Failed to delete the network: error deleting network endpoint count from store: Key not found in store

TestUserChain - opened #42483

=== RUN   TestUserChain/iptables=true,insert=true
    --- FAIL: TestUserChain/iptables=true,insert=true (0.00s)
        firewall_linux_test.go:62: assertion failed:
            --- ←
            +++ →
            {[]string}[1->?]:
            	-: "-A FORWARD -i docker0 ! -o docker0 -j ACCEPT"
            	+: <non-existent>
            {[]string}[2->?]:
            	-: "-A FORWARD -i docker0 -o docker0 -j DROP"
            	+: <non-existent>

These should be ok to ignore for the purpose they're used

    pkg/namesgenerator/names-generator.go:843:36: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
        name := fmt.Sprintf("%s_%s", left[rand.Intn(len(left))], right[rand.Intn(len(right))])
                                          ^
    pkg/namesgenerator/names-generator.go:849:36: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
            name = fmt.Sprintf("%s%d", name, rand.Intn(10))
                                             ^
    testutil/stringutils.go:11:18: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
            b[i] = letters[rand.Intn(len(letters))]
                           ^
    pkg/namesgenerator/names-generator.go:849:36: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
            name = fmt.Sprintf("%s%d", name, rand.Intn(10))
                                             ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/logger/journald/read.go:128:3 comment on exported function `CErr` should be of the form `CErr ...`

    daemon/logger/journald/read.go:131:36: unnecessary conversion (unconvert)
            return C.GoString(C.strerror(C.int(-ret)))
	                                  ^
    daemon/logger/journald/read.go:380:2: S1023: redundant `return` statement (gosimple)
        return
        ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    client/request.go:245:2: S1031: unnecessary nil check around range (gosimple)
        if headers != nil {
        ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/volumes_unix_test.go:228:13: SA4001: &*x will be simplified to x. It will not copy x. (staticcheck)
                mp:      &(*c.MountPoints["/jambolan"]), // copy the mountpoint, expect no changes
                         ^
    daemon/logger/local/local_test.go:214:22: SA4001: &*x will be simplified to x. It will not copy x. (staticcheck)
            dst.PLogMetaData = &(*src.PLogMetaData)
                               ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    pkg/devicemapper/devmapper.go:383:28: S1039: unnecessary use of fmt.Sprintf (gosimple)
        if err := task.setMessage(fmt.Sprintf("@cancel_deferred_remove")); err != nil {
                                  ^
    integration/plugin/graphdriver/external_test.go:321:18: S1039: unnecessary use of fmt.Sprintf (gosimple)
                http.Error(w, fmt.Sprintf("missing id"), 409)
                              ^
    integration-cli/docker_api_stats_test.go:70:31: S1039: unnecessary use of fmt.Sprintf (gosimple)
            _, body, err := request.Get(fmt.Sprintf("/info"))
                                        ^
    integration-cli/docker_cli_build_test.go:4547:19: S1039: unnecessary use of fmt.Sprintf (gosimple)
                "--build-arg", fmt.Sprintf("FOO1=fromcmd"),
                               ^
    integration-cli/docker_cli_build_test.go:4548:19: S1039: unnecessary use of fmt.Sprintf (gosimple)
                "--build-arg", fmt.Sprintf("FOO2="),
                               ^
    integration-cli/docker_cli_build_test.go:4549:19: S1039: unnecessary use of fmt.Sprintf (gosimple)
                "--build-arg", fmt.Sprintf("FOO3"), // set in env
                               ^
    integration-cli/docker_cli_build_test.go:4668:32: S1039: unnecessary use of fmt.Sprintf (gosimple)
            cli.WithFlags("--build-arg", fmt.Sprintf("tag=latest")))
                                         ^
    integration-cli/docker_cli_build_test.go:4690:32: S1039: unnecessary use of fmt.Sprintf (gosimple)
            cli.WithFlags("--build-arg", fmt.Sprintf("baz=abc")))
                                         ^
    pkg/jsonmessage/jsonmessage_test.go:255:4: S1039: unnecessary use of fmt.Sprintf (gosimple)
                fmt.Sprintf("ID: status\n"),
                ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    plugin/v2/plugin.go:141:50: G601: Implicit memory aliasing in for loop. (gosec)
                    updateSettingsEnv(&p.PluginObj.Settings.Env, &s)
                                                                 ^
    libcontainerd/remote/client.go:572:13: G601: Implicit memory aliasing in for loop. (gosec)
                cpDesc = &m
                         ^
    distribution/push_v2.go:400:34: G601: Implicit memory aliasing in for loop. (gosec)
                (metadata.CheckV2MetadataHMAC(&mountCandidate, pd.hmacKey) ||
                                              ^
    builder/dockerfile/builder.go:261:84: G601: Implicit memory aliasing in for loop. (gosec)
            currentCommandIndex = printCommand(b.Stdout, currentCommandIndex, totalCommands, &meta)
                                                                                             ^
    builder/dockerfile/builder.go:278:46: G601: Implicit memory aliasing in for loop. (gosec)
            if err := initializeStage(dispatchRequest, &stage); err != nil {
                                                       ^
    daemon/container.go:283:40: G601: Implicit memory aliasing in for loop. (gosec)
            if err := parser.ValidateMountConfig(&cfg); err != nil {
                                                 ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added 13 commits June 10, 2021 13:03
    daemon/cluster/executor/container/adapter.go:446:42: G601: Implicit memory aliasing in for loop. (gosec)
            req := c.container.volumeCreateRequest(&mount)
                                                   ^
    daemon/network.go:577:10: G601: Implicit memory aliasing in for loop. (gosec)
                np := &n
                      ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    integration/build/build_session_test.go:92:6: func `testBuildWithSession` is unused (unused)
    func testBuildWithSession(t *testing.T, client dclient.APIClient, daemonHost string, dir, dockerfile string) (outStr string) {
         ^
    integration/container/checkpoint_test.go:23:6: func `containerExec` is unused (unused)
    func containerExec(t *testing.T, client client.APIClient, cID string, cmd []string) {
         ^
    integration/network/service_test.go:295:6: func `swarmIngressReady` is unused (unused)
    func swarmIngressReady(client client.NetworkAPIClient) func(log poll.LogT) poll.Result {
         ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…se positive)

Also looks like a false positive, but given that these were basically
testing for the `errdefs.Conflict` and `errdefs.NotFound` interfaces, I
replaced these with those;

    daemon/stats/collector.go:154:6: type `notRunningErr` is unused (unused)
    type notRunningErr interface {
         ^
    daemon/stats/collector.go:159:6: type `notFoundErr` is unused (unused)
    type notFoundErr interface {
         ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    pkg/archive/copy.go:357:16: G110: Potential DoS vulnerability via decompression bomb (gosec)
                if _, err = io.Copy(rebasedTar, srcTar); err != nil {
                            ^

Ignoring GoSec G110. See securego/gosec#433
and https://cure53.de/pentest-report_opa.pdf, which recommends to
replace io.Copy with io.CopyN7. The latter allows to specify the
maximum number of bytes that should be read. By properly defining
the limit, it can be assured that a GZip compression bomb cannot
easily cause a Denial-of-Service.
After reviewing, this should not affect us, because here we do not
read into memory.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/logger/splunk/splunk.go:173:16: G402: TLS MinVersion too low. (gosec)
    	tlsConfig := &tls.Config{}
    	              ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    builder/builder-next/adapters/snapshot/snapshot.go:386:3: if-return: redundant if ...; err != nil check, just return error instead. (revive)
            if err := b.Put(keyIsCommitted, []byte{}); err != nil {
                return err
            }

    plugin/fetch_linux.go:112:2: if-return: redundant if ...; err != nil check, just return error instead. (revive)
        if err := images.Dispatch(ctx, images.Handlers(handlers...), nil, desc); err != nil {
            return err
        }

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Unlike regular comments, nolint comments should not have a leading space.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/config/config_unix.go:92:21: error-strings: error strings should not be capitalized or end with punctuation or a newline (revive)
            return fmt.Errorf("Default cgroup namespace mode (%v) is invalid. Use \"host\" or \"private\".", cm) // nolint: golint
                              ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
    daemon/list.go:556:18: var-declaration: should omit type bool from declaration of var shouldSkip; it will be inferred from the right-hand side (revive)
                shouldSkip    bool = true
                              ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
….SliceHeader

Probably needs a similar change as c208f03,
but this code makes my head spin, so for now suppressing, and created a
tracking issue:

    daemon/graphdriver/graphtest/graphtest_unix.go:305:12: unsafeptr: possible misuse of reflect.SliceHeader (govet)
        header := *(*reflect.SliceHeader)(unsafe.Pointer(&buf))
                  ^
    daemon/graphdriver/graphtest/graphtest_unix.go:308:36: unsafeptr: possible misuse of reflect.SliceHeader (govet)
        data := *(*[]byte)(unsafe.Pointer(&header))
                                          ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The message changed from "is deprecated" to "has been deprecated":

    client/hijack.go:85:16: SA1019: httputil.NewClientConn has been deprecated since Go 1.0: Use the Client or Transport in package net/http instead. (staticcheck)
        clientconn := httputil.NewClientConn(conn, nil)
                      ^
    integration/plugin/authz/authz_plugin_test.go:180:7: SA1019: httputil.NewClientConn has been deprecated since Go 1.0: Use the Client or Transport in package net/http instead. (staticcheck)
        c := httputil.NewClientConn(conn, nil)
             ^
    integration/plugin/authz/authz_plugin_test.go:479:12: SA1019: httputil.NewClientConn has been deprecated since Go 1.0: Use the Client or Transport in package net/http instead. (staticcheck)
        client := httputil.NewClientConn(conn, nil)
                  ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy link
Member

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants