Skip to content

[Draft] - Adding cabilities for crdcompatibilitychecker and capi CRs#2884

Open
miyadav wants to merge 4 commits into
openshift:masterfrom
miyadav:ocpcloud3368
Open

[Draft] - Adding cabilities for crdcompatibilitychecker and capi CRs#2884
miyadav wants to merge 4 commits into
openshift:masterfrom
miyadav:ocpcloud3368

Conversation

@miyadav

@miyadav miyadav commented Jun 10, 2026

Copy link
Copy Markdown
Member

Adding new capability constants
WIP - OCPCLOUD-3368
/hold

Generated by - claude-opus-4-6(2.1.169)

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 10, 2026
@openshift-ci

openshift-ci Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Hello @miyadav! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Note

Currently processing new changes in this PR. This may take a few minutes, please wait...

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 3e94ca8f-0c5e-4b85-b9ce-c863fe0e7b7a

📥 Commits

Reviewing files that changed from the base of the PR and between 1fc3a3d and 2cf942d.

⛔ Files ignored due to path filters (11)
  • config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-CustomNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
  • config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-Default.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
  • config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-DevPreviewNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
  • config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-OKD.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
  • config/v1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversions-TechPreviewNoUpgrade.crd.yaml is excluded by !**/zz_generated.crd-manifests/*
  • config/v1/zz_generated.featuregated-crd-manifests/clusterversions.config.openshift.io/AAA_ungated.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
  • config/v1/zz_generated.featuregated-crd-manifests/clusterversions.config.openshift.io/CRDCompatibilityRequirementOperator.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
  • config/v1/zz_generated.featuregated-crd-manifests/clusterversions.config.openshift.io/ClusterUpdateAcceptRisks.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
  • config/v1/zz_generated.featuregated-crd-manifests/clusterversions.config.openshift.io/ClusterUpdatePreflight.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
  • config/v1/zz_generated.featuregated-crd-manifests/clusterversions.config.openshift.io/ImageStreamImportMode.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
  • config/v1/zz_generated.featuregated-crd-manifests/clusterversions.config.openshift.io/SignatureStores.yaml is excluded by !**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (2)
  • config/v1/tests/clusterversions.config.openshift.io/CRDCompatibilityRequirementOperator.yaml
  • config/v1/types_cluster_version.go
📝 Walkthrough

Walkthrough

This PR introduces two new optional cluster capabilities to the ClusterVersion API: CompatibilityRequirements and ClusterAPI. It adds exported constants documenting that ClusterAPI depends on CompatibilityRequirements, extends the CRD schema enumeration to include both capabilities under the CRDCompatibilityRequirementOperator feature gate, registers them in the public KnownClusterVersionCapabilities list and the current capability set, and adds an XValidation rule that prevents enabling ClusterAPI without also enabling CompatibilityRequirements (either explicitly in spec.additionalEnabledCapabilities or implicitly in status.capabilities.enabledCapabilities).

🚥 Pre-merge checks | ✅ 14 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Title check ❓ Inconclusive The title contains typos ('cabilities' instead of 'capabilities', 'crdcompatibilitychecker' lacks proper capitalization) and is vague about what is actually being added despite referencing specific components. Correct typos and clarify the title, for example: 'Add CompatibilityRequirements and ClusterAPI capability constants' to accurately describe the changes.
✅ Passed checks (14 passed)
Check name Status Explanation
Description check ✅ Passed The description mentions adding capability constants for CRDCompatibilityChecker and CAPI, which aligns with the changeset, though it is minimal and marks the PR as work-in-progress.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All Ginkgo test names in the PR are stable and deterministic, containing only static descriptive strings without dynamic information like pod names, timestamps, UUIDs, or other runtime values.
Test Structure And Quality ✅ Passed Test is standard Go unit test (not Ginkgo), appropriately simple with single responsibility, meaningful assertions, and follows existing config/v1 test patterns.
Microshift Test Compatibility ✅ Passed This PR does not add any Ginkgo e2e tests. It only modifies config/v1/types_cluster_version.go with type definitions and constants, which are not test code.
Single Node Openshift (Sno) Test Compatibility ✅ Passed No new Ginkgo e2e tests added in this PR. Changes are API type definitions and unit tests using standard Go testing package, not e2e tests.
Topology-Aware Scheduling Compatibility ✅ Passed PR only modifies config/v1/types_cluster_version.go, a Go type definition file for the ClusterVersion CRD schema. No deployment manifests, operator code, or scheduling constraints are introduced—on...
Ote Binary Stdout Contract ✅ Passed The PR modifies only config/v1/types_cluster_version.go, a pure CRD type definition file with no executable code. No stdout-writing operations (fmt.Print*, log., klog., os.Stdout) are present....
Ipv6 And Disconnected Network Test Compatibility ✅ Passed PR only modifies config/v1/types_cluster_version.go to add type definitions and constants, not Ginkgo e2e tests; check is not applicable.
No-Weak-Crypto ✅ Passed The PR only modifies CRD type definitions and constants in config/v1/types_cluster_version.go; no cryptographic algorithms, custom crypto implementations, or weak cipher usage detected.
Container-Privileges ✅ Passed This PR modifies only Go type definition files for Kubernetes CRDs, not container/K8s manifests. No privileged container configurations present.
No-Sensitive-Data-In-Logs ✅ Passed No logging statements found in the PR. The modified file contains only type definitions, constants, and comments with no executable code. All validation messages are safe user-friendly error messag...

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can scan changed files for high-signal PII patterns.

Microsoft Presidio Analyzer scans changed files for sensitive identifiers such as payment card numbers, US Social Security numbers, Bitcoin wallet addresses, and phone numbers. To tune entities, score thresholds, or languages, add Microsoft Presidio configuration in the repository (for example .presidiocli or AnalyzerEngineProvider YAML). When that configuration is present, CodeRabbit skips the built-in Presidio scan.

@openshift-ci openshift-ci Bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jun 10, 2026
@openshift-ci openshift-ci Bot requested review from JoelSpeed and everettraven June 10, 2026 12:49
@openshift-ci

openshift-ci Bot commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign joelspeed for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@miyadav miyadav marked this pull request as draft June 10, 2026 12:49
@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 10, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
config/v1/types_cluster_version.go (1)

22-22: ⚡ Quick win

Add/extend CRD validation tests for the new dependency rule.

Please add explicit create/update tests for ClusterAPI requiring CompatibilityRequirements (both failing and passing paths), mirroring the existing marketplace dependency test style.

As per coding guidelines, “Add validation tests in <group>/<version>/tests/<crd-name>/” for API validation changes.

🤖 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_cluster_version.go` at line 22, Add CRD validation tests for
the new XValidation rule in types_cluster_version.go that enforces ClusterAPI
requires CompatibilityRequirements: create tests under the API group/version
tests directory (following the pattern in existing marketplace dependency tests)
that cover both failing and passing paths for create and update operations; name
tests clearly (e.g.,
TestCreateClusterVersion_ClusterAPIWithoutCompatibilityRequirements_Fails and
TestUpdateClusterVersion_EnableCompatibilityRequirements_Passes) and construct
ClusterVersion objects toggling spec.capabilities.additionalEnabledCapabilities
and status.capabilities.enabledCapabilities to assert validation rejection when
ClusterAPI is present without CompatibilityRequirements and acceptance when
CompatibilityRequirements is present either in spec or in status.

Source: Coding guidelines

🤖 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_cluster_version.go`:
- Line 22: The XValidation rule currently only applies when
spec.capabilities.baselineCapabilitySet == 'None', letting configs with a
non-None baseline (e.g., baselineCapabilitySet: v4.18) enable ClusterAPI without
CompatibilityRequirements; remove the baselineCapabilitySet check so the rule
always enforces the dependency: update the kubebuilder XValidation expression in
types_cluster_version.go to check for the presence of spec.capabilities and
spec.capabilities.additionalEnabledCapabilities, and if 'ClusterAPI' is in
spec.capabilities.additionalEnabledCapabilities require
'CompatibilityRequirements' to be either in
spec.capabilities.additionalEnabledCapabilities or (if present) in
status.capabilities.enabledCapabilities; keep the existing has(...) guards and
the same error message.

---

Nitpick comments:
In `@config/v1/types_cluster_version.go`:
- Line 22: Add CRD validation tests for the new XValidation rule in
types_cluster_version.go that enforces ClusterAPI requires
CompatibilityRequirements: create tests under the API group/version tests
directory (following the pattern in existing marketplace dependency tests) that
cover both failing and passing paths for create and update operations; name
tests clearly (e.g.,
TestCreateClusterVersion_ClusterAPIWithoutCompatibilityRequirements_Fails and
TestUpdateClusterVersion_EnableCompatibilityRequirements_Passes) and construct
ClusterVersion objects toggling spec.capabilities.additionalEnabledCapabilities
and status.capabilities.enabledCapabilities to assert validation rejection when
ClusterAPI is present without CompatibilityRequirements and acceptance when
CompatibilityRequirements is present either in spec or in status.
🪄 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: afefc07d-7be7-4bb8-a2e8-90622698cd31

📥 Commits

Reviewing files that changed from the base of the PR and between 5ce2c30 and e0a748d.

📒 Files selected for processing (1)
  • config/v1/types_cluster_version.go

Comment thread config/v1/types_cluster_version.go Outdated
@openshift-ci openshift-ci Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 10, 2026
@miyadav miyadav marked this pull request as ready for review June 17, 2026 06:42
@openshift-ci openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 17, 2026
// +kubebuilder:subresource:status
// +kubebuilder:resource:path=clusterversions,scope=Cluster
// +kubebuilder:validation:XValidation:rule="has(self.spec.capabilities) && has(self.spec.capabilities.additionalEnabledCapabilities) && self.spec.capabilities.baselineCapabilitySet == 'None' && 'marketplace' in self.spec.capabilities.additionalEnabledCapabilities ? 'OperatorLifecycleManager' in self.spec.capabilities.additionalEnabledCapabilities || (has(self.status) && has(self.status.capabilities) && has(self.status.capabilities.enabledCapabilities) && 'OperatorLifecycleManager' in self.status.capabilities.enabledCapabilities) : true",message="the `marketplace` capability requires the `OperatorLifecycleManager` capability, which is neither explicitly or implicitly enabled in this cluster, please enable the `OperatorLifecycleManager` capability"
// +kubebuilder:validation:XValidation:rule="has(self.spec.capabilities) && has(self.spec.capabilities.additionalEnabledCapabilities) && 'ClusterAPI' in self.spec.capabilities.additionalEnabledCapabilities ? 'CompatibilityRequirements' in self.spec.capabilities.additionalEnabledCapabilities || (has(self.status) && has(self.status.capabilities) && has(self.status.capabilities.enabledCapabilities) && 'CompatibilityRequirements' in self.status.capabilities.enabledCapabilities) : true",message="the `ClusterAPI` capability requires the `CompatibilityRequirements` capability, which is neither explicitly or implicitly enabled in this cluster, please enable the `CompatibilityRequirements` capability"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes here look fine, but we should make this a feature gated validation based on the presence of the two gates

Comment thread config/v1/types_cluster_version.go Outdated

// ClusterVersionCapability enumerates optional, core cluster components.
// +kubebuilder:validation:Enum=openshift-samples;baremetal;marketplace;Console;Insights;Storage;CSISnapshot;NodeTuning;MachineAPI;Build;DeploymentConfig;ImageRegistry;OperatorLifecycleManager;CloudCredential;Ingress;CloudControllerManager;OperatorLifecycleManagerV1
// +kubebuilder:validation:Enum=openshift-samples;baremetal;marketplace;Console;Insights;Storage;CSISnapshot;NodeTuning;MachineAPI;Build;DeploymentConfig;ImageRegistry;OperatorLifecycleManager;CloudCredential;Ingress;CloudControllerManager;OperatorLifecycleManagerV1;CompatibilityRequirements;ClusterAPI

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make these feature gated enum validations instead

That way we only ship the capabilities when the feature gates are promoted

@stefanonardo

Copy link
Copy Markdown

according to this guide, we should add a capability set for the current version

@miyadav

miyadav commented Jun 17, 2026

Copy link
Copy Markdown
Member Author

according to this guide, we should add a capability set for the current version

Thanks @stefanonardo for review , it is already here

…patibilityRequirements

  dependency XValidation behind the CRDCompatibilityRequirementOperator feature gate so they
  only ship in TechPreview/DevPreview CRDs until the gate is promoted.
@openshift-ci openshift-ci Bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 17, 2026
… which is

  also ungated , FeatureGateAwareXValidation causing test failure
@openshift-ci

openshift-ci Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

@miyadav: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants