Skip to content

Integration: change signatures to fix golint warnings#39332

Merged
yongtang merged 5 commits intomoby:masterfrom
thaJeztah:fix_golint_again
Jun 7, 2019
Merged

Integration: change signatures to fix golint warnings#39332
yongtang merged 5 commits intomoby:masterfrom
thaJeztah:fix_golint_again

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jun 6, 2019

The nolint directives are not supported by all linters, so fixing these warnings:

docker/integration/internal/network/network.go
Line 30: warning: context.Context should be the first parameter of a function (golint)
docker/integration/internal/container/container.go
Line 25: warning: context.Context should be the first parameter of a function (golint)
Line 44: warning: context.Context should be the first parameter of a function (golint)
Line 52: warning: context.Context should be the first parameter of a function (golint)
Line 59: warning: context.Context should be the first parameter of a function (golint)
docker/integration/network/delete_test.go
Line 30: warning: context.Context should be the first parameter of a function (golint)
docker/integration/plugin/graphdriver/external_test.go
Line 441: warning: context.Context should be the first parameter of a function (golint)

@thaJeztah
Copy link
Member Author

thaJeztah commented Jun 6, 2019

ping @vdemeester @yongtang ptal

vdemeester
vdemeester approved these changes Jun 6, 2019
View reviewed changes
Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

So, the ctx vs t 👼 t is supposed to be the first argument, and ctx as well. Seems like most of the linter prefer ctx than t, so I am ok with that change.

LGTM

vdemeester
vdemeester reviewed Jun 6, 2019
View reviewed changes
thaJeztah added 5 commits June 7, 2019 13:03
@thaJeztah
integration: change network.CreateNoError signature to fix linting
caec45a 
    Line 30: warning: context.Context should be the first parameter of a function (golint)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
integration: change container.Create signature to fix linting
b4c46b0 
```
Line 25: warning: context.Context should be the first parameter of a function (golint)
Line 44: warning: context.Context should be the first parameter of a function (golint)
Line 52: warning: context.Context should be the first parameter of a function (golint)
```

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
integration: change container.Run signature to fix linting
9f9b429 
    Line 59: warning: context.Context should be the first parameter of a function (golint)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
integration: change createAmbiguousNetworks signature to fix linting
123e29f 
    Line 30: warning: context.Context should be the first parameter of a function (golint)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
integration: change testGraphDriver signature to fix linting
dac5710 
    Line 441: warning: context.Context should be the first parameter of a function (golint)

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Member Author

thaJeztah commented Jun 7, 2019

So, the ctx vs t 👼 t is supposed to be the first argument, and ctx as well. Seems like most of the linter prefer ctx than t, so I am ok with that change.

Yes that was the reason I tried to fix it by nolint: comments, but looks indeed like most linters care more about ctx being first 🤷‍♂

I fixed the ...'s, thanks!

@thaJeztah
Copy link
Member Author

thaJeztah commented Jun 7, 2019

argh; did I forget to stage something?

@thaJeztah
Copy link
Member Author

thaJeztah commented Jun 7, 2019

oh, never mind; browser cache LOL

@thaJeztah
Copy link
Member Author

thaJeztah commented Jun 7, 2019

Interesting; looks like this test is still flaky;

11:47:41 FAIL: docker_cli_ps_test.go:439: DockerSuite.TestPsListContainersFilterExited
11:47:41 
11:47:41 docker_cli_ps_test.go:455:
11:47:41     c.Assert(out, checker.Contains, strings.TrimSpace(firstZero))
11:47:41 ... obtained string = "2c9302a80ba2c6338f096846278736d4a756b0d27650d0a14f3695a5e3a2d880\n"
11:47:41 ... substring string = "479490eb78fcd1ba7a9ca666c4a0fa1956557df3f0a9802e4e67a1f0c40e3a59"

yongtang
yongtang approved these changes Jun 7, 2019
View reviewed changes
Copy link
Member

@yongtang yongtang left a comment

Choose a reason for hiding this comment

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

LGTM. It might make more sense for golint to allow t to be the first in test setup. But it is ok to go either way I think.

@codecov
Copy link

codecov bot commented Jun 7, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@1d5748d). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #39332   +/-   ##
=========================================
  Coverage          ?   37.05%           
=========================================
  Files             ?      611           
  Lines             ?    45528           
  Branches          ?        0           
=========================================
  Hits              ?    16869           
  Misses            ?    26373           
  Partials          ?     2286

@yongtang yongtang merged commit b10cd5c into moby:master Jun 7, 2019
@thaJeztah thaJeztah deleted the fix_golint_again branch June 7, 2019 18:25
This was referenced Jun 12, 2019
Merged
Merged
This was referenced Jun 14, 2019
Merged
Merged
@thaJeztah thaJeztah mentioned this pull request Jul 16, 2019
Open
@thaJeztah thaJeztah mentioned this pull request Oct 8, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Oct 23, 2019
Closed
@thaJeztah thaJeztah added this to the 20.03.0 milestone Apr 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@vdemeester vdemeester vdemeester approved these changes

@yongtang yongtang yongtang approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/testing process/cherry-picked status/4-merge

Projects

None yet

Milestone

20.10.0

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah @vdemeester @yongtang @GordonTheTurtle