USHIFT-1031: fix golangci-lint reported issues by copejon · Pull Request #1605 · openshift/microshift

copejon · 2023-03-31T20:45:12Z

Which issue(s) this PR addresses:

This PR applies fixes to errors caught by golangci-lint, or applies ignore directives when applicable.

openshift-ci-robot · 2023-03-31T20:45:15Z

@copejon: This pull request references USHIFT-1031 which is a valid jira issue.

Details

In response to this:

Which issue(s) this PR addresses:

USHIFT-1031

This PR applies fixes to errors caught by golangci-lint, or applies ignore directives when applicable.

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.

openshift-ci-robot · 2023-03-31T20:45:19Z

@copejon: This pull request references USHIFT-1031 which is a valid jira issue.

Details

In response to this:

Which issue(s) this PR addresses:

USHIFT-1031

This PR applies fixes to errors caught by golangci-lint, or applies ignore directives when applicable.

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.

copejon · 2023-03-31T20:45:38Z

/cherry-pick release-4.13

openshift-cherrypick-robot · 2023-03-31T20:45:40Z

@copejon: once the present PR merges, I will cherry-pick it on top of release-4.13 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-4.13

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.

dhellmann

Most of these changes look like things we want.

I don't see a change to enable the linter in the Makefile, and I think we'll want that as part of this PR so we don't end up regressing.

Since the PR touches so many files, I'm hoping we can land the configuration refactoring in #1442 before this. Or at least not change the config package in this PR so I don't have to rebase again.

copejon · 2023-04-03T14:00:04Z

Most of these changes look like things we want.

I don't see a change to enable the linter in the Makefile, and I think we'll want that as part of this PR so we don't end up regressing.

Thanks, that's a good point. Will add.

Since the PR touches so many files, I'm hoping we can land the configuration refactoring in #1442 before this. Or at least not change the config package in this PR so I don't have to rebase again.

I'm fine keeping this one on hold until then.
/hold

pkg/config/config.go

pkg/controllers/kube-apiserver.go

pkg/controllers/openshift-crd-manager.go

pkg/controllers/openshift-route-controller-manager.go

pkg/mdns/controller.go

pmtk · 2023-04-03T15:11:09Z

pkg/util/cryptomaterial/certchains/chainsbuilder_test.go

Looks like 1st usage of go generics in microshift? :)

pkg/util/cryptomaterial/certchains/signers.go

pkg/util/cryptomaterial/trustupdates.go

.golangci.yaml

eggfoobar

Just a small suggestion on order

pkg/assets/crd.go

pkg/components/components.go

pkg/controllers/infra-services-controller.go

pkg/assets/crd.go

copejon · 2023-04-06T20:38:13Z

/retest

ggiguash · 2023-04-10T05:42:25Z

/retest

copejon · 2023-04-10T18:50:36Z

Failure is due to the golangci-lint trying and failing to create a .cache dir in the user's home dir, which doesn't exist. I'm working on a fix in CI to override the default cache location.

copejon · 2023-04-13T16:57:42Z

/test verify

ggiguash · 2023-04-14T07:28:31Z

/retest

ggiguash · 2023-04-14T08:20:43Z

@copejon , looks like the Verify test requires some fixes in the code

path_prefixer: 2/2, cgo: 57/57, uniq_by_line: 2/2, source_code: 2/2, severity-rules: 2/2, path_prettifier: 57/57, skip_dirs: 57/57, autogenerated_exclude: 56/57, nolint: 2/4, path_shortener: 2/2, fixer: 2/2, sort_results: 2/2, identifier_marker: 56/56, exclude: 56/56, max_per_file_from_linter: 2/2"
level=info msg="[runner] processing took 7.378718ms with stages: nolint: 3.187383ms, autogenerated_exclude: 1.096068ms, identifier_marker: 1.044983ms, path_prettifier: 934.479µs, exclude-rules: 711.378µs, skip_dirs: 161.01µs, source_code: 91.681µs, cgo: 76.37µs, filename_unadjuster: 41.556µs, max_from_linter: 12.311µs, uniq_by_line: 9.266µs, max_same_issues: 5.412µs, path_shortener: 1.979µs, skip_files: 1.176µs, max_per_file_from_linter: 831ns, exclude: 692ns, diff: 569ns, fixer: 488ns, sort_results: 442ns, severity-rules: 427ns, path_prefixer: 217ns"
level=info msg="[runner] linters took 2m19.546670502s with stages: goanalysis_metalinter: 2m19.539099305s"
pkg/cmd/init_test.go:147:11: Error return value of `os.Mkdir` is not checked (errcheck)
		os.Mkdir(filepath.Join(rootDir, dir), 0600)
		        ^
