bumping with o/api changes to reflect new capabilities (crdcompatibilitychecker+capi)#1403
Open
miyadav wants to merge 4 commits into
Open
bumping with o/api changes to reflect new capabilities (crdcompatibilitychecker+capi)#1403miyadav wants to merge 4 commits into
miyadav wants to merge 4 commits into
Conversation
openshift-ci
Bot
added
the
do-not-merge/hold
|
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:
WalkthroughThe PR adds feature-gate filtering to cluster version capabilities. A new ChangesFeature-gate filtering for cluster version capabilities
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: miyadav 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 |
![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
🤖 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 `@go.mod`:
- Line 118: go.mod currently contains a local replace directive pointing
github.com/openshift/api to a personal fork (the replace line referencing
github.com/miyadav/api), which must be removed or updated before merging;
replace that directive with the official module (remove the replace or point to
github.com/openshift/api at the appropriate tagged release) only after
OpenShift/api PR `#2884` is merged and an upstream release containing the needed
changes exists, and verify the release includes the expected capability/license
provenance before switching the dependency back to upstream.
🪄 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: e5f16882-9423-42ae-bc77-b4b83e14eb44
⛔ Files ignored due to path filters (56)
go.sumis excluded by!**/*.sum,!go.sumvendor/github.com/openshift/api/.ci-operator.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/Dockerfile.ocpis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_apiserver.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_authentication.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_operator.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_image.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_infrastructure.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_kmsencryption.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_network.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/types_tlssecurityprofile.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_apiservers-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_authentications-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_images.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-CustomNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-DevPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_networks-TechPreviewNoUpgrade.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/console/v1/types_console_plugin.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/console/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/install.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/types_machineset.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/machine/v1beta1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/types_etcd.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/generated.protois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/security/v1/types.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/security/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/github.com/openshift/api/security/v1/zz_generated.swagger_doc_generated.gois excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (1)
go.mod
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@go.mod`:
- Line 118: Remove the replace directive in go.mod that currently points to the
personal fork github.com/miyadav/api. Before proceeding, verify that
openshift/api PR `#2884` has been merged and published as an official release tag.
Update the require directive to pin to the official github.com/openshift/api
module at the appropriate release version tag instead of using a pseudo-version.
Delete the entire replace line that references the miyadav/api fork. This
ensures the project depends on the official upstream module rather than a
personal fork, eliminating the supply-chain security concern.
🪄 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: 64f2bc7f-b24f-4144-8c85-d7cd07a9655a
⛔ Files ignored due to path filters (6)
go.sumis excluded by!**/*.sum,!go.sumvendor/github.com/openshift/api/config/v1/types_cluster_version.gois excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-Default.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yamlis excluded by!vendor/**,!**/vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!vendor/**,!**/vendor/**,!**/zz_generated*vendor/modules.txtis excluded by!vendor/**,!**/vendor/**
📒 Files selected for processing (1)
go.mod
gates CompatibilityRequirements and ClusterAPI capabilities behind CRDCompatibilityRequirementOperator using FeatureGateAwareEnum
Member
Author
|
/test images |
Member
Author
|
/retest-required |
miyadav
changed the title
Member
Author
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
hongkailiu
reviewed
Jun 26, 2026
hongkailiu
left a comment
hongkailiu
left a comment
Member
There was a problem hiding this comment.
I do not know much about cluster-api and some of my comments are only to clear up my own confusion.
Comment on lines
+16
to
+24
| var featureGatedCapabilities = map[configv1.ClusterVersionCapability][]string{ | ||
| configv1.ClusterVersionCapabilityCompatibilityRequirements: { | ||
| "CRDCompatibilityRequirementOperator", | ||
| "ClusterAPIMachineManagement", | ||
| }, | ||
| configv1.ClusterVersionCapabilityClusterAPI: { | ||
| "ClusterAPIMachineManagement", | ||
| }, | ||
| } |
Member
There was a problem hiding this comment.
Could we reuse configv1.FeatureGateName instead of string? like:
Suggested change
| var featureGatedCapabilities = map[configv1.ClusterVersionCapability][]string{ | |
| configv1.ClusterVersionCapabilityCompatibilityRequirements: { | |
| "CRDCompatibilityRequirementOperator", | |
| "ClusterAPIMachineManagement", | |
| }, | |
| configv1.ClusterVersionCapabilityClusterAPI: { | |
| "ClusterAPIMachineManagement", | |
| }, | |
| } | |
| var featureGatedCapabilities = map[configv1.ClusterVersionCapability][]configv1.FeatureGateName{ | |
| configv1.ClusterVersionCapabilityCompatibilityRequirements: { | |
| features.FeatureGateCRDCompatibilityRequirementOperator, | |
| features.FeatureGateClusterAPIMachineManagement, | |
| }, | |
| configv1.ClusterVersionCapabilityClusterAPI: { | |
| features.FeatureGateClusterAPIMachineManagement, | |
| }, | |
| } |
|
|
||
| // FilterByFeatureGates returns the subset of capabilities whose required | ||
| // feature gates are enabled. | ||
| func FilterByFeatureGates(capabilities []configv1.ClusterVersionCapability, |
Member
There was a problem hiding this comment.
Is it used only by unit tests?
Member
Author
There was a problem hiding this comment.
yes (cvo scenario and capability test outside hence exported)
Member
There was a problem hiding this comment.
I am sure if I follow.
Is FilterByFeatureGates going to imported by other repos?
- If yes, I think
openshift/library-gois a better place for the function. - If no, we do not see why we define a function in the core code but not used in the core code.
Member
Author
There was a problem hiding this comment.
it might be useful to oc , but at this moment not sure for lib-go , it should have more use cases I think .
Member
There was a problem hiding this comment.
I would strongly suggest to move it to the testing code now, and add the code back to core ONLY when we really need it, or have a clear use case.
From my experience, otherwise, we will forget why it exists very soon. ^_^
Member
There was a problem hiding this comment.
After reading the usage of FilterByFeatureGates again, I think we can just remove the function completely. If it is useful to oc, we can implement and test it in oc.
For cvo, we can just use the result of FilterByFeatureGates directly.
Comment on lines
+31
to
+38
| enabled := false | ||
| for _, gate := range gates { | ||
| if enabledFeatureGates.Has(gate) { | ||
| enabled = true | ||
| break | ||
| } | ||
| } | ||
| if !enabled { |
Member
There was a problem hiding this comment.
nit: feels like we could use the syntactical sugur from sets:
Suggested change
| enabled := false | |
| for _, gate := range gates { | |
| if enabledFeatureGates.Has(gate) { | |
| enabled = true | |
| break | |
| } | |
| } | |
| if !enabled { | |
| if enabledFeatureGates.Intersection(sets.New[configv1.FeatureGateName](gates...)).Len() > 0 { |
|
|
||
| // gatedCapabilities returns the set of capabilities that should be excluded | ||
| // because none of their enabling feature gates are in enabledFeatureGates. | ||
| func gatedCapabilities(enabledFeatureGates sets.Set[string]) sets.Set[configv1.ClusterVersionCapability] { |
Member
There was a problem hiding this comment.
nit: excludedCapabilities sounds simpler to me.
Suggested change
| func gatedCapabilities(enabledFeatureGates sets.Set[string]) sets.Set[configv1.ClusterVersionCapability] { | |
| func excludedCapabilities(enabledFeatureGates sets.Set[string]) sets.Set[configv1.ClusterVersionCapability] { |
| // It ensures that each capability in the collection is still enabled in the returned cluster capabilities | ||
| // and recognizes all implicitly enabled ones. | ||
| // and recognizes all implicitly enabled ones. Capabilities gated behind feature gates that are not in | ||
| // enabledFeatureGates are excluded from both Known and Enabled sets. |
Member
There was a problem hiding this comment.
Do I understand it correctly?
If a cap is in excluded, it is retreated as not Known, not Enabled, and not implicitlyEnabled?
If that is the case, we could do on the clusterCapabilities object before returning and keep the rest of the function intact.
Something like:
clusterCapabilities.Known=clusterCapabilities.Known.Difference(excluded)
clusterCapabilities.Enabled=clusterCapabilities.Enabled.Difference(excluded)
clusterCapabilities.ImplicitlyEnabled=clusterCapabilities.ImplicitlyEnabled.Difference(excluded)Another question: what is the difference between a unknown cap and an excluded cap?
To me, all the caps defined the current version of the cluster are known, both to the cluster and the admins.
The excluded ones are kind of implicitly disabled but they are known.
Member
Author
There was a problem hiding this comment.
unknown is API server rejects the request and excluded is when code exists in the binary and its constants exist in the API types, but it's hidden until its feature gate is enabled .
Member
There was a problem hiding this comment.
Would a user be confused, if they see a cap is listed in the OpenShift docs, but it is not visible (because it is hidden) from the known API?
Are we going to talk about this in the docs?
Member
Author
There was a problem hiding this comment.
There is a doc card , We can add this to it. Will check further , thanks .
…to []configv1.FeatureGateName, using features.FeatureGateCRDCompatibilityRequirementOperator and features.FeatureGateClusterAPIMachineManagement constants. Conversion to string happens inside excludedCapabilities() when checking against enabledFeatureGates , gatedCapabilities renamed to excludedCapabilities , categorizeEnabledCapabilities reverted to its original signature (no excluded parameter). Filtering now happens as a post-step in SetCapabilities via .Difference(excluded) on Known, Enabled, and ImplicitlyEnabled and Manual for/Has/break loop replaced with sets.Intersection.
Member
Author
|
/test e2e-aws-ovn-techpreview |
hongkailiu
reviewed
Jun 29, 2026
Comment on lines
+98
to
+104
| clusterCapabilities.Enabled = clusterCapabilities.Enabled.Difference(excluded) | ||
| if clusterCapabilities.ImplicitlyEnabled != nil { | ||
| clusterCapabilities.ImplicitlyEnabled = clusterCapabilities.ImplicitlyEnabled.Difference(excluded) | ||
| if clusterCapabilities.ImplicitlyEnabled.Len() == 0 { | ||
| clusterCapabilities.ImplicitlyEnabled = nil | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Sorry. I forgot to handle the nil in the last round:
Suggested change
| clusterCapabilities.Enabled = clusterCapabilities.Enabled.Difference(excluded) | |
| if clusterCapabilities.ImplicitlyEnabled != nil { | |
| clusterCapabilities.ImplicitlyEnabled = clusterCapabilities.ImplicitlyEnabled.Difference(excluded) | |
| if clusterCapabilities.ImplicitlyEnabled.Len() == 0 { | |
| clusterCapabilities.ImplicitlyEnabled = nil | |
| } | |
| } | |
| clusterCapabilities.Enabled = exclude(clusterCapabilities.Enabled, excluded) | |
| clusterCapabilities.ImplicitlyEnabled = exclude(clusterCapabilities.ImplicitlyEnabled, excluded) |
with a syntax sugar function:
func exclude(current, excluded sets.Set[configv1.ClusterVersionCapability]) sets.Set[configv1.ClusterVersionCapability] {
if current == nil {
return nil
}
ret := current.Difference(excluded)
if ret.Len() == 0 {
return ret
}
return ret
}It will handle the nil on clusterCapabilities.Enabled as well.
Comment on lines
+160
to
+162
| for _, c := range capabilitiesSpec.AdditionalEnabledCapabilities { | ||
| enabled.Insert(c) | ||
| } |
Member
There was a problem hiding this comment.
Do we still need this change?
| // The scenario tests run without feature gates enabled, so filter out | ||
| // capabilities gated behind disabled feature gates to match runtime behavior. | ||
| noGates := sets.New[string]() | ||
| sortedCaps = capability.FilterByFeatureGates( |
Member
There was a problem hiding this comment.
Is FilterByFeatureGates sorting the result?
| } | ||
| } | ||
|
|
||
| func TestFilterByFeatureGates(t *testing.T) { |
Member
There was a problem hiding this comment.
We could remove the test if the target function is not in the core code.
|
|
||
| // FilterByFeatureGates returns the subset of capabilities whose required | ||
| // feature gates are enabled. | ||
| func FilterByFeatureGates(capabilities []configv1.ClusterVersionCapability, |
Member
There was a problem hiding this comment.
After reading the usage of FilterByFeatureGates again, I think we can just remove the function completely. If it is useful to oc, we can implement and test it in oc.
For cvo, we can just use the result of FilterByFeatureGates directly.
…pability.SortedList(...) to get filteredlist
Extract nil-safe differenceOrNil helper for Difference(excluded) calls, revert categorizeEnabledCapabilities to use one-liner Insert for AdditionalEnabledCapabilities, and remove redundant sort.Slice calls since SortedList already returns sorted output.
Member
Author
|
/retest-required |
Contributor
|
@miyadav: 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. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
bumping changes from my fork to add new capabilities .
WIP - OCPCLOUD-3368
/hold
Generated by - claude-opus-4-6(2.1.169)
Summary by CodeRabbit