Remove VideoConfig feature gate#4294
Conversation
kubevirt-bot
added
do-not-merge/release-note-label-needed
|
[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 |
📝 WalkthroughWalkthroughThe PR removes Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.0)controllers/handlers/kubevirt_test.goWarning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 13 linked repositories, but your current plan allows 0. Analyzed ``, skipped Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/v1/featuregates/feature_gates_test.go (1)
193-193: ⚡ Quick winClarify the test description to match the assertion.
The test description on line 193 says "not in list; disabled" but expects
BeTrue(). Since beta feature gates default to enabled when not explicitly listed, the description should say "not in list; enabled" to accurately reflect the expected behavior being tested.📝 Proposed fix for test description clarity
- Entry("known beta FG; not in list; disabled", featuregates.HyperConvergedFeatureGates{{Name: "deployKubeSecondaryDNS", State: ptr.To(featuregates.Enabled)}}, "decentralizedLiveMigration", BeTrue()), + Entry("known beta FG; not in list; enabled", featuregates.HyperConvergedFeatureGates{{Name: "deployKubeSecondaryDNS", State: ptr.To(featuregates.Enabled)}}, "decentralizedLiveMigration", BeTrue()),🤖 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 `@api/v1/featuregates/feature_gates_test.go` at line 193, Update the test case description string in the failing Entry to match the expected assertion: change the description from "known beta FG; not in list; disabled" to "known beta FG; not in list; enabled" in the Entry that uses featuregates.HyperConvergedFeatureGates{{Name: "deployKubeSecondaryDNS", State: ptr.To(featuregates.Enabled)}} and checks BeTrue() so the human-readable test name accurately reflects that beta FGs default to enabled when not listed.
🤖 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 `@controllers/handlers/kubevirt_test.go`:
- Around line 41-42: The test constant conditionalFeatureGatesCount is out of
sync with the feature-gate defaults after VideoConfig removal—update its value
to reflect that getFeatureGateChecks (in controllers/handlers/kubevirt.go) will
always add two conditional gates (kvDecentralizedLiveMigration and
kvHotplugVolumesGate) when .spec.featureGates is empty; change
conditionalFeatureGatesCount to 2 and also simplify the comment to read “volume
hotplug and decentralizedLiveMigration” for clarity so the test and its comment
match the behavior of getFeatureGateChecks.
---
Nitpick comments:
In `@api/v1/featuregates/feature_gates_test.go`:
- Line 193: Update the test case description string in the failing Entry to
match the expected assertion: change the description from "known beta FG; not in
list; disabled" to "known beta FG; not in list; enabled" in the Entry that uses
featuregates.HyperConvergedFeatureGates{{Name: "deployKubeSecondaryDNS", State:
ptr.To(featuregates.Enabled)}} and checks BeTrue() so the human-readable test
name accurately reflects that beta FGs default to enabled when not listed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e7854e8-dded-49e0-a87e-bb7772faa528
⛔ Files ignored due to path filters (4)
api/v1/zz_generated.openapi.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.defaults.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.featuregates_conversion.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.openapi.gois excluded by!**/zz_generated.*
📒 Files selected for processing (18)
api/v1/featuregates/feature_gates_test.goapi/v1/hyperconverged_types.goapi/v1beta1/conversion_test.goapi/v1beta1/hyperconverged_types.goconfig/crd/bases/hco.kubevirt.io_hyperconvergeds.yamlcontrollers/handlers/kubevirt.gocontrollers/handlers/kubevirt_test.gocontrollers/hyperconverged/hyperconverged_controller_test.godeploy/crds/hco00.crd.yamldeploy/hco.cr.yamldeploy/index-image/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yamldeploy/olm-catalog/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yamlpkg/featuregatedetails/feature-gates.jsonpkg/internal/kvfeaturegates/kv-beta-feature-gates.jsontests/func-tests/conversion_test.gotests/func-tests/defaults_test.gotools/csv-merger/generated-crd.yamltools/manifest-templator/generated-crd.yaml
💤 Files with no reviewable changes (7)
- controllers/hyperconverged/hyperconverged_controller_test.go
- api/v1/hyperconverged_types.go
- pkg/internal/kvfeaturegates/kv-beta-feature-gates.json
- pkg/featuregatedetails/feature-gates.json
- tests/func-tests/defaults_test.go
- deploy/hco.cr.yaml
- controllers/handlers/kubevirt.go
kubevirt-bot
added
release-note
|
/cc @nunnatsa PTAL 🙏 |
Coverage Report for CI Build 28512372997Coverage decreased (-0.004%) to 81.424%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
hco-e2e-consecutive-operator-sdk-upgrades-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws, ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-upgrade-operator-sdk-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-azure 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 kubernetes-sigs/prow repository. |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
1 similar comment
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure, ci/prow/hco-e2e-operator-sdk-aws 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 kubernetes-sigs/prow repository. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure, ci/prow/hco-e2e-operator-sdk-aws 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 kubernetes-sigs/prow repository. |
dasionov
force-pushed
the
ga-videoconfig-fg
branch
from
b05bc01 to
4802ceb
Compare
|
hco-e2e-upgrade-operator-sdk-aws lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-azure, ci/prow/hco-e2e-upgrade-operator-sdk-sno-azure, ci/prow/hco-e2e-upgrade-prev-operator-sdk-aws 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 kubernetes-sigs/prow repository. |
|
hco-e2e-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-kv-smoke-azure 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 kubernetes-sigs/prow repository. |
|
/cc @nunnatsa |
|
We're still using KubeVirt v1.9.0-beta.0, that does not contain kubevirt/kubevirt#16599. /hold |
kubevirt-bot
added
do-not-merge/hold
dasionov
force-pushed
the
ga-videoconfig-fg
branch
from
4802ceb to
e54148a
Compare
kubevirt-bot
removed
the
needs-rebase
|
[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 |
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 `@tests/func-tests/defaults_test.go`:
- Around line 67-99: The feature-gate defaults test no longer covers the removed
videoConfig contract, so add an explicit regression assertion in
defaults_test.go alongside defaultFeatureGates and the Existing GetHCO/PatchHCO
flow. Use hc.Spec.FeatureGates to verify videoConfig is ignored/absent after
defaulting, or that explicitly setting it has no effect, so the functional test
still guards the VideoConfig removal 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f1c7bca-7f34-4726-8abd-bb57961dce0e
⛔ Files ignored due to path filters (4)
api/v1/zz_generated.openapi.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.defaults.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.featuregates_conversion.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.openapi.gois excluded by!**/zz_generated.*
📒 Files selected for processing (17)
api/v1/featuregates/feature_gates_test.goapi/v1/hyperconverged_types.goapi/v1beta1/conversion_test.goapi/v1beta1/hyperconverged_types.goconfig/crd/bases/hco.kubevirt.io_hyperconvergeds.yamlcontrollers/handlers/kubevirt.gocontrollers/handlers/kubevirt_test.gocontrollers/hyperconverged/hyperconverged_controller_test.godeploy/crds/hco00.crd.yamldeploy/index-image/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yamldeploy/olm-catalog/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yamldocs/api-v1.mddocs/api.mdpkg/featuregatedetails/feature-gates.jsonpkg/internal/kvfeaturegates/kv-beta-feature-gates.jsontests/func-tests/defaults_test.gotools/csv-merger/generated-crd.yaml
💤 Files with no reviewable changes (4)
- api/v1/hyperconverged_types.go
- pkg/internal/kvfeaturegates/kv-beta-feature-gates.json
- api/v1/featuregates/feature_gates_test.go
- pkg/featuregatedetails/feature-gates.json
✅ Files skipped from review due to trivial changes (3)
- docs/api.md
- docs/api-v1.md
- tools/csv-merger/generated-crd.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- controllers/handlers/kubevirt.go
- api/v1beta1/hyperconverged_types.go
- deploy/crds/hco00.crd.yaml
- controllers/handlers/kubevirt_test.go
- controllers/hyperconverged/hyperconverged_controller_test.go
- config/crd/bases/hco.kubevirt.io_hyperconvergeds.yaml
- deploy/index-image/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yaml
- api/v1beta1/conversion_test.go
- deploy/olm-catalog/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yaml
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@tests/func-tests/defaults_test.go`:
- Around line 67-99: The feature-gate defaults test no longer covers the removed
videoConfig contract, so add an explicit regression assertion in
defaults_test.go alongside defaultFeatureGates and the Existing GetHCO/PatchHCO
flow. Use hc.Spec.FeatureGates to verify videoConfig is ignored/absent after
defaulting, or that explicitly setting it has no effect, so the functional test
still guards the VideoConfig removal 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5f1c7bca-7f34-4726-8abd-bb57961dce0e
⛔ Files ignored due to path filters (4)
api/v1/zz_generated.openapi.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.defaults.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.featuregates_conversion.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.openapi.gois excluded by!**/zz_generated.*
📒 Files selected for processing (17)
api/v1/featuregates/feature_gates_test.goapi/v1/hyperconverged_types.goapi/v1beta1/conversion_test.goapi/v1beta1/hyperconverged_types.goconfig/crd/bases/hco.kubevirt.io_hyperconvergeds.yamlcontrollers/handlers/kubevirt.gocontrollers/handlers/kubevirt_test.gocontrollers/hyperconverged/hyperconverged_controller_test.godeploy/crds/hco00.crd.yamldeploy/index-image/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yamldeploy/olm-catalog/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yamldocs/api-v1.mddocs/api.mdpkg/featuregatedetails/feature-gates.jsonpkg/internal/kvfeaturegates/kv-beta-feature-gates.jsontests/func-tests/defaults_test.gotools/csv-merger/generated-crd.yaml
💤 Files with no reviewable changes (4)
- api/v1/hyperconverged_types.go
- pkg/internal/kvfeaturegates/kv-beta-feature-gates.json
- api/v1/featuregates/feature_gates_test.go
- pkg/featuregatedetails/feature-gates.json
✅ Files skipped from review due to trivial changes (3)
- docs/api.md
- docs/api-v1.md
- tools/csv-merger/generated-crd.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- controllers/handlers/kubevirt.go
- api/v1beta1/hyperconverged_types.go
- deploy/crds/hco00.crd.yaml
- controllers/handlers/kubevirt_test.go
- controllers/hyperconverged/hyperconverged_controller_test.go
- config/crd/bases/hco.kubevirt.io_hyperconvergeds.yaml
- deploy/index-image/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yaml
- api/v1beta1/conversion_test.go
- deploy/olm-catalog/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yaml
🛑 Comments failed to post (1)
tests/func-tests/defaults_test.go (1)
67-99: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep an explicit regression check for
videoConfigbeing ignored.Dropping
videoConfigfromdefaultFeatureGatesremoves the only coverage for this PR’s main contract. This test now proves the remaining defaults, but it no longer proves thatvideoConfigis absent/ignored, so a regression that still surfaces or honors that gate would pass here unnoticed. Please add an assertion for the removed gate itself (for example, that the key is absent fromhc.Spec.FeatureGatesafter defaulting, or that setting it has no effect). As per coding guidelines, "New features must have functional tests undertests/func-tests." As per path instructions, "verify test scenarios and coverage are still sufficient after removing the VideoConfig gate (including boundary/edge cases)."🤖 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 `@tests/func-tests/defaults_test.go` around lines 67 - 99, The feature-gate defaults test no longer covers the removed videoConfig contract, so add an explicit regression assertion in defaults_test.go alongside defaultFeatureGates and the Existing GetHCO/PatchHCO flow. Use hc.Spec.FeatureGates to verify videoConfig is ignored/absent after defaulting, or that explicitly setting it has no effect, so the functional test still guards the VideoConfig removal behavior.Sources: Coding guidelines, Path instructions
kubevirt-bot
added
the
needs-rebase
The VideoConfig feature gate was enabled by default in HCO [1] as the feature was beta in KubeVirt 1.8 and GA in KubeVirt 1.9 [2]. Now that the feature has reached GA and the feature gate protection has been removed upstream in KubeVirt, there is no point to set it anymore. This PR modifies HCO to stop adding the VideoConfig feature gate to the KubeVirt CR and marks the v1beta1 API field as discontinued. [1] kubevirt#4180 [2] kubevirt/kubevirt#16599 Assisted-By: Claude opus 4.6 <noreply@anthropic.com> Signed-off-by: Daniel Sionov <dsionov@redhat.com>
dasionov
force-pushed
the
ga-videoconfig-fg
branch
from
e54148a to
831fdfb
Compare
kubevirt-bot
removed
the
needs-rebase
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tools/csv-merger/generated-crd.yaml (1)
5383-5392: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
videoConfigenabled until the KubeVirt version bump lands.tools/csv-merger/generated-crd.yaml:5383-5392removesvideoConfigfrom the default feature-gate map, but the operator is still pinned tov1.9.0-beta.0inhack/config:3anddeploy/operator.yaml:62,508. That pre-GA KubeVirt build still needs the gate, so this would silently disable video config for existing users on upgrade.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/csv-merger/generated-crd.yaml` around lines 5383 - 5392, The default feature-gate map in the CRD generator is dropping videoConfig too early, which would disable it for upgrades while the operator still targets the older KubeVirt build. Update the featureGates block in generated-crd.yaml to keep videoConfig enabled alongside the existing gates until the version bump is completed. Use the featureGates map in the generated CRD output as the place to restore the missing videoConfig entry.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tools/csv-merger/generated-crd.yaml`:
- Around line 5383-5392: The default feature-gate map in the CRD generator is
dropping videoConfig too early, which would disable it for upgrades while the
operator still targets the older KubeVirt build. Update the featureGates block
in generated-crd.yaml to keep videoConfig enabled alongside the existing gates
until the version bump is completed. Use the featureGates map in the generated
CRD output as the place to restore the missing videoConfig entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a64e389-491d-481a-99dd-a19f3195d17d
⛔ Files ignored due to path filters (4)
api/v1/zz_generated.openapi.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.defaults.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.featuregates_conversion.gois excluded by!**/zz_generated.*api/v1beta1/zz_generated.openapi.gois excluded by!**/zz_generated.*
📒 Files selected for processing (18)
api/v1/featuregates/feature_gates_test.goapi/v1/hyperconverged_types.goapi/v1beta1/conversion_test.goapi/v1beta1/hyperconverged_types.goconfig/crd/bases/hco.kubevirt.io_hyperconvergeds.yamlcontrollers/handlers/kubevirt.gocontrollers/handlers/kubevirt_test.gocontrollers/hyperconverged/hyperconverged_controller_test.godeploy/crds/hco00.crd.yamldeploy/index-image/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yamldeploy/olm-catalog/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yamldocs/api-v1.mddocs/api.mdpkg/featuregatedetails/feature-gates.jsonpkg/internal/kvfeaturegates/kv-beta-feature-gates.jsontests/func-tests/defaults_test.gotools/csv-merger/generated-crd.yamltools/manifest-templator/generated-crd.yaml
💤 Files with no reviewable changes (4)
- pkg/internal/kvfeaturegates/kv-beta-feature-gates.json
- tests/func-tests/defaults_test.go
- api/v1/featuregates/feature_gates_test.go
- controllers/hyperconverged/hyperconverged_controller_test.go
✅ Files skipped from review due to trivial changes (4)
- docs/api-v1.md
- docs/api.md
- pkg/featuregatedetails/feature-gates.json
- api/v1/hyperconverged_types.go
🚧 Files skipped from review as they are similar to previous changes (7)
- tools/manifest-templator/generated-crd.yaml
- controllers/handlers/kubevirt.go
- api/v1beta1/conversion_test.go
- api/v1beta1/hyperconverged_types.go
- deploy/olm-catalog/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yaml
- config/crd/bases/hco.kubevirt.io_hyperconvergeds.yaml
- controllers/handlers/kubevirt_test.go
|
@dasionov: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
What this PR does / why we need it:
The VideoConfig feature gate was enabled by default in HCO [1]
as the feature was beta in KubeVirt 1.8 and GA in KubeVirt 1.9 [2].
Now that the feature has reached GA and the feature gate protection has been removed upstream in KubeVirt, there is no point to set it anymore.
This PR modifies HCO to stop adding the VideoConfig feature gate
to the KubeVirt CR and marks the v1beta1 API field as discontinued.
[1] #4180
[2] kubevirt/kubevirt#16599
Reviewer Checklist
Jira Ticket:
Release note: