refactor: bump golangci-lint to 2.6.2 #1815
Conversation
bernot-dev
force-pushed
the
golangci-lint-2.6.2
branch
2 times, most recently
from
24f83f9 to
ebd82b9
Compare
bwplotka
left a comment
bwplotka
left a comment
There was a problem hiding this comment.
Thanks! Majority changes make sense, just few suggestions.
The removals of omitempty in json struct tags have been verified by modernize to be ineffective. omitzero was not added to avoid behavior changes. Corresponding fields have been explicitly marked optional. This results in some updates to the documentation, but no changes to the CRDs themselves.
Hm, are we sure about the modernize check? This is true for std json, but we don't use std json decoding but that yaml.v3 which does have a unique omitempty-almost-like-omitzero behaviour - https://pkg.go.dev/gopkg.in/yaml.v3#section-readme:~:text=Only%20include%20the%20field%20if%20it%27s%20not%20set%20to%20the%20zero
Notably zero value is omitted in our yaml parsing (also what k8s is using) (!), so I think this can potentially break things. Can we ignore that modernize rule just to be safe (unless we have time to test this thoroughly)?
cmd/rule-evaluator/main.go
Outdated
| if _, err := fmt.Fprintln(w, "rule-evaluator is Ready."); err != nil { | ||
| _ = level.Error(logger).Log("msg", "Unable to write readiness check message", "err", err) |
There was a problem hiding this comment.
Can we have a global ignore for logging and fmt.Fprintln? It's 99% not needed.
E.g. here we never handle error for HTTP response bytes (best effort), even in the most high quality prod servers I worked with.
| SourceLabels: prommodel.LabelNames{objectLabelPresent + sanitizeLabelName(exp.Key)}, | ||
| Regex: relabel.MustNewRegexp("true"), | ||
| }) | ||
| default: |
There was a problem hiding this comment.
TODO for myself (or anyone else): Double check if this is for sure all cases in op.Operator.
There was a problem hiding this comment.
| // The last time this condition was updated. | ||
| // +optional | ||
| LastUpdateTime metav1.Time `json:"lastUpdateTime,omitempty"` | ||
| LastUpdateTime metav1.Time `json:"lastUpdateTime"` |
There was a problem hiding this comment.
Hm, I think those are not safe to remove. Modernize is doing this assuming we use std JSON encoder/decoder but I wonder if that's true (we use yaml.v3, same k8s).
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
| metav1.TypeMeta `json:",inline"` | ||
| // +optional |
There was a problem hiding this comment.
This is manual change for kubebuilder not modernize change right?
There was a problem hiding this comment.
Yes. I think it's safe to just make these required....you can't have a valid resource without a name. But it changes the CRD spec and explicitly marks it as required unless it's marked optional.
There was a problem hiding this comment.
Can we do nothing (what was before?) 🙃
Just to pick our battles?
| type PodMonitoring struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
| // +kubebuilder:validation:Optional |
There was a problem hiding this comment.
Hm, sometimes +optional is used too. Which one we want? Can we skip kubebuilder vs linter changes to separate PRs?
bernot-dev
force-pushed
the
golangci-lint-2.6.2
branch
from
ebd82b9 to
5f2f54f
Compare
2 participants
Bump golangci-lint to 2.6.2.
The removals of
omitemptyin json struct tags have been verified by modernize to be ineffective.omitzerowas not added to avoid behavior changes. Corresponding fields have been explicitly marked optional. This results in some updates to the documentation, but no changes to the CRDs themselves.Instances of
interface{}have been changed toany.String formatting functions have been replaced when they do not use actual formatting placeholders.
TestMainimplicitly callsos.Exitsince 1.15.The pattern of splitting and then iterating over the parts has been replaced with
SplitSeq, which is more efficient.