Updates golangci-lint #635
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| @@ -61,18 +61,18 @@ func DependentPredicateFuncs() crtpredicate.Funcs { | |||
| // necessary. Ignore updates that only change the status and | |||
| // resourceVersion. | |||
| UpdateFunc: func(e event.UpdateEvent) bool { | |||
| old := e.ObjectOld.(*unstructured.Unstructured).DeepCopy() | |||
| new := e.ObjectNew.(*unstructured.Unstructured).DeepCopy() | |||
There was a problem hiding this comment.
New was shadowing built-in new. Renamed old into oldObj for consistency.
| @@ -21,7 +21,7 @@ const ( | |||
| ProvisionerID = "core-rukpak-io-helm" | |||
| ) | |||
|
|
|||
| func HandleBundle(ctx context.Context, fsys fs.FS, bundle *rukpakv1alpha1.Bundle) (fs.FS, error) { | |||
| func HandleBundle(_ context.Context, fsys fs.FS, _ *rukpakv1alpha1.Bundle) (fs.FS, error) { | |||
There was a problem hiding this comment.
Here and in other places with _ args - we don't use args in functions, but we still need args to statisfy interfaces.
| @@ -103,14 +103,13 @@ func SetupWithManager(mgr manager.Manager, opts ...Option) error { | |||
| } | |||
|
|
|||
| controllerName := fmt.Sprintf("controller.bundledeployment.%s", c.provisionerID) | |||
| l := mgr.GetLogger().WithName(controllerName) | |||
| } | ||
|
|
||
| func (c *controller) reconcile(ctx context.Context, bundle *rukpakv1alpha1.Bundle) (ctrl.Result, error) { |
There was a problem hiding this comment.
Here and in internal/controllers/bundledeployment/bundledeployment.go we always return an empty value in ctrl.Result{}. It makes sense in Reconcile to satisfy the interface, but not in this unexported func.
There was a problem hiding this comment.
I'm not sure I agree with this change. If we ever do want to return something other than ctrl.Result{}, it'll likely be coming from reconcile(), not Reconcile().
I'd be slightly worried that our future selves would be less inclined to change the return values back to (ctrl.Result, error) and would instead do something less desirable overall.
The primary purpose of this split is to make us not really have to think at all about status updating. The result is merely passed through.
Is a linter complaining about this? If so, WDYT about applying the //nolint:<whatever> label with an explanation along these lines.
There was a problem hiding this comment.
@joelanford I also was not super happy about this linting issue at first, but ended up obeying because I convinced myself that the code is not set in stone. It is super easy to add a new return param once we have a need for it (especially since this is not an exported method).
To me it is the same as adding input arguments into a function: we usually add new args as the need for them arises. There are exceptions (e.g. designing a public interfaces where signature changes are more painful), but it is not the case here.
But I can look into either adding nolint or disabling this linting rule altogether tomorrow. I am not a big fan of inline linting exceptions, but also do not have very strong objections against them.
There was a problem hiding this comment.
Updated: added //nolint:unparam to both
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Signed-off-by: Mikalai Radchuk <mradchuk@redhat.com>
Starting version v1.51.0
golangci-lintcomes with a newginkgolinterlinter which checks for common ginkgo issues like wrong error assertions (Expect(err).To(BeNil())instead ofExpect(err).NotTo(HaveOccurred())), etc.This PR does not add
ginkgolinter: it will be a followup PR because it will further increase the diff.In this PR:
depguardlinter removed. We do not have configuration for it and new version ofgolangci-lintcomes with the configuration where only Go standard library is allowed.reviveandunparamwere complaining)