OCPNODE-4125: Introduce to v1 CRIOCredentialProviderConfig#2725
OCPNODE-4125: Introduce to v1 CRIOCredentialProviderConfig#2725QiWang19 wants to merge 1 commit into
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: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (6)
💤 Files with no reviewable changes (2)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis pull request introduces a new Suggested reviewers
🚥 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 |
openshift-ci
Bot
added
the
size/XL
|
/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
QiWang19
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@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
size/XXL
QiWang19
force-pushed
the
upgrade-v1-criocp
branch
from
a0f1b87 to
09e8c83
Compare
|
@JoelSpeed PTAL |
|
@QiWang19 Thanks for the updates, changes here look good. Could you please line up PRs for all component repositories that need to be updated to account for this change, and then we will test them together with payload commands. I'm expecting that to be client-go, origin and MCO, anything else you think? |
|
/pipeline required |
|
Scheduling tests matching the |
Signed-off-by: Qi Wang <qiwan@redhat.com>
QiWang19
force-pushed
the
upgrade-v1-criocp
branch
from
09e8c83 to
c85beac
Compare
|
/pipeline required |
|
Scheduling tests matching the |
I think we also add a PR that upgrade the singlton payload resoucre Draft PRs that are ready to be tested after this API PR merged:
After merged above PRs, we can continue on Remove the v1alpha1 manual override, and upgrade the FeatureGate to Default @JoelSpeed would the above work? |
|
@QiWang19 Yep, that works. What's a good test suite to test these PRs together? Can we get a smoke test using |
|
we can run under MCO PR: Both job can pass, for
launch the aws techpreview cluster with the PRs succeeded.(view logs) |
|
Should we get this PR in, and then merge a vendor bump openshift/client-go#385 first? |
|
Ack, those smoke tests look good to me |
openshift-ci-robot
added
the
verified
|
@JoelSpeed: 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. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/retest |
|
/test e2e-aws-ovn-techpreview |
|
should we override test |
|
@QiWang19: The following test failed, say
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. |
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.