OCPNODE-4125: Introduce to v1 CRIOCredentialProviderConfig#2725
OCPNODE-4125: Introduce to v1 CRIOCredentialProviderConfig#2725QiWang19 wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Hello @QiWang19! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThis pull request introduces a new 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/XL
|
[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 |
|
/test all |
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
QiWang19
force-pushed
the
upgrade-v1-criocp
branch
from
202816d to
4b7758f
Compare
|
/test verify |
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
|
/test all |
ⓘ Your monthly quota for Qodo has expired. Upgrade your plan ⓘ Paying users. Check that your Qodo account is linked with this Git user account |
|
/verified by cluster-bot |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
verified
|
@QiWang19: This PR has been marked as verified by DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/v1/types_crio_credential_provider_config.go (1)
125-127: Keep documented condition types in sync with declared constants.Line 125 documents only
"Validated"as an expected condition type, but Lines 171–176 also define"MachineConfigRendered". Please document both to avoid API contract drift.Also applies to: 171-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_crio_credential_provider_config.go` around lines 125 - 127, The documented list of expected condition types is out of sync with the declared constants: update the comment block that currently lists only "Validated" to include both "Validated" and "MachineConfigRendered" so it matches the constants defined later (the "Validated" and "MachineConfigRendered" condition type constants in types_crio_credential_provider_config.go); ensure both names are spelled exactly as the constants and marked +optional if appropriate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_crio_credential_provider_config.go`:
- Line 22: Remove the feature-gate annotation that gates the v1 API by deleting
the line containing "+openshift:enable:FeatureGate=CRIOCredentialProviderConfig"
in types_crio_credential_provider_config.go so the v1 CRD is unconditionally
exposed; after removing that annotation, regenerate any CRD manifests or API
docs (e.g., run the project’s codegen/make target) to ensure the CRD and
generated artifacts no longer include the FeatureGate metadata.
---
Nitpick comments:
In `@config/v1/types_crio_credential_provider_config.go`:
- Around line 125-127: The documented list of expected condition types is out of
sync with the declared constants: update the comment block that currently lists
only "Validated" to include both "Validated" and "MachineConfigRendered" so it
matches the constants defined later (the "Validated" and "MachineConfigRendered"
condition type constants in types_crio_credential_provider_config.go); ensure
both names are spelled exactly as the constants and marked +optional if
appropriate.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (3)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (9)
config/v1/register.goconfig/v1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1/types_crio_credential_provider_config.goconfig/v1/zz_generated.deepcopy.goconfig/v1/zz_generated.featuregated-crd-manifests.yamlconfig/v1/zz_generated.swagger_doc_generated.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
| // +kubebuilder:subresource:status | ||
| // +openshift:api-approved.openshift.io=https://github.com/openshift/api/pull/2725 | ||
| // +openshift:file-pattern=cvoRunLevel=0000_10,operatorName=config-operator,operatorOrdering=01 | ||
| // +openshift:enable:FeatureGate=CRIOCredentialProviderConfig |
There was a problem hiding this comment.
Remove the feature-gate annotation if this API is GA.
Line 22 still gates the v1 resource behind CRIOCredentialProviderConfig. That conflicts with the GA objective and can keep CRD exposure conditional instead of unconditional.
Proposed fix
-// +openshift:enable:FeatureGate=CRIOCredentialProviderConfig📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // +openshift:enable:FeatureGate=CRIOCredentialProviderConfig |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/v1/types_crio_credential_provider_config.go` at line 22, Remove the
feature-gate annotation that gates the v1 API by deleting the line containing
"+openshift:enable:FeatureGate=CRIOCredentialProviderConfig" in
types_crio_credential_provider_config.go so the v1 CRD is unconditionally
exposed; after removing that annotation, regenerate any CRD manifests or API
docs (e.g., run the project’s codegen/make target) to ensure the CRD and
generated artifacts no longer include the FeatureGate metadata.
|
@QiWang19: This pull request references OCPNODE-4125 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@JoelSpeed could you help review this PR? Since in the feature https://issues.redhat.com/browse/OCPSTRAT-2853 we targeted GA of this API in 4.22, we haven't done other implementation outside of o/api, so moving to v1 now won't break any external dependencies. |
@QiWang19 Before we promote a type to v1, we expect it to be feature complete and be in a state to demonstrate the feature is ready for promotion. Based on your comment, it sounds like we aren't there? Before merging a PR like this, I'd expect to see a feature promotion PR with good data showing we are on track to have the required tests and pass rate soon |
|
@JoelSpeed The feature is still in progress, not ready for promotion, which is why it's currently under a TechPreview featuregate. This PR is to update v1 API early to avoid breaking changes later, but lock the feature under TechPreviewNoUpgrade featuregate. I intended to provide the test pass rate and promotion data when we actually promote the FeatureGate to GA. I was under the impression that the test pass rate are requirements applied to the featuregate promotion rather than the initial addition of the v1 API type itself. |
|
We don't want to ship v1 types under tech preview in case we need to change the types later as we learn something during implementation. The normal path here is that you get the feature implemented in tech preview as v1alpha1, demonstrate it's working, and then we submit a handful of PRs to move the types to v1 once we are confident in the release it will ship GA in |
openshift-ci
Bot
added
the
do-not-merge/hold
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions 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. |
|
@QiWang19: This pull request references OCPNODE-4125 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
Signed-off-by: Qi Wang <qiwan@redhat.com>
QiWang19
force-pushed
the
upgrade-v1-criocp
branch
from
4b7758f to
418ebef
Compare
openshift-ci-robot
removed
the
verified
openshift-ci
Bot
removed
the
needs-rebase
|
/test verify-feature-promotion |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/v1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml (1)
46-105: Good validation coverage; consider adding boundary tests.The rejection tests comprehensively cover the
MatchImagevalidation rules. Consider adding tests for boundary conditions:
- Maximum items limit (50 entries)
- Maximum length limit (512 characters per entry)
- Minimum items enforcement when list is present but empty
These would help ensure the
kubebuilder:validationconstraints are enforced as expected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml` around lines 46 - 105, Add boundary tests for CRIOCredentialProviderConfig.spec.matchImages: create cases that verify the maximum items limit (build a test with exactly 50 valid entries that should pass and one with 51 entries that should produce an error referencing spec.matchImages), verify maximum length per entry (one entry of length 512 characters should pass and an otherwise-identical entry of length 513 should fail with an error referencing spec.matchImages[0]), and verify the empty-list case (spec.present but matchImages: [] should produce a validation error). Reference the CRIOCredentialProviderConfig kind and the spec.matchImages field in expectedError messages so they match existing test style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_crio_credential_provider_config.go`:
- Around line 33-36: The Spec field on CRIOCredentialProviderConfig (Spec
*CRIOCredentialProviderConfigSpec) is annotated as +required but uses
`json:"spec,omitempty,omitzero"` and tests accept an empty spec; either enforce
non-nil at the API level or make the metadata accurate—either add a CEL
validation rule to require spec != null (or require its required fields) on the
CRIOCredentialProviderConfig type, or change the comment and remove the
+required annotation to reflect that Spec is optional and keep the
`omitempty,omitzero` tag to match test behavior.
---
Nitpick comments:
In
`@config/v1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yaml`:
- Around line 46-105: Add boundary tests for
CRIOCredentialProviderConfig.spec.matchImages: create cases that verify the
maximum items limit (build a test with exactly 50 valid entries that should pass
and one with 51 entries that should produce an error referencing
spec.matchImages), verify maximum length per entry (one entry of length 512
characters should pass and an otherwise-identical entry of length 513 should
fail with an error referencing spec.matchImages[0]), and verify the empty-list
case (spec.present but matchImages: [] should produce a validation error).
Reference the CRIOCredentialProviderConfig kind and the spec.matchImages field
in expectedError messages so they match existing test style.
🪄 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: f018a105-2147-4284-84d2-67bb83b81260
⛔ Files ignored due to path filters (6)
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.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (4)
config/v1/register.goconfig/v1/tests/criocredentialproviderconfigs.config.openshift.io/CRIOCredentialProviderConfig.yamlconfig/v1/types_crio_credential_provider_config.gohack/update-payload-crds.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/update-payload-crds.sh
| // 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"` |
There was a problem hiding this comment.
Inconsistency between +required annotation and omitempty usage.
The comment states "This field is required and must be provided" and uses +required, but the field is a pointer with omitempty,omitzero tags. This combination allows the field to be omitted in JSON serialization. Additionally, the test file shows a minimal CR with spec: {} being valid.
If Spec is truly required, consider either:
- Adding CEL validation to enforce non-nil spec, or
- Updating the comment to reflect that spec is optional (matching the test behavior)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/v1/types_crio_credential_provider_config.go` around lines 33 - 36, The
Spec field on CRIOCredentialProviderConfig (Spec
*CRIOCredentialProviderConfigSpec) is annotated as +required but uses
`json:"spec,omitempty,omitzero"` and tests accept an empty spec; either enforce
non-nil at the API level or make the metadata accurate—either add a CEL
validation rule to require spec != null (or require its required fields) on the
CRIOCredentialProviderConfig type, or change the comment and remove the
+required annotation to reflect that Spec is optional and keep the
`omitempty,omitzero` tag to match test behavior.
|
@QiWang19: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
@QiWang19: This pull request references OCPNODE-4125 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn 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 openshift-eng/jira-lifecycle-plugin repository. |
QiWang19
changed the title
|
@JoelSpeed could you review? This introduces only the v1 type of the API for feature GA. Next, we will prepare simul-merge PRs to migrate consumers when we ship the v1 manifest. |
4 participants
Introduce v1 type CRIOCredentialProviderConfig, without shipping it in the payload(The change to update-payload-crds.sh). Next, need to migrate consumers, then swap the manifests from v1alpha1 to v1 for feature GA.