- Notifications
You must be signed in to change notification settings - Fork21
Switch to Go 1.24 as a min version, bump CI, modernize sources#28
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 ourterms of service andprivacy 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?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
kolyshkin commentedAug 14, 2025
lint-extra warnings should be ignored |
kolyshkin commentedAug 20, 2025
Draft until we release v0.0.5 |
kolyshkin commentedSep 5, 2025
Rebased. |
| ThrottlingDataThrottlingData`json:"throttling_data,omitempty"` | ||
| PSI*PSIStats`json:"psi,omitempty"` | ||
| BurstDataBurstData`json:"burst_data,omitempty"` | ||
| CpuUsageCpuUsage`json:"cpu_usage,omitzero"` |
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.
Ah, this is technical debt. We have two options:
- Make a breaking change by renaming it to
CPUUsage. - Add a
//nolint:revivedirective to suppress the warning.
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.
The third option is to ignore linters-extra warnings as they are technically for new code and this is not new code. In the next commit or PR linters-extra won't flag this issue as it won't be "new".
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.
@lifubang I've added the nolint directives as requested. Now the main linter errors out.
Alas our linter setup is not ideal -- the whole point of lint-extra job is to use more rules for new code only. When we change something, it is treated as "new code" while in fact it is not. This is why I said "let's ignore these warnings".
Also, this is not technical debt, as future patches won't hit this warning (lint-extra only checks "new" code).
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'm keeping the code as is for now, so you can take a look@lifubang
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 agree with keeping it as-is. The mess with lint-extra is unfortunate but such is the cost of working on "legacy" code. ;)
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.
Ah! Ended up naming exception to golangi-extra config (hoping we won't add new IDs with the whitelisted names).
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.
...and moving existing exceptions to config file, too -- yay to less code clutter!
Bump go to 1.24 in go.mod. Remove go 1.23 and add go 1.25 to CI.Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
e67ac1f to324a463CompareFor some fields (like nested struct fields), omitempty has no effectbut omitzero does. Let's use it.NOTE it might result in some compatibility issues since nowzero-only fields are not serialized.Since linter-extra thinks it's a new code, add some naming exceptions.Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
9e042c0 to0764414CompareFor some fields (like nested struct fields), omitempty has no effectbut omitzero does. Let's use it (everywhere -- for consistency).NOTE it might result in some compatibility issues since nowzero-only fields are not serialized.Since linter-extra thinks it's a new code, add some naming exceptions.Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This is technically a no-op, but let's change it for consistency.While at it, move linter-extra exceptions to golangci-extra.yml.Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Mostly done bymodernize -fix -test ./...with some minor manual edits on top.Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Uh oh!
There was an error while loading.Please reload this page.
See individual commits for details. High level overview:
omitemptywithomitzeroeverywhere (mostly this is a no-op and done for consistency, but for some fields, like nested struct fields, it will result in more "omitting" behavior)Similar runc PR:opencontainers/runc#4851