Skip to content

Conversation

@NathanBaulch
Copy link
Contributor

Keeping up the momentum of #4931. A new minor version of golangci-lint was released after that PR was opened. One commit per linter as before.

Copy link
Collaborator

@julio-lopez julio-lopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🥇Thanks for doing this.

And thanks for splitting it into self contained, easily reviewable commits. It simplifies and speeds up the review process.

The omitempty changes (in the last commit) probably need to be reverted for now. Even though omitempty is ineffective on structs, there is a test that checks for the presence of these tags, it really checks for an equivalence between the "policy definition source" and the structs containing the actual values.

if sleepInterval < minWaitInterval {
sleepInterval = minWaitInterval
}
sleepInterval := max(min(maxWaitTime/10, maxWaitInterval), minWaitInterval) //nolint:mnd
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be split for clarity?

Suggested change
sleepInterval := max(min(maxWaitTime/10, maxWaitInterval), minWaitInterval) //nolint:mnd
sleepInterval := min(maxWaitTime/10, maxWaitInterval) //nolint:mnd
sleepInterval = max(sleepInterval, minWaitInterval)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm accustomed to seeing min/max composed together to "clamp" a value, but happy to split if you prefer.

@codecov
Copy link

codecov bot commented Nov 12, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.02%. Comparing base (cb455c6) to head (1d96102).
⚠️ Report is 746 commits behind head on master.

Files with missing lines Patch % Lines
cli/command_snapshot_fix_remove_files.go 0.00% 2 Missing and 1 partial ⚠️
internal/indextest/indextest.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4973      +/-   ##
==========================================
+ Coverage   75.86%   78.02%   +2.15%     
==========================================
  Files         470      548      +78     
  Lines       37301    31412    -5889     
==========================================
- Hits        28299    24508    -3791     
+ Misses       7071     4853    -2218     
- Partials     1931     2051     +120     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@julio-lopez julio-lopez merged commit 657fda2 into kopia:master Nov 12, 2025
23 checks passed
@NathanBaulch NathanBaulch deleted the golangcilint261 branch November 12, 2025 05:27
julio-lopez added a commit that referenced this pull request Nov 13, 2025
The `omitempty` JSON tag is ineffective on fields of struct type, among others.

The fields with tags changed from `omitempty` to `omitzero` can be classified into 2 categories:
- fields for which the intent was to avoid serializing the value when it was zero or empty (such as structs with the zero value for the struct);
- policy-definition fields that have the `omitempty` to match the corresponding policy fields.

The fields are changed such that
- the fields are not serialized when they have the zero (or empty) value; and
- consistency is maintained between policy fields and the corresponding policy definition fields.

---

In the context of policy definitions:

**Fields of type `struct`**:
In some cases, the fields in the policy definition and values are of type struct, such as is the case for the `policy.Policy` and `policy.Definition` structs.
In these cases, the `omitzero` JSON tag avoids marshaling empty fields, making the serialized representation more compact and less noisy, while preserving the same behavior and thus semantic when unmarshaling omitted fields.

**Fields of pointer types**:
The `omitempty` and `omitzero` have _practically_ the same effect on fields of pointer types:
- the field is omitted when it is null
- the field is included when it is not null, even if the value that it points to is "the zero value" for the non-pointer type.

Note: when the pointer type defines an `IsZero()` member function, then that field would also be omitted during marshaling. There are no defined `IsZero()` function for these pointers, so
the semantics are preserved in this case.

The `omitzero` JSON tag in the fields definition structs, such as the `ActionPolicyDefinition struct`, does not change the semantics, it simply makes the marshaled representation more compact.

**Fields of type slice**:
The behavior for `omitempty` and `omitzero` differs for slices, and maps as well.

The struct fields of slice type, such as `[]string`, are left with the `omitempty` tag to be able to tell the difference between a nil slice and a non-nil, zero-length slice. Even though, currently most code paths do not explicitly differentiate between a nil slice and an empty slice, the `omitempty` tag is left unmodified out of abundance of caution.

---

Ref:

- #4973
- #4907
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants