enable linting and bingo management of the lint tool #1524

grokspawn · 2024-12-20T21:33:41Z

Discovered that the lint make target doesn't lint anything. It only checks whether go imports are in expected grouping/ordering.

This PR introduces gci support (replacing goimports) as a targeted linter, and implements golang-linter support via a bingo-managed version.

There are discrete commits to topically collect:

adding lint infrastructure
automated lint fixes
manual test lint fixes
manual functional lint fixes
bingo update of golangci-lint version
review updates

The parent issue has a link to a google sheet which includes a list of every single update made here, categorized and broken down by reporting linter.

The ONLY FOCUS of this PR is to establish a "bright line" beyond which no new reports will slip through the cracks, but due to the age (and the "feature completeness") of much of the reported code, there are a LOT of issues which are specifically and locally disabled to remove them from lint reports without making updates.

I'd initially felt sanguine about updating style changes which modified (read: broke) APIs, but I backed away from this after seeing how prevalent those changes would need to be.

The only included case of a stylistic breaking API change was the JSONUnmarshaller, since this is a recent addition that was added for internal use, so it seemed a safe bet to not break anyone's access.

I was initially much more willing to make updates to test code to bring them up to date, but

again, the scope of this was soul-destroying; and
I started encountering some unit test failures because the tests weren't seamlessly constructed. These were of the nature of "called twice" or "doesn't need to be called" failure scenarios, and I just restored the original flows and disabled the specific linter in that specific scenario

Description of the change:

Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

codecov · 2024-12-20T21:42:40Z

Codecov Report

Attention: Patch coverage is 60.54591% with 159 lines in your changes missing coverage. Please review.

Project coverage is 46.80%. Comparing base (81056f9) to head (ac872de).
Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/sqlite/query.go	63.04%	13 Missing and 4 partials ⚠️
pkg/mirror/options.go	0.00%	14 Missing ⚠️
pkg/lib/bundle/validate.go	7.14%	11 Missing and 2 partials ⚠️
pkg/containertools/runner.go	0.00%	12 Missing ⚠️
pkg/sqlite/load.go	65.62%	11 Missing ⚠️
pkg/containertools/containertool.go	0.00%	9 Missing ⚠️
pkg/registry/csv.go	63.63%	4 Missing and 4 partials ⚠️
pkg/sqlite/migrations/003_required_apis.go	61.11%	2 Missing and 5 partials ⚠️
pkg/client/kubeclient.go	0.00%	6 Missing ⚠️
pkg/registry/empty.go	0.00%	6 Missing ⚠️
... and 28 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1524      +/-   ##
==========================================
+ Coverage   46.76%   46.80%   +0.04%     
==========================================
  Files         135      135              
  Lines       15776    15873      +97     
==========================================
+ Hits         7377     7430      +53     
- Misses       7358     7402      +44     
  Partials     1041     1041

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

perdasilva · 2025-02-06T13:58:51Z

.golangci.yaml

+  # Default timeout is 1m, up to give more room
+  timeout: 4m
+
+linters:


