Upgrade v1 criocp payload resource#2900
Conversation
Signed-off-by: Qi Wang <qiwan@redhat.com>
Signed-off-by: Qi Wang <qiwan@redhat.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Hello @QiWang19! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XXL
|
[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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThis PR introduces 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
config/v1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
8-106: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a negative onCreate case for singleton name enforcement.
The suite validates
spec.matchImageswell, but it doesn’t assert rejection whenmetadata.nameis notcluster. Since singleton-name enforcement is part of the CRD contract, adding this case will protect against validation regressions.✅ Suggested test case
tests: onCreate: + - name: Should reject CRIOCredentialProviderConfig with non-cluster name + initial: | + apiVersion: config.openshift.io/v1 + kind: CRIOCredentialProviderConfig + metadata: + name: not-cluster + spec: {} + expectedError: "metadata.name: Invalid value: \"not-cluster\": must be 'cluster'"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml` around lines 8 - 106, Add a new negative test case in the onCreate section to validate singleton name enforcement for the CRIOCredentialProviderConfig resource. Create a test case similar to the existing negative validation cases (e.g., "Should reject matchImages with invalid characters") that attempts to create a CRIOCredentialProviderConfig with metadata.name set to something other than "cluster" and include an expectedError that reflects the singleton name requirement. This will ensure that the validation rejects any name that is not "cluster" and prevent future regressions in singleton enforcement.config/v1/types_crio_credential_provider_config.go (1)
45-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
+kubebuilder:validation:MinProperties=0is a no-op.
MinProperties=0imposes no constraint and only adds noise to the generated schema. Drop it, or set it to1if the intent was to require at least one property inspec.♻️ Proposed change
// CRIOCredentialProviderConfigSpec defines the desired configuration of the CRI-O Credential Provider. -// +kubebuilder:validation:MinProperties=0 type CRIOCredentialProviderConfigSpec struct {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1/types_crio_credential_provider_config.go` around lines 45 - 47, Remove the `+kubebuilder:validation:MinProperties=0` annotation from the CRIOCredentialProviderConfigSpec struct definition since MinProperties=0 does not impose any constraint and only adds noise to the generated schema. If the intent was to require at least one property in the spec, replace it with `+kubebuilder:validation:MinProperties=1` instead; otherwise, simply delete the annotation line entirely.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yaml (1)
211-212: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueStorage version moved from
v1alpha1tov1— confirm migration of any persisted objects.After this change
v1is the single storage version (storage: true) andv1alpha1becomesstorage: false, which is the correct invariant. Since the storage version is switching, any objects previously persisted asv1alpha1will remain stored at that version until a storage migration runs, andstatus.storedVersionsmust converge tov1beforev1alpha1can ever be removed. For this new TechPreview singleton there is likely no GA-persisted data, but please verify no in-cluster consumer still writes/readsv1alpha1and that a storage migration is planned. Schemas for both versions are identical, so identity conversion is sufficient.Based on learnings: replacing/changing a CRD version is acceptable only if all in-cluster consumers and tooling are switched over atomically and no consumer still references the removed/old version afterward.
Also applies to: 407-407
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yaml` around lines 211 - 212, The storage version has been moved from v1alpha1 to v1, which requires ensuring the v1alpha1 version definition has storage set to false (at the location also applies to line 407). Verify that all in-cluster consumers and tooling have been atomically switched to use the v1 version instead of v1alpha1 and confirm that no code or configurations still reference v1alpha1 for reads or writes. Additionally, ensure a storage migration plan exists to migrate any persisted v1alpha1 objects to v1 (note that since the schemas for both versions are identical, identity conversion can be used for this migration), and validate that status.storedVersions will converge to v1 before v1alpha1 can be removed in the future.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/v1/types_crio_credential_provider_config.go`:
- Around line 117-132: The comment for the Conditions field in
CRIOCredentialProviderConfigStatus is incomplete regarding validation
constraints. Update the comment to document the MinItems=1 constraint that is
already specified in the kubebuilder validation marker, as per coding guidelines
requiring array length constraints to be documented in field comments.
Additionally, clarify in the comment that due to the struct-level
MinProperties=1 constraint on CRIOCredentialProviderConfigStatus and the
Conditions field being the only property in the struct, this creates a
requirement that the controller MUST include at least one condition whenever it
writes the status.
- Around line 28-42: Remove the redundant `omitempty` JSON tag from both the
Spec and Status fields in the CRIOCredentialProviderConfig struct. For the Spec
field with type CRIOCredentialProviderConfigSpec, change the tag from
`json:"spec,omitempty,omitzero"` to `json:"spec,omitzero"`. For the Status field
with type CRIOCredentialProviderConfigStatus, change the tag from
`json:"status,omitzero,omitempty"` to `json:"status,omitzero"`. This removes the
redundancy since `omitzero` already handles omitting nil pointers (for Spec) and
struct zero values (for Status), and aligns with the pattern already used for
the metadata field.
---
Nitpick comments:
In
`@config/v1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml`:
- Around line 8-106: Add a new negative test case in the onCreate section to
validate singleton name enforcement for the CRIOCredentialProviderConfig
resource. Create a test case similar to the existing negative validation cases
(e.g., "Should reject matchImages with invalid characters") that attempts to
create a CRIOCredentialProviderConfig with metadata.name set to something other
than "cluster" and include an expectedError that reflects the singleton name
requirement. This will ensure that the validation rejects any name that is not
"cluster" and prevent future regressions in singleton enforcement.
In `@config/v1/types_crio_credential_provider_config.go`:
- Around line 45-47: Remove the `+kubebuilder:validation:MinProperties=0`
annotation from the CRIOCredentialProviderConfigSpec struct definition since
MinProperties=0 does not impose any constraint and only adds noise to the
generated schema. If the intent was to require at least one property in the
spec, replace it with `+kubebuilder:validation:MinProperties=1` instead;
otherwise, simply delete the annotation line entirely.
In
`@payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yaml`:
- Around line 211-212: The storage version has been moved from v1alpha1 to v1,
which requires ensuring the v1alpha1 version definition has storage set to false
(at the location also applies to line 407). Verify that all in-cluster consumers
and tooling have been atomically switched to use the v1 version instead of
v1alpha1 and confirm that no code or configurations still reference v1alpha1 for
reads or writes. Additionally, ensure a storage migration plan exists to migrate
any persisted v1alpha1 objects to v1 (note that since the schemas for both
versions are identical, identity conversion can be used for this migration), and
validate that status.storedVersions will converge to v1 before v1alpha1 can be
removed in the future.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ea5c75e9-c6f6-4149-b40c-7fa46e2f02ba
⛔ Files ignored due to path filters (7)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.model_name.gois excluded by!**/zz_generated*config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (7)
config/v1/manual-override-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1/register.goconfig/v1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1/types_crio_credential_provider_config.gohack/update-payload-crds.shpayload-command/empty-resources/0000_05_config-operator_02_criocredentialproviderconfig.cr.yamlpayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yaml
💤 Files with no reviewable changes (1)
- hack/update-payload-crds.sh
| // metadata is the standard object's metadata. | ||
| // More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#metadata | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata,omitzero"` | ||
|
|
||
| // spec defines the desired configuration of the CRI-O Credential Provider. | ||
| // This field is required and must be provided when creating the resource. | ||
| // +required | ||
| Spec *CRIOCredentialProviderConfigSpec `json:"spec,omitempty,omitzero"` | ||
|
|
||
| // status represents the current state of the CRIOCredentialProviderConfig. | ||
| // When omitted or nil, it indicates that the status has not yet been set by the controller. | ||
| // The controller will populate this field with validation conditions and operational state. | ||
| // +optional | ||
| Status CRIOCredentialProviderConfigStatus `json:"status,omitzero,omitempty"` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
Does Kubernetes apimachinery v0.35 / Go 1.24 support the json "omitzero" struct tag for ObjectMeta and status serialization?
💡 Result:
Yes, Kubernetes apimachinery v0.35 and Go 1.24+ (and later) support the omitzero JSON struct tag option [1][2]. This support is a result of the upstream Go proposal for omitzero in encoding/json [3], which Kubernetes has integrated into its serialization stack to improve API efficiency and correctness, particularly for fields like creationTimestamp [4][2]. Key details regarding this support include: 1. Functionality: The omitzero tag option instructs the serializer to omit a struct field if the value is the zero Go value (or reports true for an IsZero() method if present) [3]. Unlike the traditional omitempty tag—which does not work as expected for nested structs or certain types—omitzero correctly handles these cases, including omitting nested structs when they are empty [1]. 2. Kubernetes Integration: Kubernetes has backported and implemented omitzero support across its core libraries, including k8s.io/apimachinery and its serialization helpers (such as ToUnstructured converters), to ensure consistent behavior across JSON, YAML, and CBOR serialization [5][2]. 3. Use Cases: One of the primary drivers for this adoption was resolving issues where fields like creationTimestamp were being serialized as explicit null values even when empty [4]. With omitzero, these fields are now correctly omitted from the serialized output, which aligns with Kubernetes API conventions for optional fields [6][2]. 4. Tooling and Linting: Modern Go linters and Kubernetes tooling (e.g., Kubebuilder scaffolds) have been updated to recognize omitzero. In many cases, it is now preferred over omitempty for nested structs, as omitzero is more semantically accurate and avoids misleading linter warnings [1][7].
Citations:
- 1: 🐛 (go/v4;deploy-image/v1-alpha): Remove omitempty from nested struct JSON tags in API scaffolds kubernetes-sigs/kubebuilder#5212
- 2: Omit null creationTimestamp kubernetes/kubernetes#130989
- 3: encoding/json: add omitzero option golang/go#45669
- 4: Avoid serializing
creationTimestamp: nullkubernetes/kubernetes#67610 - 5: Make ToUnstructured match stdlib omitempty and anonymous behavior kubernetes/kubernetes#131029
- 6: https://github.com/kubernetes/community/blob/main/contributors/devel/sig-architecture/api-conventions.md
- 7: x/tools/go/analysis/passes/modernize: omitzero: don't modify kubebuilder structs golang/go#76649
🏁 Script executed:
# First, let's examine the actual file to see the context
cat -n config/v1/types_crio_credential_provider_config.goRepository: openshift/api
Length of output: 12244
🏁 Script executed:
# Check patterns of similar field declarations in the codebase
rg -A 2 'json:.*omitzero.*omitempty|json:.*omitempty.*omitzero' --type goRepository: openshift/api
Length of output: 151
🏁 Script executed:
# Look for how other stable APIs handle metadata, spec, and status fields
rg -B 2 -A 1 'metav1\.ObjectMeta.*json:.*omitzero' --type go | head -30Repository: openshift/api
Length of output: 151
Remove redundant omitempty tags from Spec and Status fields; retain only omitzero.
omitzero is properly supported in Go 1.24+ and k8s.io/apimachinery v0.35. The combined omitempty,omitzero on line 36 (Spec) is redundant—both omit a nil pointer. For the Status struct on line 42, omitempty has no effect on struct types (per Go's encoding/json specification), making the combination odd. Simplify to:
- Line 36:
json:"spec,omitzero" - Line 42:
json:"status,omitzero"
This aligns with the pattern already used for metadata on line 31 and improves consistency.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/v1/types_crio_credential_provider_config.go` around lines 28 - 42,
Remove the redundant `omitempty` JSON tag from both the Spec and Status fields
in the CRIOCredentialProviderConfig struct. For the Spec field with type
CRIOCredentialProviderConfigSpec, change the tag from
`json:"spec,omitempty,omitzero"` to `json:"spec,omitzero"`. For the Status field
with type CRIOCredentialProviderConfigStatus, change the tag from
`json:"status,omitzero,omitempty"` to `json:"status,omitzero"`. This removes the
redundancy since `omitzero` already handles omitting nil pointers (for Spec) and
struct zero values (for Status), and aligns with the pattern already used for
the metadata field.
| // CRIOCredentialProviderConfigStatus defines the observed state of CRIOCredentialProviderConfig | ||
| // +kubebuilder:validation:MinProperties=1 | ||
| type CRIOCredentialProviderConfigStatus struct { | ||
| // conditions represent the latest available observations of the configuration state. | ||
| // When omitted, it indicates that no conditions have been reported yet. | ||
| // The maximum number of conditions is 16. | ||
| // Conditions are stored as a map keyed by condition type, ensuring uniqueness. | ||
| // | ||
| // Expected condition types include: | ||
| // "Validated": indicates whether the matchImages configuration is valid | ||
| // +optional | ||
| // +kubebuilder:validation:MaxItems=16 | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +listType=map | ||
| // +listMapKey=type | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document MinItems=1 and the MinProperties=1 interaction on status.
The conditions comment documents the max (16) but not the +kubebuilder:validation:MinItems=1 constraint. Per coding guidelines, array length constraints must be documented in the field comment. Also note that CRIOCredentialProviderConfigStatus carries MinProperties=1 while conditions is its only property and is +optional with MinItems=1; the net effect is that whenever the controller writes status, it MUST include at least one condition. Confirm this is intended and reflect it in the comment.
📝 Suggested comment update
// conditions represent the latest available observations of the configuration state.
// When omitted, it indicates that no conditions have been reported yet.
+ // When present, at least one condition must be set (minimum 1).
// The maximum number of conditions is 16.
// Conditions are stored as a map keyed by condition type, ensuring uniqueness.As per coding guidelines: "String/array length constraints must be documented in field comments when using ... +kubebuilder:validation:MinItems ... markers".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/v1/types_crio_credential_provider_config.go` around lines 117 - 132,
The comment for the Conditions field in CRIOCredentialProviderConfigStatus is
incomplete regarding validation constraints. Update the comment to document the
MinItems=1 constraint that is already specified in the kubebuilder validation
marker, as per coding guidelines requiring array length constraints to be
documented in field comments. Additionally, clarify in the comment that due to
the struct-level MinProperties=1 constraint on
CRIOCredentialProviderConfigStatus and the Conditions field being the only
property in the struct, this creates a requirement that the controller MUST
include at least one condition whenever it writes the status.
Source: Coding guidelines
1 participant
No description provided.