-
Notifications
You must be signed in to change notification settings - Fork 1.7k
✨ (go/v4): upgrade golangci-lint to v2.12.1 #5676
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 2 commits
aa4414e
29a7c08
4aa1086
e4fedc5
f38779e
88aa6e6
e264b9b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,9 @@ linters: | |
| logcheck: | ||
| type: "module" | ||
| description: Checks Go logging calls for Kubernetes logging conventions. | ||
| goconst: | ||
| ignore-tests: true | ||
| min-occurrences: 5 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the goal of this config is to reduce noise from test files and we are already ignoring the tests, is
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm...so the summary in the description is a bit misleading. In regards to this config, I favor explicitly adding no-lint directives to ocurrences than silently ignoring them. Also, a minimum of 5 seems a bit too much, doesn't it? It may solve problems at hand, but might also hide ocurrences in the long run.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Agreed on both points. • I’ll lower min-occurrences to 3. • For specific cases, we’ll use explicit //nolint directives instead of silent ignores WDYT? Default value
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Final offer: min of 2, take it or leave it! Just kidding. Let's wait and hear what @camilamacedo86 has to say.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think that we should ignore
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Got it. I'll remove
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am concern with the changes in the default scaffold. |
||
| revive: | ||
| rules: | ||
| - name: comment-spacings | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,6 +28,9 @@ linters: | |
| logcheck: | ||
| type: "module" | ||
| description: Checks Go logging calls for Kubernetes logging conventions. | ||
| goconst: | ||
| ignore-tests: true | ||
| min-occurrences: 5 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not think we should add it at all and change the golang ci config for the default scaffold, could we revert this change?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmmmm.... The lint rules in samples are broader in scope than those in golangci-linter default value(remove it). 🤔
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is in the sample because the default scaffold was changed. |
||
| revive: | ||
| rules: | ||
| - name: comment-spacings | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -71,6 +71,9 @@ linters: | |
| logcheck: | ||
| type: "module" | ||
| description: Checks Go logging calls for Kubernetes logging conventions. | ||
| goconst: | ||
| ignore-tests: true | ||
| min-occurrences: 5 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please revert it?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @camilamacedo86 Do you mind if I remove it in the next PR to keep the scope of code changes smaller for this PR? |
||
| revive: | ||
| rules: | ||
| - name: comment-spacings | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,6 +100,7 @@ func (r *BusyboxReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct | |
| } | ||
|
|
||
| if len(busybox.Status.Conditions) == 0 { | ||
| //nolint:goconst | ||
| meta.SetStatusCondition(&busybox.Status.Conditions, metav1.Condition{Type: typeAvailableBusybox, Status: metav1.ConditionUnknown, Reason: "Reconciling", Message: "Starting reconciliation"}) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the error here?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ref: https://github.com/kubernetes-sigs/kubebuilder/actions/runs/25423812234/job/74572117344 goconst default value:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will need to check it properly.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @camilamacedo86 |
||
| if err = r.Status().Update(ctx, busybox); err != nil { | ||
| log.Error(err, "Failed to update Busybox status") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we should add it here as well.
What are the failures ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove it done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so many errors: