-
Notifications
You must be signed in to change notification settings - Fork 139
BREAKING CHANGE: fix golangci-lint's ST1003 staticcheck issues #1334
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: main
Are you sure you want to change the base?
Conversation
13ce4e5
to
22dfe37
Compare
22dfe37
to
da7de39
Compare
kind: SqlJob | ||
listKind: SqlJobList | ||
kind: SQLJob | ||
listKind: SQLJobList |
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.
Are changes in yaml files acceptable? We can avoid those changes by adding //nolint:staticcheck
in Golang struct definition like below. please let me know your opinion
// SqlJob is the Schema for the sqljobs API. It is used to run sql scripts as jobs.
type SqlJob struct { //nolint:staticcheck
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`
Spec SQLJobSpec `json:"spec,omitempty"`
Status SQLJobStatus `json:"status,omitempty"`
}
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.
Hmmm... this could be problematic. Could we scope this PR to only private types i.e. starting with lowercase? We need to think how to handle public ones, specially CRs, as it will imply not trivial migrations
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.
Ideally, there might be an automatic way in golangcilint to automatically ignore public types, as adding //nolint:staticcheck
to every single public type will be too much of a hassle.
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.
Yeah, I think so. Changing public types is going to cause huge breaking changes
Alright, I try to find the configuration to ignore all public structs and work on stuff except them
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.
While reading the golangci-lint documentation, it seems there isn't a way to ignore only public struct definitions for ST1003.
According to the golangci-lint docs, exclude-rules can filter by text or path, but not by whether a struct is public or private.
Here’s a list of ST1003 issues related to public structs.
It might not be too difficult to manually add //nolint:staticcheck comments for them:
make lint | grep "ST1003: type"
api/v1alpha1/sqljob_types.go:10:6: ST1003: type SqlJobSpec should be SQLJobSpec (staticcheck)
api/v1alpha1/sqljob_types.go:81:6: ST1003: type SqlJobStatus should be SQLJobStatus (staticcheck)
api/v1alpha1/sqljob_types.go:105:6: ST1003: type SqlJob should be SQLJob (staticcheck)
api/v1alpha1/sqljob_types.go:127:6: ST1003: type SqlJobList should be SQLJobList (staticcheck)
internal/controller/sqljob_controller.go:30:6: ST1003: type SqlJobReconciler should be SQLJobReconciler (staticcheck)
internal/webhook/v1alpha1/sqljob_webhook.go:30:6: ST1003: type SqlJobCustomValidator should be SQLJobCustomValidator (staticcheck)
pkg/command/sql.go:10:6: ST1003: type SqlOpts should be SQLOpts (staticcheck)
pkg/command/sql.go:18:6: ST1003: type SqlOpt should be SQLOpt (staticcheck)
pkg/command/sql.go:52:6: ST1003: type SqlCommand should be SQLCommand (staticcheck)
pkg/controller/sql/controller.go:20:6: ST1003: type SqlOptions should be SQLOptions (staticcheck)
pkg/controller/sql/controller.go:26:6: ST1003: type SqlOpt should be SQLOpt (staticcheck)
pkg/controller/sql/controller.go:46:6: ST1003: type SqlReconciler should be SQLReconciler (staticcheck)
pkg/controller/sql/finalizer.go:18:6: ST1003: type SqlFinalizer should be SQLFinalizer (staticcheck)
I also noticed that staticcheck has an option for customizing initialisms. Removing SQL
from the list could help in our case: staticcheck doc. But If we use it, the golangci.yaml
configuration might get a bit lengthy, and it would apply to everything including local variables and functions.
Since the amount of changes might be a bit of a large change set, please take a look whenever you have time. |
This PR continues from #1321, where four staticcheck issues were fixed.
In this PR, I’ve addressed the remaining ST1003 warnings.
Note that ST1003 involves renaming identifiers, so it may introduce breaking changes.