OCPNODE-4125: Promote CRIOCredentialProviderConfig feature gate Default#2844
OCPNODE-4125: Promote CRIOCredentialProviderConfig feature gate Default#2844QiWang19 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:
📝 WalkthroughWalkthroughThe PR enables Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/S
|
[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 verify-feature-promotion |
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 "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. |
| productScope(ocpSpecific). | ||
| enhancementPR("https://github.com/openshift/enhancements/pull/1861"). | ||
| enable(inDevPreviewNoUpgrade(), inTechPreviewNoUpgrade()). | ||
| enable(inDefault(), inDevPreviewNoUpgrade(), inTechPreviewNoUpgrade()). |
There was a problem hiding this comment.
You'll need inOKD() too here
|
/test verify-feature-promotion |
openshift-ci
Bot
added
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
features/features.go (1)
157-163:⚠️ Potential issue | 🟠 MajorCRIOCredentialProviderConfig feature-promotion: OKD payload featuregates updated, but CRD feature-set annotation still mismatched
FeatureGateCRIOCredentialProviderConfigenablesinDefault()andinOKD()(plus Dev/TechPreviewNoUpgrade) infeatures/features.go.payload-manifests/featuregates/*-OKD*.yamlincludesCRIOCredentialProviderConfig.payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlstill listsrelease.openshift.io/feature-set: CustomNoUpgrade,Default,DevPreviewNoUpgrade,TechPreviewNoUpgrade(missingOKD, still includingCustomNoUpgrade), so the CRD feature-set payloads need regeneration to match the promoted feature gate.🤖 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 `@features/features.go` around lines 157 - 163, FeatureGateCRIOCredentialProviderConfig is promoted to include inDefault() and inOKD(), but the CRD resource for CRIOCredentialProviderConfig still has a mismatched release.openshift.io/feature-set annotation; regenerate the CRD payloads for the CRIOCredentialProviderConfig CRD so the annotation matches the feature gate promotion (remove CustomNoUpgrade, add OKD) and ensure the resulting payload manifests (CRD and featuregates YAMLs for OKD/Default/DevPreviewNoUpgrade/TechPreviewNoUpgrade) are consistent with FeatureGateCRIOCredentialProviderConfig.
🤖 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.
Outside diff comments:
In `@features/features.go`:
- Around line 157-163: FeatureGateCRIOCredentialProviderConfig is promoted to
include inDefault() and inOKD(), but the CRD resource for
CRIOCredentialProviderConfig still has a mismatched
release.openshift.io/feature-set annotation; regenerate the CRD payloads for the
CRIOCredentialProviderConfig CRD so the annotation matches the feature gate
promotion (remove CustomNoUpgrade, add OKD) and ensure the resulting payload
manifests (CRD and featuregates YAMLs for
OKD/Default/DevPreviewNoUpgrade/TechPreviewNoUpgrade) are consistent with
FeatureGateCRIOCredentialProviderConfig.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 848b3736-6b18-4b6a-8969-24cf4e2631b6
⛔ Files ignored due to path filters (1)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/zz_generated.crd-manifests/*
📒 Files selected for processing (7)
features.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
💤 Files with no reviewable changes (4)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
✅ Files skipped from review due to trivial changes (1)
- payload-manifests/crds/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yaml
|
/test verify-feature-promotion |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
828-832: ⚡ Quick winMove regex compilation outside the loop.
The
ocpReleaseregex is compiled on every iteration of the loop, which is inefficient. Follow the established pattern in this file (seenonHypershiftPlatformsat Line 599) and define it as a package-level variable.♻️ Proposed refactor to define regex at package level
Add the regex near the other package-level regex definitions:
var ( requiredSelfManagedJobVariants = []JobVariant{ { Cloud: "aws", Architecture: "amd64", Topology: "ha", },nonHypershiftPlatforms = regexp.MustCompile("(?i)nutanix|metal|vsphere|openstack|azure|gcp") + ocpReleasePattern = regexp.MustCompile(`^\d+\.\d+$`) requiredHypershiftJobVariants = []JobVariant{Then update the function to use the package-level variable:
// Only consider OCP releases (e.g. "4.18", "5.0"), skip OKD ("5.0-okd"), // HCM ("aro-integration"), and other non-OCP releases. - ocpRelease := regexp.MustCompile(`^\d+\.\d+$`) for _, release := range result.Releases { - if !ocpRelease.MatchString(release) { + if !ocpReleasePattern.MatchString(release) { continue }🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 828 - 832, Move the regexp.MustCompile call out of the loop and declare a package-level variable (following the pattern used by nonHypershiftPlatforms) so it is compiled once; e.g., add a top-level var ocpRelease = regexp.MustCompile(`^\d+\.\d+$`) near the other package-level regexes, then in the loop inside the code that iterates over result.Releases simply use ocpRelease.MatchString(release) instead of recompiling each iteration.
🤖 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.
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 828-832: Move the regexp.MustCompile call out of the loop and
declare a package-level variable (following the pattern used by
nonHypershiftPlatforms) so it is compiled once; e.g., add a top-level var
ocpRelease = regexp.MustCompile(`^\d+\.\d+$`) near the other package-level
regexes, then in the loop inside the code that iterates over result.Releases
simply use ocpRelease.MatchString(release) instead of recompiling each
iteration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 20b8a977-c410-40cb-a108-9e912cd26fb3
📒 Files selected for processing (1)
tools/codegen/cmd/featuregate-test-analyzer.go
|
@QiWang19 I think the feature gate analyzer should now be fixed, you can drop your temporary patch |
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
|
/test verify-feature-promotion |
1 similar comment
|
/test verify-feature-promotion |
|
/test verify-feature-promotion |
1 similar comment
|
/test verify-feature-promotion |
|
/test verify-feature-promotion |
2 similar comments
|
/test verify-feature-promotion |
|
/test verify-feature-promotion |
|
/test all |
|
Scheduling tests matching the |
Signed-off-by: Qi Wang <qiwan@redhat.com>
|
@QiWang19: The following tests 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. |
openshift-ci
Bot
added
size/XL
|
/test all |
|
Scheduling tests matching the |
3 participants
Uh oh!
There was an error while loading. Please reload this page.