CORENET-6500: fix lint job and linting failures #201

jluhrsen · 2026-01-21T07:05:03Z

No description provided.

there is no lint job for this project so nobody knew that lint issues were creeping in. In fact, golangci-lint was not even working because it was using an old/outdated config. this allows the linter to run, although there are failures to fix Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com>

openshift-ci-robot · 2026-01-21T07:05:07Z

@jluhrsen: This pull request references CORENET-6500 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set.

Details

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

coderabbitai · 2026-01-21T07:05:27Z

Walkthrough

The PR renames exported error sentinels to Go-idiomatic names (e.g., ErrAlreadyExistingIP), updates their usages across cloudprovider implementations and tests, tweaks some test error messages, adjusts golangci-lint config, and refactors the controller workqueue to use workqueue.TypedRateLimitingInterface[string] with string-typed items.

Changes

Cohort / File(s)	Summary
Cloud Provider Error Renaming `pkg/cloudprovider/cloudprovider.go`, `pkg/cloudprovider/aws.go`, `pkg/cloudprovider/azure.go`, `pkg/cloudprovider/gcp.go`, `pkg/cloudprovider/openstack.go`, `pkg/cloudprovider/cloudprovider_fake.go`	Renamed exported error variables: `AlreadyExistingIPError` → `ErrAlreadyExistingIP`, `NonExistingIPError` → `ErrNonExistingIP`, `NoNetworkInterfaceError` → `ErrNoNetworkInterface`. Updated returns, `errors.Is` checks, and some mock error message casing.
Controller Error Handling Updates `pkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller.go`, `pkg/controller/cloudprivateipconfig/cloudprivateipconfig_controller_test.go`	Updated error sentinel comparisons and test names to use new error identifiers; minor test fixes (timestamp initialization, ignored return values).
Workqueue Type Refactor `pkg/controller/controller.go`	Switched `workqueue` field type from `workqueue.RateLimitingInterface` to `workqueue.TypedRateLimitingInterface[string]`; changed queue creation to `NewTypedRateLimitingQueueWithConfig`, and removed runtime type assertions by treating queued items as `string`.
Test and Import Cleanup `pkg/cloudprovider/openstack_test.go`, `pkg/controller/node/node_controller.go`	Updated subnet type references in tests (`neutronsubnets.Subnet`), ignored `fmt.Fprint`/`w.Write` errors in handlers, removed unused import alias and updated `taintKeyExists` signature to use `corev1.Taint`.
Linter Configuration `.golangci.yaml`	Added top-level `version: "2"` and `exclude-files` pattern; removed some linters (`gofmt`, `gosimple`, `typecheck`, `exportloopref`), added `copyloopvar`, and set `formatters.settings.gofmt.simplify: false`.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

✨ Finishing touches

📝 Generate docstrings

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

openshift-ci · 2026-01-21T07:05:30Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jluhrsen
Once this PR has been reviewed and has the lgtm label, please assign kyrtapz for approval. For more information see the Code Review Process.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

the lint job was never there so linting failures have been creeping in over time. This PR[0] in CNCC fixes the job and failures. [0] openshift/cloud-network-config-controller#201 Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com>

jluhrsen · 2026-01-21T18:35:47Z

/retest-required

jluhrsen · 2026-01-21T18:37:53Z

@arkadeepsen @pliurh , could you PTAL at this? once we get this in, I have the release PR ready to get the lint job running as a pre-submit and we can use /pj-rehearse on that PR to double check it's ok.

the lint job was never there so linting failures have been creeping in over time. This PR[0] in CNCC fixes the job and failures. [0] openshift/cloud-network-config-controller#201 Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com>

arkadeepsen · 2026-01-22T12:56:10Z

.golangci.yaml

-    - gofmt
    - errcheck
-    - gofmt
-    - gosimple
    - govet
    - ineffassign
    - nosprintfhostport
    - staticcheck
-    - typecheck
    - unused
-    - exportloopref


Why are some of the linters removed?

this is moving to v2. here are the details:

gosimple it's now been merged inside of staticcheck (docs)

typecheck is no longer a linter (docs)

exportloopref is deprecated entirely (docs)

BUT, thanks for questioning this because I realized that I removed both cases of gofmt. I've updated this PR adding it back, although now it's called a formatter and not a linter. (docs)

arkadeepsen · 2026-01-22T12:59:01Z

pkg/cloudprovider/openstack.go

-			// Because we're dealing with a pointer here for matchingSubnet:
-			// we must reassign s:= s or we'd overwrite the content that we point
-			// to.
-			s := s


Better to use subnetCopy here rater than removing this assignment completely.

Suggested change

// Because we're dealing with a pointer here for matchingSubnet:

// we must reassign s:= s or we'd overwrite the content that we point

// to.

s := s

// Because we're dealing with a pointer here for matchingSubnet:

// we must reassign subnetCopy := s or we'd overwrite the content that we point

// to.

subnetCopy := s

I made this change explicitly from what I learned by running the v2 linters. these loop variables now have a per-iteration scope so there is no need to make a copy:
https://go.dev/blog/loopvar-preview#the-fix

arkadeepsen · 2026-01-22T13:00:12Z

pkg/controller/controller.go

-	workqueue := workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), resourceControllerKey)
+	workqueue := workqueue.NewTypedRateLimitingQueueWithConfig(
+		workqueue.DefaultTypedControllerRateLimiter[string](),
+		workqueue.TypedRateLimitingQueueConfig[string]{
+			Name: resourceControllerKey,
+		},
+	)


Why is this a lint issue?

it showed up as a typecheck failure:

pkg/cloudprovider/openstack.go:792:11: undefined: AlreadyExistingIPError) (typecheck) cloudprovider "github.com/openshift/cloud-network-config-controller/pkg/cloudprovider"

so it's changed to NewTypedRateLimitingQueueWithConfig needing explicit types passed to it.

arkadeepsen · 2026-01-22T13:02:46Z

pkg/controller/controller.go

-	err := func(obj interface{}) error {
+	err := func(obj string) error {
 		// We call Done here so the workqueue knows we have finished
 		// processing this item. We also must remember to call Forget if we
 		// do not want this work item being re-queued. For example, we do
 		// not call Forget if a transient error occurs, instead the item is
 		// put back on the workqueue and attempted again after a back-off
 		// period.
 		defer c.workqueue.Done(obj)
-		var key string
-		var ok bool
-		// We expect strings to come off the workqueue. These are of the
-		// form namespace/name. We do this as the delayed nature of the
-		// workqueue means the items in the informer cache may actually be
-		// more up to date that when the item was initially put onto the
-		// workqueue.
-		if key, ok = obj.(string); !ok {
-			// As the item in the workqueue is actually invalid, we call
-			// Forget here else we'd go into a loop of attempting to
-			// process a work item that is invalid.
-			c.workqueue.Forget(obj)
-			klog.Errorf("expected string in %s workqueue but got %#v", c.controllerKey, obj)
-			return nil
-		}
 		// Run the syncHandler, passing it the namespace/name string of the
 		// Foo resource to be synced.
-		if err := c.SyncHandler(key); err != nil && c.workqueue.NumRequeues(key) <= maxRetries {
+		if err := c.SyncHandler(obj); err != nil && c.workqueue.NumRequeues(obj) <= maxRetries {
 			// Put the item back on the workqueue to handle any transient errors.
-			c.workqueue.AddRateLimited(key)
-			return fmt.Errorf("error syncing '%s': %s, requeuing in %s workqueue", key, err.Error(), c.controllerKey)
+			c.workqueue.AddRateLimited(obj)
+			return fmt.Errorf("error syncing '%s': %s, requeuing in %s workqueue", obj, err.Error(), c.controllerKey)
 		}
 		// Finally, if no error occurs or if we supersede maxRetries we Forget
 		// this item so it does not get queued again until another change happens.
 		c.workqueue.Forget(obj)
-		klog.Infof("Dropping key '%s' from the %s workqueue", key, c.controllerKey)
+		klog.Infof("Dropping key '%s' from the %s workqueue", obj, c.controllerKey)


This seems to change the implementation logic a little bit. Why is this a lint issue?

this is a result of the NewTypedRateLimitingQueueWithConfig expecting strict types now. we no longer have to check if it's a string now, it has to be

lint failures have creeped in over time because there was no lint presubmit job to gate them. A job is being created here[0] which will prevent this creep in the future. this commit cleans up the existing failures summary: - Rename error variables to follow Go convention (ErrFoo instead of FooError) - Remove duplicate package imports that caused type checking failures - Update to typed workqueue API (generics-based client-go v0.32+) - Remove loop variable copies (unnecessary in Go 1.22+) - Add explicit error ignoring with _ where errors don't matter - Use keyed struct literal fields for v1.Time initialization - Simplify embedded field access patterns [0] openshift/release#73766 Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com>

jluhrsen · 2026-01-23T02:42:07Z

/test e2e-azure-ovn

jluhrsen · 2026-01-23T15:26:21Z

/test e2e-azure-ovn

openshift-ci · 2026-01-23T18:36:28Z

@jluhrsen: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/security	`9f72b56`	link	false	`/test security`

Full PR test history. Your PR dashboard.

Details

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-sigs/prow repository. I understand the commands that are listed here.

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

openshift-ci bot requested review from arkadeepsen and pliurh January 21, 2026 07:05

jluhrsen mentioned this pull request Jan 21, 2026

CORENET-6500: Add lint job for CNCC openshift/release#73766

Open

jluhrsen force-pushed the CORENET-6500 branch from 0a2d2db to dbe388f Compare January 21, 2026 07:07

arkadeepsen reviewed Jan 22, 2026

View reviewed changes

jluhrsen force-pushed the CORENET-6500 branch from dbe388f to 1f72184 Compare January 22, 2026 22:12

jluhrsen force-pushed the CORENET-6500 branch from 1f72184 to 9f72b56 Compare January 22, 2026 22:15

CORENET-6500: fix lint job and linting failures #201

Are you sure you want to change the base?

CORENET-6500: fix lint job and linting failures #201

Uh oh!

Conversation

jluhrsen commented Jan 21, 2026

Uh oh!

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

Uh oh!

Uh oh!

coderabbitai bot commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Uh oh!

openshift-ci bot commented Jan 21, 2026

Uh oh!

jluhrsen commented Jan 21, 2026

Uh oh!

jluhrsen commented Jan 21, 2026

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!

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!

jluhrsen commented Jan 23, 2026

Uh oh!

jluhrsen commented Jan 23, 2026

Uh oh!

openshift-ci bot commented Jan 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

openshift-ci-robot commented Jan 21, 2026 •

edited by openshift-ci bot

Loading

coderabbitai bot commented Jan 21, 2026 •

edited

Loading