you don't have to, but if it's easy enough, we could add govet here as well (then maybe drop it from the Makefile if it's there)

the .golangci.yaml file in this project is a copy of that in op-con. Since that one has just updated, I can take that change, but IMO we can update the Makefile in a separate PR. This one is already way beyond any reasonable complexity.

Hi @grokspawn

You are with an outdated version :-P
See that we added govet there and we cleanup the staff: operator-framework/operator-controller@099a6cf

But I am ok with we move forward as it is and add govet in a follow up.
It is fine for me too. @perdasilva wdyt?

I did add govet in ac872deec7a3f69452cdf7d1cdcc38b7dec70911

.golangci.yaml

alpha/action/migrate_test.go

perdasilva · 2025-02-06T14:02:09Z

alpha/declcfg/write_test.go

@@ -500,13 +500,13 @@ func removeJSONWhitespace(cfg *DeclarativeConfig) {
 	for ib := range cfg.Bundles {
 		for ip := range cfg.Bundles[ib].Properties {
 			var buf bytes.Buffer
-			json.Compact(&buf, cfg.Bundles[ib].Properties[ip].Value)
+			_ = json.Compact(&buf, cfg.Bundles[ib].Properties[ip].Value)


maybe also require.NoError ?

No test object available in this scope, or I would've. 🤷

perdasilva · 2025-02-06T14:02:17Z

alpha/declcfg/write_test.go

 			cfg.Bundles[ib].Properties[ip].Value = buf.Bytes()
 		}
 	}
 	for io := range cfg.Others {
 		var buf bytes.Buffer
-		json.Compact(&buf, cfg.Others[io].Blob)
+		_ = json.Compact(&buf, cfg.Others[io].Blob)


Same problem as pre-ditto.

perdasilva · 2025-02-06T14:06:38Z

test/e2e/bundle_image_test.go

@@ -276,7 +276,7 @@ func pushLoadImages(client *kubernetes.Clientset, w io.Writer, images ...string)
 		}
 	} else {
 		for _, image := range images {
-			pushWith("docker", image)
+			err = pushWith("docker", image)


nit:

if err := pushWith("docker", image); err != nil { return err }

err is declared in line 262. Is there some benefit to re-defining it in this scope? (I'd missed this)

Signed-off-by: Jordan Keister <[email protected]>

Signed-off-by: Jordan <[email protected]>

- errcheck - stylecheck - nestif - prealloc - errorlint - nonamedreturns - gosec - gosimple - govet - importas - staticcheck - unused Signed-off-by: Jordan Keister <[email protected]>

Signed-off-by: Jordan Keister <[email protected]>

azych

That's great and I completely agree with your decision to back out of actually fixing those lint issues in favor of silencing most of them just to establish a baseline given the already significant scope of work - I'm sure people can be good scouts and gradually make fixes as they touch upon different areas of the code they work on.

I didn't notice anything critical silenced, so if it passes CI, it's good to me.

camilamacedo86

/lgtm

🚀

openshift-ci · 2025-02-10T17:15:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: azych, camilamacedo86, grokspawn

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [grokspawn]

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

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 20, 2024

openshift-ci bot requested review from ankitathomas and everettraven December 20, 2024 21:33

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 20, 2024

grokspawn force-pushed the add-linting branch 2 times, most recently from e3d597f to 2a7814d Compare December 20, 2024 21:38

grokspawn force-pushed the add-linting branch from 2a7814d to b044ec9 Compare December 20, 2024 21:48

grokspawn mentioned this pull request Dec 21, 2024

Add goimports as a managed tool via bingo #1523

Closed

5 tasks

grokspawn force-pushed the add-linting branch 2 times, most recently from e831f94 to 3b764ff Compare January 2, 2025 16:24

grokspawn mentioned this pull request Jan 6, 2025

lint make target... doesn't lint #1525

Closed

grokspawn force-pushed the add-linting branch 4 times, most recently from ef05a4a to c9deb72 Compare January 9, 2025 15:39

perdasilva reviewed Feb 6, 2025

View reviewed changes

.golangci.yaml Show resolved Hide resolved

perdasilva reviewed Feb 6, 2025

View reviewed changes

alpha/action/migrate_test.go Outdated Show resolved Hide resolved

perdasilva reviewed Feb 6, 2025

View reviewed changes

grokspawn force-pushed the add-linting branch 7 times, most recently from e223708 to edc24a7 Compare February 7, 2025 17:30

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2025

grokspawn added 3 commits February 7, 2025 11:31

enable linting and bingo management of the lint tool

4e9cbe1

Signed-off-by: Jordan Keister <[email protected]>

automated lint fixes

424c4f0

Signed-off-by: Jordan Keister <[email protected]>

manual test fixes

f1100b1

Signed-off-by: Jordan <[email protected]>

grokspawn force-pushed the add-linting branch from edc24a7 to 8480d85 Compare February 7, 2025 17:34

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2025

grokspawn force-pushed the add-linting branch from 8480d85 to 2e6a90b Compare February 7, 2025 18:30

grokspawn added 2 commits February 7, 2025 12:53

linter feature code

94c8ea2

- errcheck - stylecheck - nestif - prealloc - errorlint - nonamedreturns - gosec - gosimple - govet - importas - staticcheck - unused Signed-off-by: Jordan Keister <[email protected]>

update bingo to work with go version stampede

92f5b29

Signed-off-by: Jordan Keister <[email protected]>

grokspawn force-pushed the add-linting branch from 2e6a90b to 92f5b29 Compare February 7, 2025 18:53

review revisions

ac872de

Signed-off-by: Jordan Keister <[email protected]>

grokspawn changed the title ~~WIP: enable linting and bingo management of the lint tool~~ enable linting and bingo management of the lint tool Feb 7, 2025

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 7, 2025

azych approved these changes Feb 10, 2025

View reviewed changes

camilamacedo86 approved these changes Feb 10, 2025

View reviewed changes

openshift-ci bot assigned camilamacedo86 Feb 10, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 10, 2025

openshift-merge-bot bot merged commit 5531412 into operator-framework:master Feb 10, 2025
11 of 12 checks passed

grokspawn deleted the add-linting branch February 10, 2025 19:43

github-actions bot mentioned this pull request Feb 27, 2025

[operator-framework/operator-registry] v1.51.0 vdemeester/chapeau-rouge#226

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

enable linting and bingo management of the lint tool #1524

enable linting and bingo management of the lint tool #1524

grokspawn commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

perdasilva Feb 6, 2025

grokspawn Feb 7, 2025

camilamacedo86 Feb 7, 2025

camilamacedo86 Feb 7, 2025

grokspawn Feb 7, 2025

perdasilva Feb 6, 2025

grokspawn Feb 7, 2025

perdasilva Feb 6, 2025

grokspawn Feb 7, 2025

perdasilva Feb 6, 2025

grokspawn Feb 7, 2025

azych left a comment •

edited

Loading

camilamacedo86 left a comment

openshift-ci bot commented Feb 10, 2025

enable linting and bingo management of the lint tool #1524

enable linting and bingo management of the lint tool #1524

Conversation

grokspawn commented Dec 20, 2024 • edited Loading

codecov bot commented Dec 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azych left a comment • edited Loading

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 10, 2025

grokspawn commented Dec 20, 2024 •

edited

Loading

codecov bot commented Dec 20, 2024 •

edited

Loading

azych left a comment •

edited

Loading