CNV-92360: v1 API version: make feature gate names case-insensitive - #4384
Conversation
kubevirt-prow
Bot
added
release-note
Coverage Report for CI Build 29002673961Coverage increased (+0.006%) to 81.537%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (19)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughThis PR makes HyperConverged feature-gate lookup case-insensitive, updates feature-gate JSON/defaulting behavior, and adds validation for name length, array size, and case-insensitive uniqueness across the CRD and generated manifests. v1beta1 conversion now stores the full Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 13 linked repositories, but your current plan allows 10. Analyzed Linked repositories: Public OSS repositories can only analyze public repositories installed in this organization. Analyzed 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: 5
🧹 Nitpick comments (1)
tools/fg-conversion-generator/conversion.go.tmpl (1)
34-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the shared
Stateconstant over the"Enabled"string literal.The nil-aware enable/disable/delete logic traces correctly for both alpha (default off) and beta (default on) gates. One nit on Line 39: the effective-state check compares against the literal
"Enabled", while the package already exportshcofg.Enabled. Using the constant keeps the generated code aligned with the rest of the codebase and avoids drift if the enum value ever changes.♻️ Proposed tweak
- v1Enabled = v1Found && ((*out)[v1Idx].State == nil || *((*out)[v1Idx].State) == "Enabled") + v1Enabled = v1Found && ((*out)[v1Idx].State == nil || *((*out)[v1Idx].State) == hcofg.Enabled)🤖 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/fg-conversion-generator/conversion.go.tmpl` around lines 34 - 46, Update the enable/disable/delete generation logic in conversion.go.tmpl so the effective-state check in the conversion block uses the shared hcofg.Enabled constant instead of the hardcoded "Enabled" string. Keep the nil-aware behavior in the FieldName handling unchanged, and adjust the comparison around v1Enabled to reference the exported constant so generated code stays consistent with the rest of the codebase.Source: Path instructions
🤖 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 `@api/v1/featuregates/feature_gates_test.go`:
- Line 196: The test case label in feature_gates_test.go is misleading because
it says “disabled” even though the expectation for declarativeHotplugVolumes is
BeTrue() when a known beta gate is absent from the list. Update the Entry
description to match the actual beta-gate behavior, using the surrounding Ginkgo
Entry names in the feature gate test table as the reference so it clearly
reflects that the gate is enabled by default when not listed.
In `@hack/build-manifests.sh`:
- Line 294: The manifest-splitting command in build-manifests.sh uses an
unquoted ${TOOLS} expansion and an unnecessary cat pipe. Update the invocation
around the manifest-splitter call to quote the TOOLS path to prevent
word-splitting/globbing, and replace the cat | pattern with input redirection
while keeping the same --operator-name="hco" behavior.
In `@tests/func-tests/feature_gates_test.go`:
- Around line 81-82: The “all upper case” test case in feature_gates_test is not
actually using an upper-case value, so it duplicates the lower-case path instead
of covering the intended rejection case. Update the call in the addFeatureGate
assertion to pass the upper-case feature gate value (matching the existing
ToUpper-based helper used elsewhere) and keep the By description aligned with
the input being exercised.
- Around line 90-101: The JSON patch in addFeatureGate is wrapping the feature
gate entry in an array, which makes the appended item the wrong shape for
/spec/featureGates/-. Update the patch payload so the value is a single object
with the name field, and keep the change localized in addFeatureGate to ensure
the feature gate list receives one FeatureGate entry as intended.
In `@tests/func-tests/hyperconverged.go`:
- Line 175: The cleanup patch in PatchHCO is malformed JSON and currently cannot
remove /spec/featureGates, which breaks RestoreDefaultFeatureGates cleanup. Fix
the JSON patch string so it is valid and includes the complete remove operation,
using the PatchHCO call in RestoreDefaultFeatureGates as the target to update.
Ensure the resulting patch is syntactically correct so DeferCleanup in
feature_gates_test.go can reliably clear feature gates.
---
Nitpick comments:
In `@tools/fg-conversion-generator/conversion.go.tmpl`:
- Around line 34-46: Update the enable/disable/delete generation logic in
conversion.go.tmpl so the effective-state check in the conversion block uses the
shared hcofg.Enabled constant instead of the hardcoded "Enabled" string. Keep
the nil-aware behavior in the FieldName handling unchanged, and adjust the
comparison around v1Enabled to reference the exported constant so generated code
stays consistent with the rest of the codebase.
🪄 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: 13839c4b-ff40-4da1-8a37-92a29228b77e
⛔ Files ignored due to path filters (1)
api/v1beta1/zz_generated.featuregates_conversion.gois excluded by!**/zz_generated.*
📒 Files selected for processing (15)
Makefileapi/v1/featuregates/feature_gates.goapi/v1/featuregates/feature_gates_test.goapi/v1beta1/conversion.goapi/v1beta1/conversion_test.goconfig/crd/bases/hco.kubevirt.io_hyperconvergeds.yamldeploy/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.yamlhack/build-manifests.shtests/func-tests/feature_gates_test.gotests/func-tests/hyperconverged.gotools/csv-merger/generated-crd.yamltools/fg-conversion-generator/conversion.go.tmpltools/manifest-templator/generated-crd.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
kubevirt/hyperconverged-cluster-operator(manual)kubevirt/monitoring(manual)
069cfe3 to
74f9cea
Compare
kubevirt-prow
Bot
added
dco-signoff: yes
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Makefile (1)
83-83: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
build-crd-creatorprerequisite inbuild-manifests.
build-crd-creatoris already transitively satisfied throughprepare-tools-crd→generate-crd→build-crd-creator. The direct listing is harmless but redundant, andbuild-manifests.shno longer invokescrd-creatordirectly (the "Write HCO CRDs" block was removed).🧹 Proposed cleanup
-build-manifests: build-crd-creator prepare-tools-crd build-csv-merger build-manifest-splitter build-manifest-templator +build-manifests: prepare-tools-crd build-csv-merger build-manifest-splitter build-manifest-templator🤖 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 `@Makefile` at line 83, Remove the redundant build-crd-creator prerequisite from build-manifests in the Makefile, since prepare-tools-crd already pulls it in transitively through generate-crd and the build-manifests.sh flow no longer calls crd-creator directly. Keep build-manifests depending on prepare-tools-crd and the other required targets (build-csv-merger, build-manifest-splitter, build-manifest-templator) so the dependency chain stays intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Makefile`:
- Line 83: Remove the redundant build-crd-creator prerequisite from
build-manifests in the Makefile, since prepare-tools-crd already pulls it in
transitively through generate-crd and the build-manifests.sh flow no longer
calls crd-creator directly. Keep build-manifests depending on prepare-tools-crd
and the other required targets (build-csv-merger, build-manifest-splitter,
build-manifest-templator) so the dependency chain stays intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b4197c77-07b0-4f32-8087-14925c868411
⛔ Files ignored due to path filters (1)
api/v1beta1/zz_generated.featuregates_conversion.gois excluded by!**/zz_generated.*
📒 Files selected for processing (15)
Makefileapi/v1/featuregates/feature_gates.goapi/v1/featuregates/feature_gates_test.goapi/v1beta1/conversion.goapi/v1beta1/conversion_test.goconfig/crd/bases/hco.kubevirt.io_hyperconvergeds.yamldeploy/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.yamlhack/build-manifests.shtests/func-tests/feature_gates_test.gotests/func-tests/hyperconverged.gotools/csv-merger/generated-crd.yamltools/fg-conversion-generator/conversion.go.tmpltools/manifest-templator/generated-crd.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
kubevirt/hyperconverged-cluster-operator(manual)kubevirt/monitoring(manual)
🚧 Files skipped from review as they are similar to previous changes (13)
- deploy/crds/hco00.crd.yaml
- deploy/index-image/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yaml
- tools/csv-merger/generated-crd.yaml
- config/crd/bases/hco.kubevirt.io_hyperconvergeds.yaml
- tools/manifest-templator/generated-crd.yaml
- tools/fg-conversion-generator/conversion.go.tmpl
- api/v1/featuregates/feature_gates_test.go
- tests/func-tests/feature_gates_test.go
- deploy/olm-catalog/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yaml
- tests/func-tests/hyperconverged.go
- api/v1/featuregates/feature_gates.go
- api/v1beta1/conversion_test.go
- api/v1beta1/conversion.go
74f9cea to
bab0ba1
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/func-tests/feature_gates_test.go (2)
90-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCall
GinkgoHelper()inaddFeatureGatefor better failure attribution.
addFeatureGateis a test helper function. As per func-tests guidelines, helper functions should callGinkgoHelper()so failures point to the callingItblock rather than the helper internals.🔧 Proposed fix
func addFeatureGate(ctx context.Context, cli client.Client, fgName string) error { + GinkgoHelper() const (As per path instructions: "In helper functions, use GinkgoHelper() (not Gomega offset functions)."
🤖 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/feature_gates_test.go` around lines 90 - 110, addFeatureGate is a test helper, so it should mark itself with GinkgoHelper() to keep failures attributed to the calling It block instead of the helper internals. Update the addFeatureGate function in feature_gates_test.go to invoke GinkgoHelper() at the start of the helper, alongside the existing retry.RetryOnConflict logic, without changing its patching behavior or error handling.Source: Path instructions
21-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd Ginkgo Labels to categorize the test suite.
The
DescribeandItblocks have no Ginkgo labels. As per func-tests guidelines, functional tests should be categorized with labels for filtering and traceability.🏷️ Suggested labels
-var _ = Describe("test feature gates", func() { +var _ = Describe("test feature gates", Label("feature-gates"), func() {Individual
Itblocks could also carry labels such asLabel("casings")orLabel("validation")depending on the desired granularity.As per path instructions: "Categorize tests with Ginkgo Labels."
🤖 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/feature_gates_test.go` around lines 21 - 87, The feature-gate test suite in the Describe and It blocks needs Ginkgo labels so it can be filtered and categorized consistently. Add the appropriate Label annotations to the top-level Describe("test feature gates", ...) and to each It block in feature_gates_test.go, using symbols like Describe and the individual It descriptions to distinguish cases such as casing and validation. Keep the labels aligned with func-test conventions so the suite is traceable and easy to select by category.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/func-tests/feature_gates_test.go`:
- Around line 90-110: addFeatureGate is a test helper, so it should mark itself
with GinkgoHelper() to keep failures attributed to the calling It block instead
of the helper internals. Update the addFeatureGate function in
feature_gates_test.go to invoke GinkgoHelper() at the start of the helper,
alongside the existing retry.RetryOnConflict logic, without changing its
patching behavior or error handling.
- Around line 21-87: The feature-gate test suite in the Describe and It blocks
needs Ginkgo labels so it can be filtered and categorized consistently. Add the
appropriate Label annotations to the top-level Describe("test feature gates",
...) and to each It block in feature_gates_test.go, using symbols like Describe
and the individual It descriptions to distinguish cases such as casing and
validation. Keep the labels aligned with func-test conventions so the suite is
traceable and easy to select by category.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 763861ff-6ca2-4d58-af40-c6496f46cfdb
⛔ Files ignored due to path filters (1)
api/v1beta1/zz_generated.featuregates_conversion.gois excluded by!**/zz_generated.*
📒 Files selected for processing (15)
Makefileapi/v1/featuregates/feature_gates.goapi/v1/featuregates/feature_gates_test.goapi/v1beta1/conversion.goapi/v1beta1/conversion_test.goconfig/crd/bases/hco.kubevirt.io_hyperconvergeds.yamldeploy/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.yamlhack/build-manifests.shtests/func-tests/feature_gates_test.gotests/func-tests/hyperconverged.gotools/csv-merger/generated-crd.yamltools/fg-conversion-generator/conversion.go.tmpltools/manifest-templator/generated-crd.yaml
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
kubevirt/hyperconverged-cluster-operator(manual)kubevirt/monitoring(manual)
✅ Files skipped from review due to trivial changes (2)
- tools/manifest-templator/generated-crd.yaml
- tools/csv-merger/generated-crd.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- deploy/olm-catalog/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yaml
- deploy/index-image/community-kubevirt-hyperconverged/1.19.0/manifests/hco00.crd.yaml
- config/crd/bases/hco.kubevirt.io_hyperconvergeds.yaml
- tools/fg-conversion-generator/conversion.go.tmpl
- deploy/crds/hco00.crd.yaml
- api/v1/featuregates/feature_gates.go
- api/v1/featuregates/feature_gates_test.go
- api/v1beta1/conversion_test.go
- tests/func-tests/hyperconverged.go
- api/v1beta1/conversion.go
- hack/build-manifests.sh
346094a to
e572d71
Compare
|
hco-e2e-upgrade-operator-sdk-sno-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-sno-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-upgrade-prev-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, 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. |
Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
Also, make sure the feature gate are unique in the FeatureGates list. Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
we only marshal the FeatureGate state field if it disabled, but k8s marshling sometimes keeps it even if it Enabled, so for consistency, we now marshaling the state field if exists, no matter its value. Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
The automatic conversion only handles feature gates that are defined in both v1beta1 and v1 API versions. Any feature gate tat is not defined in v1beta1, will be lost in conversion. This commit preserves all v1 feature gates using the v1-only-fields mechanism. Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
e572d71 to
188f957
Compare
|
|
hco-e2e-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-gcp 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. |
|
@nunnatsa: 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. |
|
/override-bot |
|
hco-e2e-upgrade-operator-sdk-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-upgrade-operator-sdk-aws, ci/prow/hco-e2e-upgrade-prev-operator-sdk-sno-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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: avlitman The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubevirt-prow
Bot
added
the
approved
|
/test pull-hyperconverged-cluster-operator-unit-test-s390x |
|
@nunnatsa: The specified target(s) for The following commands are available to trigger optional jobs: Use 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. |
|
pull-hyperconverged-cluster-operator-unit-test-s390x lane is not starting. It seems the s390x server is down. /override pull-hyperconverged-cluster-operator-unit-test-s390x |
|
@nunnatsa: Overrode contexts on behalf of nunnatsa: pull-hyperconverged-cluster-operator-unit-test-s390x 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. |
4 participants
What this PR does / why we need it:
In v1beta1 API version, the feature gate name were field names and so they always started with a lower case letter. Many feature gates are actually proxy to KubeVirt feature gate, where they are starting with an upper case letter. This may be confusing and error prompt.
To solve this, HCO now treats the feature gate names as case-insensitive; i.e setting a feature gate in any casing will work to enable or disable the feature.
The CRD now also prevent multiple feature gates with the same name, case-insensitive.
Implementing this change, exposed a hidden bug, where v1 only feature gates (that are no exist yet) do not survive
v1=>v1beta1=>v1conversion (e.g. when editing or patching the CR using v1beta1, the API server call the conversion webhook to return the CR in v1beta1, and when it is write back to ETCD, the conversion webhook is called again to store in v1).This PR also solves this issue using the v1-only-field mechanism.
Jira Ticket:
Release note: