Fix local imports, add appropriate linter #2962

kolyshkin · 2021-10-18T19:59:10Z

@Creatone points out in #2959 comments that imports are not properly grouped.

This PR

Fixes a few imports that goimports chokes on (first 2 commits).
Modifies Makefile's format target to format imports in three groups:
- standard library packages;
- third-party packages;
- local packages (i.e. ones with the github.com/google/cadvisor prefix).
Note goimports does the same thing as gofmt, plus imports sorting.
It can't do what gofmt -s does, but it was not done before (although
it is checked according to .golangci.yml), so let's leave it as is.
Adds goimports linter with proper configuration to check imports
formatting in CI. At this step we have checked that the linter does complain.
Actually implements these changes by running make format.
The linter no longer complains.

Please review commit-by-commit.

k8s-ci-robot · 2021-10-18T19:59:19Z

Hi @kolyshkin. Thanks for your PR.

I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kolyshkin · 2021-10-18T20:02:39Z

The steps this commit was made are:

add goimports with proper config to .golangci.yml, run golangci-lint to check that the linter generates warings;
modify format target in Makefile; run it;
re-run golangci-lint to see the warnings are gone.

bobbypage · 2021-10-18T20:07:49Z

/ok-to-test

kolyshkin · 2021-10-18T21:16:44Z

Going to redo this in a cleaner way

kolyshkin · 2021-10-18T21:57:25Z

/retest

kolyshkin · 2021-10-18T21:58:36Z

No longer a draft. @bobbypage can you please re-trigger the e2e ci job?

bobbypage · 2021-10-18T22:00:52Z

/retest

bobbypage · 2021-10-18T22:35:04Z

sorry for conflict, can you please rebase?

This is required because goimports@v0.1.7 fails to recognize this import, adding a duplicated one. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

This is required since goimports@v0.1.7 can not recognize that import "github.com/mesos/mesos-go/api/v1/lib" gives the name "mesos" to the imported package (frankly, I can't do that either). The patch is generated by git grep -l '"github.com/mesos/mesos-go/api/v1/lib"' | \ xargs sed -i 's|"github.com/mesos/mesos-go/api/v1/lib"|mesos \0|' Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2021-10-18T22:51:10Z

sorry for conflict, can you please rebase?

NP; done

Makefile

kolyshkin · 2021-10-19T01:50:47Z

@bobbypage PTAL; need to re-run e2e test again; apparently I can't initiate it.

bobbypage · 2021-10-19T02:04:21Z

/retest

bobbypage · 2021-10-19T04:59:01Z

/retest

bobbypage · 2021-10-20T00:35:49Z

hmm, now the prow presubmit is failing with

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/google_cadvisor/2962/pull-cadvisor-e2e/1450618281479114752/

W1020 00:24:33.424] + make all
W1020 00:24:33.473] docker: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?.
W1020 00:24:33.473] See 'docker run --help'.

I think it's because it's using COS (https://cloud.google.com/container-optimized-os/docs) as OS image there, and docker runs to be run under sudo in COS...

Not sure on best solution, maybe we can check if we need to use sudo on docker or not? Or maybe alternatively we can install the golant-ci-lint instead of the whole docker run solution with go get, so it will be available in the prow job?

kolyshkin · 2021-10-20T01:42:20Z

I think it's because it's using COS (https://cloud.google.com/container-optimized-os/docs) as OS image there, and docker runs to be run under sudo in COS...

The doc (https://cloud.google.com/container-optimized-os/docs/how-to/run-container-instance) does not tell anything about sudo, and uses $ in examples which makes me think sudo is not required...

I have added a debug commit, let's see what it shows.

Or maybe alternatively we can install the golant-ci-lint instead of the whole docker run solution with go get, so it will be available in the prow job?

Surely I can do that, too.

kolyshkin · 2021-10-20T01:46:08Z

My debug don't reveal anything, and alas I know nothing about prow or COS.

W1020 01:44:01.049] + systemctl status docker
W1020 01:44:01.050] /bin/sh: 7: systemctl: not found
W1020 01:44:01.050] + docker version
W1020 01:44:01.108] Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?
W1020 01:44:01.112] + docker run --rm -v /go/src/github.com/google/cadvisor:/pkg -w /pkg golangci/golangci-lint:v1.42.1 golangci-lint run
W1020 01:44:01.156] docker: Cannot connect to the Docker daemon at unix:///var/run/docker.sock. Is the docker daemon running?.
W1020 01:44:01.156] See 'docker run --help'.
W1020 01:44:01.161] make: *** [Makefile:94: lint] Error 125

As much as I don't like shortcuts, it seems the best option is to install golangci-lint if not available. I'll add it to Makefile.

Changes from v1 are: - Adds GOBIN to the PATH - Proxy Support - stable input - Bug Fixes (including issues around version matching and semver) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

Check if golangci-lint is available locally -- if not, install it to GOPATH/bin. This helps with cases when golangci-lint is not available on the host (such as in prow). As we now have golangci-lint installation in Makefile, remove the code installing it from .github/workflows/test.yml for better maintainability. While at it, add lint to .PHONY targets. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2021-10-20T02:25:29Z

Redid with golangci-lint being auto-installed from Makefile if absent. Apparently, since now test CI job runs make presubmit, which runs golangci-lint, the whole lint job is no longer required, so I have removed it 👍🏻

This means that after this PR is merged, you need to go to https://github.com/google/cadvisor/settings/branches, find "Branch protection rules" and edit those to remove lint jobs from the list of required ones. Now lint is part of test.

kolyshkin · 2021-10-20T02:26:40Z

This means that after this PR is merged, you need to go to https://github.com/google/cadvisor/settings/branches, find "Branch protection rules" and edit those to remove lint jobs from the list of required ones. Now lint is part of test.

In fact you have to do it before the merge, because apparently GHA CI is run using config from the PR (rather than the master branch).

kolyshkin · 2021-10-20T02:27:15Z

@bobbypage it's finally ready 🥵

kolyshkin · 2021-10-20T02:30:22Z

@Creatone this "let's fix those imports" remark resulted in much more work that I had anticipated 😃

Creatone · 2021-10-20T12:26:09Z

@kolyshkin I meant fix only imports for changed files :D

Thanks for your contribution!

kolyshkin · 2021-10-20T16:28:57Z

@bobbypage PTAL 🙏🏻 (the "missing" checks are removed, see #2962 (comment))

bobbypage · 2021-10-21T00:08:13Z

Big thank you @kolyshkin for all your work here and cleanup in process.

I've removed the lint github action checks as you suggested since they're no longer necessary.

LGTM!

bobbypage · 2021-10-21T00:10:43Z

One last comment - #2962 (comment). Please let me know if there is any changes needed there that we can simplify and we can merge this!

1. Modify Makefile's format target to format imports in three groups: - standard library packages; - third-party packages; - local packages (i.e. ones with the `github.com/google/cadvisor` prefix). Note goimports does the same job as gofmt, plus imports sorting. It can't do what gofmt -s does, but it was not done before (although this is checked according to .golangci.yml), so let's leave it as is. Note we don't have to use $(pkgs) and $(cmd_pkgs) in Makefile's format target, as goimports works recursively given the current directory, and it also skips vendor dir. 2. Add goimports linter with proper configuration to check imports formatting in CI. Check that the linter complains. 3. Actually implements these changes by running "make format". Check that the linter no longer complains. 4. Fix presubmit target to make sure it checks the new formatting rules. Instead of modifying build/check_gofmt.sh, let's use golangci-lint which already does this check (and more). While at it, let's remove vet target, as lint (golangci-lint) already calls it. Now, since make presumbit is already there in test job, the whole lint job is superseded by it, so let's remove it. While doing it, make sure we use the code from lint job to run make presubmit, with the following two exceptions: - "set -e" is not required, as it is set by GHA CI; - GOFLAGS="$GO_FLAGS" is not required, as it is done by Makefile. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin · 2021-10-21T00:33:02Z

This one is ready I think.

I found another semi-related issue though -- since ./cmd is totally separate from the golang POV, golangci-lint is not working on it. The consequences in context of this PR is, imports formatting is not checked under ./cmd. I have checked though that goimports linter is not complaining when run on ./cmd.

As this is definitely not an issue with this PR, I am going to address this separately.

bobbypage · 2021-10-21T00:34:09Z

perfect, thanks again @kolyshkin!

google-cla bot added the cla: yes label Oct 18, 2021

k8s-ci-robot added the needs-ok-to-test label Oct 18, 2021

kolyshkin mentioned this pull request Oct 18, 2021

cgroup cleanups #2959

Merged

k8s-ci-robot added ok-to-test and removed needs-ok-to-test labels Oct 18, 2021

kolyshkin force-pushed the imports branch 2 times, most recently from 057aa73 to 108705a Compare October 18, 2021 20:32

kolyshkin marked this pull request as draft October 18, 2021 21:16

kolyshkin force-pushed the imports branch 2 times, most recently from 4670a26 to 9003f48 Compare October 18, 2021 21:46

kolyshkin marked this pull request as ready for review October 18, 2021 21:57

kolyshkin added 2 commits October 18, 2021 15:46

manager: fix v2 import

10830d7

This is required because goimports@v0.1.7 fails to recognize this import, adding a duplicated one. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the imports branch from 9003f48 to 6d7298e Compare October 18, 2021 22:51

bobbypage reviewed Oct 18, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

bobbypage reviewed Oct 18, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

bobbypage reviewed Oct 18, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

kolyshkin force-pushed the imports branch 2 times, most recently from 94cbb3c to 1ef61c2 Compare October 19, 2021 01:46

kolyshkin added 2 commits October 19, 2021 19:14

ci: bump actions/setup-go to v2

a4f66a3

Changes from v1 are: - Adds GOBIN to the PATH - Proxy Support - stable input - Bug Fixes (including issues around version matching and semver) Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

kolyshkin force-pushed the imports branch from edf4862 to 0910abd Compare October 20, 2021 02:21

kolyshkin force-pushed the imports branch from 0910abd to b36394b Compare October 21, 2021 00:27

bobbypage approved these changes Oct 21, 2021

View reviewed changes

bobbypage merged commit d6b0ddb into google:master Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix local imports, add appropriate linter #2962

Fix local imports, add appropriate linter #2962

kolyshkin commented Oct 18, 2021 •

edited

Loading

k8s-ci-robot commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

bobbypage commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

bobbypage commented Oct 18, 2021

bobbypage commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

kolyshkin commented Oct 19, 2021

bobbypage commented Oct 19, 2021

bobbypage commented Oct 19, 2021

bobbypage commented Oct 20, 2021 •

edited

Loading

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

Creatone commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

bobbypage commented Oct 21, 2021

bobbypage commented Oct 21, 2021

kolyshkin commented Oct 21, 2021

bobbypage commented Oct 21, 2021

Fix local imports, add appropriate linter #2962

Fix local imports, add appropriate linter #2962

Conversation

kolyshkin commented Oct 18, 2021 • edited Loading

k8s-ci-robot commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

bobbypage commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

bobbypage commented Oct 18, 2021

bobbypage commented Oct 18, 2021

kolyshkin commented Oct 18, 2021

kolyshkin commented Oct 19, 2021

bobbypage commented Oct 19, 2021

bobbypage commented Oct 19, 2021

bobbypage commented Oct 20, 2021 • edited Loading

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

Creatone commented Oct 20, 2021

kolyshkin commented Oct 20, 2021

bobbypage commented Oct 21, 2021

bobbypage commented Oct 21, 2021

kolyshkin commented Oct 21, 2021

bobbypage commented Oct 21, 2021

kolyshkin commented Oct 18, 2021 •

edited

Loading

bobbypage commented Oct 20, 2021 •

edited

Loading