-
Notifications
You must be signed in to change notification settings - Fork 96
Fix modernize linter errors #789
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
Conversation
Signed-off-by: Erik Godding Boye <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| The bundle has "changeit" as the default password. | ||
| For more information refer to this link https://cert-manager.io/docs/faq/#keystore-passwords | ||
| Deprecated: Writing JKS is subject for removal. Please migrate to PKCS12. | ||
| Format is deprecated: Writing JKS is subject for removal. Please migrate to PKCS12. |
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.
This change appeared unexpectedly when running make generate. There is probably a bug in our CI that allowed this leftover from an older PR to pop up here.
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.
Pull Request Overview
This pull request modernizes the codebase to use Go 1.24 features by replacing interface{} with any, using strings.SplitSeq() instead of strings.Split(), adopting maps.Copy(), and updating API field tags. The changes also include enabling the modernize linter and updating documentation.
- Replaced
interface{}withanyalias throughout the codebase - Migrated from
strings.Split()tostrings.SplitSeq()for iteration - Used
maps.Copy()instead of manual map copying - Updated JSON tags for Kubernetes API types to remove
omitemptyon metadata fields
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| trust-packages/debian/main.go | Updated to use strings.SplitSeq() for iterating over split strings |
| pkg/util/pem_test.go | Updated to use strings.SplitSeq() for iterating over split strings |
| pkg/util/conversion/conversion.go | Replaced []interface{} with []any for fuzzer function |
| pkg/bundle/internal/target/target_test.go | Replaced interface{} with any for resource patches and function signatures |
| pkg/bundle/internal/target/target.go | Added maps import and replaced manual map copying with maps.Copy(), updated function signature to use any |
| pkg/bundle/bundle_test.go | Replaced []interface{} with []any for expected patches slice |
| pkg/apis/trustmanager/v1alpha2/types_cluster_bundle.go | Updated ObjectMeta JSON tag from metadata,omitempty to metadata and added alignment |
| pkg/apis/trust/v1alpha1/types_bundle.go | Updated ObjectMeta JSON tag from metadata,omitempty to metadata and added alignment |
| pkg/apis/trust/v1alpha1/conversion_test.go | Replaced []interface{} with []any for fuzzer function |
| deploy/charts/trust-manager/templates/crd-trust.cert-manager.io_bundles.yaml | Updated JKS deprecation message to clarify "Format is deprecated" |
| .golangci.yaml | Removed exclusion rules that disabled the modernize linter |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
| metav1.TypeMeta `json:",inline"` | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata"` |
Copilot
AI
Nov 8, 2025
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.
Removing omitempty from the metadata JSON tag is a breaking API change. The field will now always be serialized even when empty, which could break existing clients that depend on the field being omitted when not set. Consider whether this breaking change is intentional and properly documented in the release notes.
| metav1.ObjectMeta `json:"metadata"` | |
| metav1.ObjectMeta `json:"metadata,omitempty"` |
| metav1.ObjectMeta `json:"metadata,omitempty"` | ||
| metav1.TypeMeta `json:",inline"` | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata"` |
Copilot
AI
Nov 8, 2025
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.
Removing omitempty from the metadata JSON tag is a breaking API change. The field will now always be serialized even when empty, which could break existing clients that depend on the field being omitted when not set. Consider whether this breaking change is intentional and properly documented in the release notes.
|
/cc @inteon |
|
This is a duplicate of #786 |
Whoops, sorry. I will review your PR instead. 😊 /close |
|
@erikgb: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This enables the
modernizelinter and fixes new errors by applying the principles laid out in cert-manager/approver-policy#740.In addition to run
make fix-golangci-lintafter enabling the linter, I manually modified themetadataAPI field markers.