pkg/cmd/init_test.go:154:25: Error return value is not checked (errcheck)
	cleanupStaleKubeconfigs(cfg, rootDir)
	                       ^

fzdarsky · 2023-04-14T08:43:15Z

pkg/config/config.go

Flipping this condition around will cause problems when more validations get added later.

Fair point. I've reverted the change and added a linter skip tag.

I don't see a linter skip tag here. Was a change lost in a rebase?

fzdarsky · 2023-04-14T08:49:11Z

pkg/mdns/controller.go

copejon · 2023-04-14T19:27:49Z

/retest

copejon · 2023-04-17T14:05:01Z

/test verify

copejon · 2023-04-17T20:09:09Z

The linter is satisfied, however the vulnerability check found 4 items and is failing the verify test.

.golangci.yaml

pkg/controllers/openshift-route-controller-manager.go

pkg/util/cryptomaterial/certchains/signers.go

dhellmann · 2023-04-17T20:22:20Z

pkg/config/config.go

I don't see a linter skip tag here. Was a change lost in a rebase?

dhellmann · 2023-04-17T20:27:20Z

pkg/cmd/run.go

Should this be the context that everything else is using to know when to exit?

Moved the runCtx declaration above the AddServer chunk so it can be passed down to services

Signed-off-by: Jon Cope <jcope@redhat.com>

use Set generics over deprecated Set types

…hey are always nil

Signed-off-by: Jon Cope <jcope@redhat.com> the presence of a vendor dir causes install to place the bin outside of PATH, forcing it to behave as a module fixes this Signed-off-by: Jon Cope <jcope@redhat.com> post-rebase fixes. These include err checking, nested ifs, empty newlines, and a couple ctx passing errors

…d up linter fixes b/c of it, instead follow up in another PR Signed-off-by: Jon Cope <jcope@redhat.com>

…to 3m Signed-off-by: Jon Cope <jcope@redhat.com>

Signed-off-by: Jon Cope <jcope@redhat.com>

… make vendor-etcd

dhellmann · 2023-04-18T18:56:13Z

This looks good. I set the merge method to squash, but you can remove that if you want. Remove the hold when you're ready.

/lgtm

openshift-ci · 2023-04-18T18:57:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: copejon, dhellmann

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [copejon,dhellmann]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

copejon · 2023-04-18T19:00:57Z

/unhold

openshift-cherrypick-robot · 2023-04-18T19:14:25Z

@copejon: #1605 failed to apply on top of branch "release-4.13":

Applying: sets.String is deprecated, use the new generic call instead
Using index info to reconstruct a base tree...
A	.golangci.yaml
M	pkg/components/render_test.go
M	pkg/controllers/cluster-policy-controller.go
M	pkg/controllers/etcd.go
M	pkg/controllers/infra-services-controller.go
M	pkg/controllers/kube-apiserver.go
M	pkg/controllers/kube-scheduler.go
M	pkg/controllers/openshift-crd-manager.go
M	pkg/controllers/version.go
M	pkg/loadbalancerservice/controller.go
M	pkg/mdns/controller.go
M	pkg/util/net.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/util/net.go
CONFLICT (content): Merge conflict in pkg/util/net.go
Auto-merging pkg/mdns/controller.go
Auto-merging pkg/loadbalancerservice/controller.go
Auto-merging pkg/controllers/version.go
Auto-merging pkg/controllers/openshift-crd-manager.go
Auto-merging pkg/controllers/kube-scheduler.go
Auto-merging pkg/controllers/kube-apiserver.go
CONFLICT (content): Merge conflict in pkg/controllers/kube-apiserver.go
Auto-merging pkg/controllers/infra-services-controller.go
Auto-merging pkg/controllers/etcd.go
Auto-merging pkg/controllers/cluster-policy-controller.go
Auto-merging pkg/components/render_test.go
CONFLICT (modify/delete): .golangci.yaml deleted in HEAD and modified in sets.String is deprecated, use the new generic call instead. Version sets.String is deprecated, use the new generic call instead of .golangci.yaml left in tree.
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 sets.String is deprecated, use the new generic call instead
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Details

In response to this:

/cherry-pick release-4.13

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.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 31, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2023

openshift-ci bot requested review from dhellmann and ggiguash March 31, 2023 20:45

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 31, 2023

copejon force-pushed the ushift-1031 branch from 6f40141 to 8e78563 Compare March 31, 2023 20:49

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 31, 2023

dhellmann reviewed Apr 2, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 3, 2023

pmtk reviewed Apr 3, 2023

View reviewed changes

copejon force-pushed the ushift-1031 branch 3 times, most recently from 848df23 to 8019687 Compare April 3, 2023 21:50

eggfoobar reviewed Apr 5, 2023

View reviewed changes

pkg/assets/crd.go Outdated Show resolved Hide resolved

pkg/components/components.go Outdated Show resolved Hide resolved

pkg/controllers/infra-services-controller.go Outdated Show resolved Hide resolved

pkg/assets/crd.go Outdated Show resolved Hide resolved

copejon force-pushed the ushift-1031 branch from b8e7fa6 to 0159b41 Compare April 5, 2023 19:29

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 5, 2023

copejon force-pushed the ushift-1031 branch 2 times, most recently from 5fb9a60 to eb78cce Compare April 6, 2023 19:01

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 6, 2023

copejon force-pushed the ushift-1031 branch from 69e28a0 to b1797b9 Compare April 10, 2023 16:05

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 10, 2023

copejon mentioned this pull request Apr 10, 2023

MicroShift - set golangci-lint explicitly openshift/release#38194

Merged

fzdarsky reviewed Apr 14, 2023

View reviewed changes

copejon force-pushed the ushift-1031 branch from bc0c658 to 33c6dd8 Compare April 17, 2023 15:09

dhellmann reviewed Apr 17, 2023

View reviewed changes

copejon force-pushed the ushift-1031 branch from 020eb3c to 12a9ec3 Compare April 17, 2023 20:57

copejon added 5 commits April 17, 2023 16:43

sets.String is deprecated, use the new generic call instead

37606af

detangle nested ifs

084fbde

Signed-off-by: Jon Cope <jcope@redhat.com>

plumb context through kube client callers

68f74f8

use Set generics over deprecated Set types

More err checking. some errors are ignored by convention or because t…

b7495be

…hey are always nil

copejon force-pushed the ushift-1031 branch from 12a9ec3 to 074d445 Compare April 17, 2023 21:57

copejon and others added 4 commits April 18, 2023 09:37

govuln is throwing errors and is out of scope for this pr. do not hol…

16a0a15

…d up linter fixes b/c of it, instead follow up in another PR Signed-off-by: Jon Cope <jcope@redhat.com>

linter validation is timing out in CI so increase it from 1m default …

9990e94

…to 3m Signed-off-by: Jon Cope <jcope@redhat.com>

still timing out, bump limit to 20m

3569953

Signed-off-by: Jon Cope <jcope@redhat.com>

verify check crash and is complaining that etcd must be vendored, ran…

45a6bb4

… make vendor-etcd

dhellmann added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 18, 2023

openshift-ci bot assigned dhellmann Apr 18, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 18, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 18, 2023

openshift-merge-robot merged commit 6e88dfc into openshift:main Apr 18, 2023

copejon deleted the ushift-1031 branch April 19, 2023 15:22

Conversation

copejon commented Mar 31, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 31, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 31, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

copejon commented Mar 31, 2023

Uh oh!

openshift-cherrypick-robot commented Mar 31, 2023

Uh oh!

dhellmann left a comment

Choose a reason for hiding this comment

Uh oh!

copejon commented Apr 3, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eggfoobar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

copejon commented Apr 6, 2023

Uh oh!

ggiguash commented Apr 10, 2023

Uh oh!

copejon commented Apr 10, 2023

Uh oh!

copejon commented Apr 13, 2023

Uh oh!

ggiguash commented Apr 14, 2023

Uh oh!

ggiguash commented Apr 14, 2023

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!

copejon commented Apr 14, 2023

Uh oh!

copejon commented Apr 17, 2023

Uh oh!

copejon commented Apr 17, 2023

Uh oh!

Uh oh!

Uh oh!

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!

dhellmann commented Apr 18, 2023

Uh oh!

openshift-ci bot commented Apr 18, 2023

Uh oh!

copejon commented Apr 18, 2023

Uh oh!

openshift-cherrypick-robot commented Apr 18, 2023

Uh oh!

Reviewers

copejon commented Mar 31, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 31, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 31, 2023 •

edited by openshift-ci bot

Loading