Move func tests to v1#4276
Conversation
kubevirt-bot
added
release-note-none
|
[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 |
Coverage Report for CI Build 27349649267Coverage increased (+0.008%) to 80.32%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Caution Review failedAn error occurred during the review process. Please try again later. 📝 WalkthroughWalkthroughThis PR migrates functional tests and test utilities to the hcov1 HyperConverged API and updates all field accesses and JSON/merge-patch paths to the reorganized spec: virtualization settings under Spec.Virtualization., deployment under Spec.Deployment., security under Spec.Security.*, and DICT/workload sources under Spec.WorkloadSources. Test helpers (GetHCO/UpdateHCO/PatchHCO/PatchMergeHCO) and debug printers now operate on v1 types; many tests changed cleanup logic to handle nil vs present nested fields and switched some patches from JSONPatch to merge-patch. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Warning 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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/func-tests/debugFuncs_test.go (2)
42-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the lazy failure formatter side-effect free.
This closure appends to the captured
formatandargson every invocation. If Gomega evaluates the message more than once, the suffix and arguments accumulate and the debug output becomes misleading. Build local copies inside the closure instead.Suggested fix
func PrintHyperConvergedBecause(hc *hcov1.HyperConverged, format string, args ...any) func() string { return func() string { - format += "; current HyperConverged CR:\n%s\n" hcYAML := marshalHyperConverged(hc) - args = append(args, hcYAML) - - return fmt.Sprintf(format, args...) + fullFormat := format + "; current HyperConverged CR:\n%s\n" + fullArgs := append(append([]any(nil), args...), hcYAML) + return fmt.Sprintf(fullFormat, fullArgs...) } }As per coding guidelines, "check for determinism to avoid flaky behavior".
🤖 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/debugFuncs_test.go` around lines 42 - 47, The closure returned mutates captured variables `format` and `args` each time it's called, causing the suffix and arguments to accumulate; instead, make local copies inside the closure (e.g., copy `format` to a local `f := format` and `args` to a new slice `a := append([]interface{}{}, args...)`), append the extra format fragment and the `marshalHyperConverged(hc)` result to those locals, then call `fmt.Sprintf(f, a...)` so the original `format` and `args` remain unchanged across invocations.
22-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't mutate the
HyperConvergedobject just to print it.Clearing
hc.ManagedFieldsin place changes the same object the test is still holding. If this printer runs before later assertions or cleanup, the helper has already mutated test state. Marshal aDeepCopy()and stripManagedFieldson the copy instead.Suggested fix
- hc.ManagedFields = nil // remove noise - - hcYAML, err := yaml.Marshal(hc) + hcCopy := hc.DeepCopy() + hcCopy.ManagedFields = nil // remove noise + + hcYAML, err := yaml.Marshal(hcCopy)As per coding guidelines, "avoid introducing flaky behavior".
🤖 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/debugFuncs_test.go` around lines 22 - 25, Don't mutate the original HyperConverged object for logging: use hc.DeepCopy(), set the copy's ManagedFields to nil (e.g., copy := hc.DeepCopy(); copy.ManagedFields = nil) and then call yaml.Marshal(copy) so that yaml.Marshal(copy) produces hcYAML without altering hc; update references to hcYAML and err accordingly (keep function names/vars: hc, ManagedFields, DeepCopy(), yaml.Marshal).tests/func-tests/utils.go (1)
218-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd scheme registration to patch helpers for consistency.
The
PatchHCOandPatchMergeHCOhelpers lack the explicithcov1.AddToSchemecall thatGetHCOandUpdateHCOperform. While the current test setup pre-registers the scheme globally, these helpers should register explicitly to avoid relying on implicit client state and to remain consistent with other mutation helpers.Suggested fix
func PatchHCO(ctx context.Context, cli client.Client, patchBytes []byte) error { + if err := hcov1.AddToScheme(cli.Scheme()); err != nil { + return err + } patch := client.RawPatch(types.JSONPatchType, patchBytes) hco := &hcov1.HyperConverged{ ObjectMeta: metav1.ObjectMeta{ Name: hcoutil.HyperConvergedName, Namespace: InstallNamespace, }, } return cli.Patch(ctx, hco, patch) } func PatchMergeHCO(ctx context.Context, cli client.Client, patchBytes []byte) error { + if err := hcov1.AddToScheme(cli.Scheme()); err != nil { + return err + } patch := client.RawPatch(types.MergePatchType, patchBytes) hco := &hcov1.HyperConverged{ ObjectMeta: metav1.ObjectMeta{ Name: hcoutil.HyperConvergedName, Namespace: InstallNamespace, }, } return cli.Patch(ctx, hco, patch) }🤖 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/utils.go` around lines 218 - 240, Add explicit scheme registration at the start of both PatchHCO and PatchMergeHCO by calling hcov1.AddToScheme(scheme.Scheme) (or the test package's scheme variable) before constructing the HyperConverged object and creating the patch; update imports if needed to reference the scheme package. Ensure the call appears exactly in PatchHCO and PatchMergeHCO so they match GetHCO/UpdateHCO behavior and do not rely on global pre-registration.
🧹 Nitpick comments (5)
tests/func-tests/conversion_test.go (2)
87-88: ⚡ Quick winUse shared feature-gate constants instead of string literals.
Hardcoded gate names make refactors brittle and are easy to mistype; use the shared constants from the feature-gates API package.
As per coding guidelines, "Go-level review: use shared constants for feature gate names, annotation keys, and label values (flag duplicated string literals)."
🤖 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/conversion_test.go` around lines 87 - 88, Replace the hardcoded gate name string literals passed to hcv1.Spec.FeatureGates.IsEnabled (currently "downwardMetrics" and "videoConfig") with the shared feature-gate constants from the feature-gates API package; update the test file imports to include that package (e.g., the feature-gates package) and call the appropriate exported constant identifiers instead of raw strings so the Expect calls use feature-gate constants rather than literals.
75-79: ⚡ Quick winPrefer
PatchoverUpdatefor this mutation.This path only changes two feature-gate fields;
Updatebroadens write scope and increases conflict risk in func tests.Proposed change
- hcv1beta1.Spec.FeatureGates.VideoConfig = ptr.To(false) - hcv1beta1.Spec.FeatureGates.DownwardMetrics = ptr.To(true) - - g.Expect(cli.Update(ctx, hcv1beta1)).To(Succeed()) + orig := hcv1beta1.DeepCopy() + hcv1beta1.Spec.FeatureGates.VideoConfig = ptr.To(false) + hcv1beta1.Spec.FeatureGates.DownwardMetrics = ptr.To(true) + + g.Expect(cli.Patch(ctx, hcv1beta1, client.MergeFrom(orig))).To(Succeed())As per coding guidelines, "For files matching tests/**/*.go: ... prefer Patch over full Update for API operations."
🤖 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/conversion_test.go` around lines 75 - 79, The test is using cli.Update to change only two feature-gate fields (FeatureGates.VideoConfig and FeatureGates.DownwardMetrics) which increases conflict risk; replace the full Update with a targeted Patch: before mutating hcv1beta1 create a snapshot (e.g., orig := hcv1beta1.DeepCopy()), apply the two field changes to hcv1beta1, then call cli.Patch(ctx, hcv1beta1, client.MergeFrom(orig)) so only the delta is sent (ensure client.MergeFrom is used from controller-runtime client).tests/func-tests/golden_image_test.go (1)
379-383: ⚡ Quick winReplace duplicated feature-gate string literals with the shared constant.
"enableMultiArchBootImageImport"is repeated in multiple places; this makes rename/refactor errors more likely.As per coding guidelines, "use shared constants for feature gate names, annotation keys, and label values (flag duplicated string literals)".
Also applies to: 413-415
🤖 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/golden_image_test.go` around lines 379 - 383, Replace the duplicated string literal "enableMultiArchBootImageImport" with the shared constant from the featuregates package (e.g. featuregates.EnableMultiArchBootImageImport) in both the IsEnabled call and the slices.IndexFunc comparator (and the other occurrence noted later); update the two places where hc.Spec.FeatureGates is checked/queried to use that constant instead of the raw string so renames/refactors stay correct.tests/func-tests/hypervisors_test.go (1)
29-35: ⚡ Quick winUse a defensive copy when snapshotting/restoring Hypervisors slice.
This currently reuses the same slice header from the fetched API object. Copying avoids shared-reference bugs if later mutations are introduced.
Proposed fix
- initialHypervisors = hc.Spec.Virtualization.Hypervisors + initialHypervisors = append([]kubevirtcorev1.HypervisorConfiguration(nil), hc.Spec.Virtualization.Hypervisors...) ... - hc.Spec.Virtualization.Hypervisors = initialHypervisors + hc.Spec.Virtualization.Hypervisors = append([]kubevirtcorev1.HypervisorConfiguration(nil), initialHypervisors...)As per coding guidelines "when assigning slices from Kubernetes API objects into other objects, use
DeepCopy()to prevent shared-reference bugs."🤖 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/hypervisors_test.go` around lines 29 - 35, When snapshotting and restoring the Hypervisors slice, avoid reusing the API object's slice header by making a defensive copy; in the setup where you assign initialHypervisors = hc.Spec.Virtualization.Hypervisors use a deep copy (e.g., call DeepCopy() on the parent object or allocate and copy the slice contents) so subsequent mutations don't share the same underlying array, and when restoring in AfterEach (where you set hc.Spec.Virtualization.Hypervisors = initialHypervisors and call tests.UpdateHCORetry) assign the copied slice back to the object instead of the original reference.tests/func-tests/aaq_test.go (1)
96-126: ⚡ Quick winMark test helpers with
GinkgoHelper().
enableAAQ,disableAAQ, andsetAAQEnablementare helper functions and should callGinkgoHelper()so failures point to callsites instead of helper internals.Proposed fix
func enableAAQ(ctx context.Context, cli client.Client) { + GinkgoHelper() By("enable the AAQ FG") setAAQEnablement(ctx, cli, true) } func disableAAQ(ctx context.Context, cli client.Client) { + GinkgoHelper() By("disable the AAQ FG") setAAQEnablement(ctx, cli, false) ... } func setAAQEnablement(ctx context.Context, cli client.Client, fgState bool) { + GinkgoHelper() patchBytes := []byte(fmt.Sprintf(setAAQFGPatchTemplate, fgState)) ... }As per coding guidelines "inside helper functions call
GinkgoHelper()(don’t rely on Gomega offset helpers)."🤖 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/aaq_test.go` around lines 96 - 126, The helper functions enableAAQ, disableAAQ, and setAAQEnablement should call GinkgoHelper() at their start so test failures are attributed to the call site; update each function (enableAAQ, disableAAQ, setAAQEnablement) to invoke GinkgoHelper() as the first statement before any By/Eventually/Patch calls to comply with the Ginkgo helper guideline.
🤖 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/aaq_test.go`:
- Line 25: The patch template variable setAAQFGPatchTemplate currently targets
spec.deployment.applicationAwareConfig.enable but this migration layer should
toggle the AAQ setting under Spec.Virtualization; update the template value so
it patches spec.virtualization.applicationAwareConfig.enable (replace the
deployment path with the virtualization path) so the test toggles the correct
field.
In `@tests/func-tests/cluster_eviction_strategy_test.go`:
- Around line 45-49: The AfterEach cleanup must build a valid JSON patch
depending on whether initialEvictionStrategy is nil: if initialEvictionStrategy
== nil, create a JSON patch with op "remove" for the evictionStrategy path;
otherwise create a JSON patch with op "replace" and the actual string value (use
*initialEvictionStrategy, not the pointer) as the value (ensure you format raw
string into the JSON, not the pointer address). Replace the current
fmt.Sprintf(setEvictionStrategyPatch, initialEvictionStrategy) call used with
tests.PatchHCO in the cleanup with logic that picks the correct patch payload
and passes []byte(patch) to tests.PatchHCO.
In `@tests/func-tests/console_plugin_test.go`:
- Around line 140-142: The JSON patch in removeNodeSelectorPatch uses the
singular path "/spec/deployment/nodePlacement/infra" but the API expects the
plural "nodePlacements"; update removeNodeSelectorPatch to remove
"/spec/deployment/nodePlacements/infra" (matching the existing
addNodeSelectorPatch) so that the call to tests.PatchHCO actually removes the
node selector and the subsequent Eventually blocks can proceed.
- Around line 99-100: The test builds a malformed JSON merge patch and compares
different types: change expectedNodeSelector from a raw JSON string to a
map[string]string (e.g. map[string]string{"foo":"bar"}), marshal that map with
json.Marshal when creating addNodeSelectorPatch (use fmt.Sprintf with the
marshalled bytes inserted into a valid JSON merge patch like
{"spec":{"deployment":{"nodePlacements":{"infra":{"nodeSelector": ... }}}}}) so
the patch JSON is well-formed and the types match the target
Spec.Template.Spec.NodeSelector; also correct the JSON patch path from the
singular "nodePlacement" to the plural "/spec/deployment/nodePlacements/infra"
when applying/removing the patch.
In `@tests/func-tests/defaults_test.go`:
- Around line 90-97: Inside the Eventually block, replace the package-level
Expect calls with the provided local Gomega instance (use g.Expect instead of
Expect) so failures are handled by the poller, and remove the redundant
reflect.DeepEqual assertion comparing hc.Spec.FeatureGates to
defaultFeatureGates (the per-gate loop already validates each gate and the map
uses different concrete matcher types so the DeepEqual check is invalid); update
the block around Eventually, the loop over defaultFeatureGates, and remove the
line that calls reflect.DeepEqual.
In `@tests/func-tests/golden_image_test.go`:
- Line 385: The JSON patch byte literal assigned to the variable patch is
malformed (invalid quoting/structure) and must be corrected; replace the current
inline byte string with a valid JSON Patch document—for example by constructing
a Go struct or a properly escaped string such as
[]byte(`[{"op":"add","path":"/spec/featureGates/-","value":{"name":"enableMultiArchBootImageImport"}}]`)
for the enable path and the corresponding valid delete/cleanup patch for the
cleanup path, ensuring both occurrences of patch (the add and the cleanup)
produce well-formed JSON before using them in the request.
- Around line 410-420: DeferCleanup is computing idx from a stale hc captured
earlier; before using slices.IndexFunc and building the patch to remove the
"enableMultiArchBootImageImport" gate, re-fetch the current HyperConverged
object inside the DeferCleanup closure (e.g., via the existing test helper that
retrieves HCO or via the client) into a fresh variable, then compute idx from
that fresh object and call tests.PatchHCO(ctx, cli, patch) only if idx > -1 so
cleanup reliably removes the gate when present.
In `@tests/func-tests/node_placement_test.go`:
- Around line 66-76: Store a defensive deep copy of the existing NodePlacements
instead of the raw pointer: replace the assignment to originalNodePlacement with
originalHco.Spec.Deployment.NodePlacements.DeepCopy(), then modify only the
NodeSelector maps on that copied NodePlacement (update Infra.NodeSelector and
Workload.NodeSelector) rather than replacing the whole NodePlacements struct,
and when restoring set hco.Spec.Deployment.NodePlacements =
originalNodePlacement.DeepCopy() to avoid shared-reference mutation and preserve
Affinity/Tolerations.
In `@tests/func-tests/role_aggregation_strategy_test.go`:
- Around line 30-32: The AfterEach currently ignores the result of
tests.PatchHCO when applying rmRoleAgrrStrgPatch; capture its error return
instead and assert success (e.g., err := tests.PatchHCO(ctx, cli,
rmRoleAgrrStrgPatch) followed by an Expect/require that err did not occur) so
cleanup failures fail the test rather than being silently ignored; alternatively
use DeferCleanup to ensure tests.PatchHCO is run and its result asserted to
restore mutated HCO state.
In `@tests/func-tests/wasp_agent_test.go`:
- Around line 62-64: The snapshot logic treats originalOvercommitPercent as an
int sentinel so an explicit zero is lost; change originalOvercommitPercent to a
pointer (e.g., *int) or pair it with a boolean flag so you can distinguish
"unset" vs "0", populate it from
originalHco.Spec.Virtualization.HigherWorkloadDensity.MemoryOvercommitPercentage
only when HigherWorkloadDensity != nil, and update any restoration/cleanup
condition (currently checking originalOvercommitPercent != 0) to check for
nil/presence instead before restoring in the teardown logic (referencing
originalHco, originalOvercommitPercent, HigherWorkloadDensity,
MemoryOvercommitPercentage).
---
Outside diff comments:
In `@tests/func-tests/debugFuncs_test.go`:
- Around line 42-47: The closure returned mutates captured variables `format`
and `args` each time it's called, causing the suffix and arguments to
accumulate; instead, make local copies inside the closure (e.g., copy `format`
to a local `f := format` and `args` to a new slice `a := append([]interface{}{},
args...)`), append the extra format fragment and the `marshalHyperConverged(hc)`
result to those locals, then call `fmt.Sprintf(f, a...)` so the original
`format` and `args` remain unchanged across invocations.
- Around line 22-25: Don't mutate the original HyperConverged object for
logging: use hc.DeepCopy(), set the copy's ManagedFields to nil (e.g., copy :=
hc.DeepCopy(); copy.ManagedFields = nil) and then call yaml.Marshal(copy) so
that yaml.Marshal(copy) produces hcYAML without altering hc; update references
to hcYAML and err accordingly (keep function names/vars: hc, ManagedFields,
DeepCopy(), yaml.Marshal).
In `@tests/func-tests/utils.go`:
- Around line 218-240: Add explicit scheme registration at the start of both
PatchHCO and PatchMergeHCO by calling hcov1.AddToScheme(scheme.Scheme) (or the
test package's scheme variable) before constructing the HyperConverged object
and creating the patch; update imports if needed to reference the scheme
package. Ensure the call appears exactly in PatchHCO and PatchMergeHCO so they
match GetHCO/UpdateHCO behavior and do not rely on global pre-registration.
---
Nitpick comments:
In `@tests/func-tests/aaq_test.go`:
- Around line 96-126: The helper functions enableAAQ, disableAAQ, and
setAAQEnablement should call GinkgoHelper() at their start so test failures are
attributed to the call site; update each function (enableAAQ, disableAAQ,
setAAQEnablement) to invoke GinkgoHelper() as the first statement before any
By/Eventually/Patch calls to comply with the Ginkgo helper guideline.
In `@tests/func-tests/conversion_test.go`:
- Around line 87-88: Replace the hardcoded gate name string literals passed to
hcv1.Spec.FeatureGates.IsEnabled (currently "downwardMetrics" and "videoConfig")
with the shared feature-gate constants from the feature-gates API package;
update the test file imports to include that package (e.g., the feature-gates
package) and call the appropriate exported constant identifiers instead of raw
strings so the Expect calls use feature-gate constants rather than literals.
- Around line 75-79: The test is using cli.Update to change only two
feature-gate fields (FeatureGates.VideoConfig and FeatureGates.DownwardMetrics)
which increases conflict risk; replace the full Update with a targeted Patch:
before mutating hcv1beta1 create a snapshot (e.g., orig :=
hcv1beta1.DeepCopy()), apply the two field changes to hcv1beta1, then call
cli.Patch(ctx, hcv1beta1, client.MergeFrom(orig)) so only the delta is sent
(ensure client.MergeFrom is used from controller-runtime client).
In `@tests/func-tests/golden_image_test.go`:
- Around line 379-383: Replace the duplicated string literal
"enableMultiArchBootImageImport" with the shared constant from the featuregates
package (e.g. featuregates.EnableMultiArchBootImageImport) in both the IsEnabled
call and the slices.IndexFunc comparator (and the other occurrence noted later);
update the two places where hc.Spec.FeatureGates is checked/queried to use that
constant instead of the raw string so renames/refactors stay correct.
In `@tests/func-tests/hypervisors_test.go`:
- Around line 29-35: When snapshotting and restoring the Hypervisors slice,
avoid reusing the API object's slice header by making a defensive copy; in the
setup where you assign initialHypervisors = hc.Spec.Virtualization.Hypervisors
use a deep copy (e.g., call DeepCopy() on the parent object or allocate and copy
the slice contents) so subsequent mutations don't share the same underlying
array, and when restoring in AfterEach (where you set
hc.Spec.Virtualization.Hypervisors = initialHypervisors and call
tests.UpdateHCORetry) assign the copied slice back to the object instead of the
original reference.
🪄 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: 61e82ce9-f3f5-4d2e-b89c-2b13d915b44f
📒 Files selected for processing (15)
tests/func-tests/aaq_test.gotests/func-tests/cluster_eviction_strategy_test.gotests/func-tests/console_plugin_test.gotests/func-tests/conversion_test.gotests/func-tests/debugFuncs_test.gotests/func-tests/defaults_test.gotests/func-tests/golden_image_test.gotests/func-tests/hypervisors_test.gotests/func-tests/network_policy_test.gotests/func-tests/node_placement_test.gotests/func-tests/role_aggregation_strategy_test.gotests/func-tests/tuningpolicy_test.gotests/func-tests/utils.gotests/func-tests/validation_test.gotests/func-tests/wasp_agent_test.go
|
|
||
| const ( | ||
| setAAQFGPatchTemplate = `[{"op": "replace", "path": "/spec/enableApplicationAwareQuota", "value": %t}]` | ||
| setAAQFGPatchTemplate = `{"spec":{"deployment":{"applicationAwareConfig":{"enable": %t}}}}` |
There was a problem hiding this comment.
Patch path appears off for this migration layer.
This template patches spec.deployment.applicationAwareConfig.enable, but this PR layer is migrating AAQ under Spec.Virtualization.*. If this path is wrong, the test toggles the wrong field and can silently validate the wrong behavior.
Proposed fix
- setAAQFGPatchTemplate = `{"spec":{"deployment":{"applicationAwareConfig":{"enable": %t}}}}`
+ setAAQFGPatchTemplate = `{"spec":{"virtualization":{"applicationAwareConfig":{"enable": %t}}}}`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| setAAQFGPatchTemplate = `{"spec":{"deployment":{"applicationAwareConfig":{"enable": %t}}}}` | |
| setAAQFGPatchTemplate = `{"spec":{"virtualization":{"applicationAwareConfig":{"enable": %t}}}}` |
🤖 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/aaq_test.go` at line 25, The patch template variable
setAAQFGPatchTemplate currently targets
spec.deployment.applicationAwareConfig.enable but this migration layer should
toggle the AAQ setting under Spec.Virtualization; update the template value so
it patches spec.virtualization.applicationAwareConfig.enable (replace the
deployment path with the virtualization path) so the test toggles the correct
field.
There was a problem hiding this comment.
@coderabbitai not sure what you mean by "but this PR layer is migrating AAQ under Spec.Virtualization.*", but the patch is valid.
There was a problem hiding this comment.
🧩 Analysis chain
Let me verify the actual v1 spec struct to confirm the correct path.
🏁 Script executed:
#!/bin/bash
# Find where ApplicationAwareConfig is defined in the v1 API
rg -n "ApplicationAwareConfig" --type go -C 3
Repository: kubevirt/hyperconverged-cluster-operator
Length of output: 43155
@nunnatsa, you are absolutely right — my review comment was incorrect. The codebase confirms that in the v1 API, ApplicationAwareConfig lives under Spec.Deployment, not Spec.Virtualization:
api/v1/hyperconverged_types.goLine 414:ApplicationAwareConfig *ApplicationAwareConfigurationsis a field ofDeploymentConfigcontrollers/handlers/aaq.goLine 29:hc.Spec.Deployment.ApplicationAwareConfig
The patch template {"spec":{"deployment":{"applicationAwareConfig":{"enable": %t}}}} targets exactly the right field. I apologize for the noise.
✏️ Learnings added
Learnt from: nunnatsa
Repo: kubevirt/hyperconverged-cluster-operator PR: 4276
File: tests/func-tests/aaq_test.go:25-25
Timestamp: 2026-05-20T12:19:44.225Z
Learning: In the kubevirt/hyperconverged-cluster-operator repo, the `ApplicationAwareConfig` field (`*ApplicationAwareConfigurations`) is located under `Spec.Deployment` (i.e., `hc.Spec.Deployment.ApplicationAwareConfig`) in the v1 HyperConverged API — NOT under `Spec.Virtualization`. The correct merge-patch path to toggle AAQ enablement is `{"spec":{"deployment":{"applicationAwareConfig":{"enable": <bool>}}}}`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: nunnatsa
Repo: kubevirt/hyperconverged-cluster-operator PR: 4237
File: controllers/handlers/ssp_test.go:403-405
Timestamp: 2026-05-12T06:43:03.761Z
Learning: When reviewing this repo’s Go test code, don’t flag slice-aliasing issues for patterns that do `append(dst, src...)` where `src` is a slice whose `cap == len` (e.g., it was built from slice literals/constructed such that there’s no extra capacity). In that case `append` must allocate a new backing array, so there is no risk of mutating/aliasing an underlying array shared elsewhere (including the specific `hco.Spec.Deployment.NodePlacements.Infra.Tolerations` usage constructed via `commontestutils.SetNodeCustomPlacement`).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/func-tests/utils.go (1)
220-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit scheme registration to
PatchHCOandPatchMergeHCOhelpers.Lines 218 and 231 patch a typed
hcov1.HyperConvergedwithout registeringhcov1in the scheme. This creates fragile, order-dependent behavior: tests relying on implicit prior calls toGetHCOorUpdateHCOwill succeed, but direct patch calls may fail at runtime if the scheme is not already populated. TheUpdateHCOhelper at line 196 explicitly registers the scheme; both patch helpers should follow the same defensive pattern.Proposed fix
func PatchHCO(ctx context.Context, cli client.Client, patchBytes []byte) error { + if err := hcov1.AddToScheme(cli.Scheme()); err != nil { + return err + } patch := client.RawPatch(types.JSONPatchType, patchBytes) hco := &hcov1.HyperConverged{ ObjectMeta: metav1.ObjectMeta{ Name: hcoutil.HyperConvergedName, Namespace: InstallNamespace, }, } return cli.Patch(ctx, hco, patch) } // PatchMergeHCO patches the HCO CR using a DynamicClient, it can return errors on failures func PatchMergeHCO(ctx context.Context, cli client.Client, patchBytes []byte) error { + if err := hcov1.AddToScheme(cli.Scheme()); err != nil { + return err + } patch := client.RawPatch(types.MergePatchType, patchBytes) hco := &hcov1.HyperConverged{ ObjectMeta: metav1.ObjectMeta{ Name: hcoutil.HyperConvergedName, Namespace: InstallNamespace, }, } return cli.Patch(ctx, hco, patch) }🤖 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/utils.go` around lines 220 - 240, PatchHCO and PatchMergeHCO create a typed hcov1.HyperConverged and call cli.Patch without ensuring the hcov1 types are registered in the client scheme; update both helpers to explicitly register the hcov1 scheme before creating/patching the object (mirror the pattern used in UpdateHCO) by calling the hcov1.AddToScheme on the scheme used by the client (or otherwise ensuring the client's scheme contains hcov1) so the Patch calls are not order-dependent.
♻️ Duplicate comments (3)
tests/func-tests/console_plugin_test.go (1)
105-109:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix malformed merge-patch payload for nodeSelector propagation.
Line 105 builds invalid JSON (
"spec:/"deployment:), sotests.PatchMergeHCOin Line 108 cannot apply the change reliably.Proposed fix
- addNodeSelectorPatch := []byte(fmt.Sprintf(`{"spec:{"deployment:{"nodePlacements":{"infra":{"nodeSelector": %s}}}}}`, string(expectedNodeSelectorBytes))) + addNodeSelectorPatch := []byte(fmt.Sprintf(`{"spec":{"deployment":{"nodePlacements":{"infra":{"nodeSelector": %s}}}}}`, string(expectedNodeSelectorBytes)))🤖 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/console_plugin_test.go` around lines 105 - 109, The patch payload string assigned to addNodeSelectorPatch is malformed (missing closing quotes after spec and deployment) so tests.PatchMergeHCO fails; fix it by constructing valid JSON for the merge-patch (e.g. ensure keys are quoted and braces balanced like {"spec":{"deployment":{"nodePlacements":{"infra":{"nodeSelector": %s}}}}}) and keep using expectedNodeSelectorBytes for the nodeSelector value before converting to []byte and passing addNodeSelectorPatch to tests.PatchMergeHCO.tests/func-tests/golden_image_test.go (1)
417-420:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix invalid JSON patch in multi-arch feature-gate cleanup.
Line 418 has an extra quote (
.../%d""}]), so cleanup patch generation is invalid and may fail to remove the gate.Proposed fix
- patch = []byte(fmt.Sprintf(`[{ "op": "remove", "path": "/spec/featureGates/%d""}]`, idx)) + patch = []byte(fmt.Sprintf(`[{ "op": "remove", "path": "/spec/featureGates/%d"}]`, idx))🤖 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/golden_image_test.go` around lines 417 - 420, The JSON patch string used to remove a feature gate in tests/func-tests/golden_image_test.go contains an extra double quote (`.../%d""`) causing invalid JSON; update the fmt.Sprintf call that sets patch (near the PatchHCO call) to produce valid JSON, e.g. use `fmt.Sprintf(`[{ "op": "remove", "path": "/spec/featureGates/%d"}]`, idx)` so the generated patch is syntactically correct before calling tests.PatchHCO(ctx, cli, patch).tests/func-tests/node_placement_test.go (1)
69-76:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the existing placement object when overriding selectors.
Line 69 still replaces the whole
NodePlacementsvalue, so any pre-existingAffinityorTolerationsare dropped for the duration of the suite. That makes the test less deterministic on clusters that already have placement settings. Start fromoriginalNodePlacement.DeepCopy()and override onlyInfra.NodeSelector/Workload.NodeSelector.Suggested fix
- hco.Spec.Deployment.NodePlacements = &hcov1.NodePlacements{ - Infra: &sdkapi.NodePlacement{ - NodeSelector: map[string]string{hcoLabel: infra}, - }, - Workload: &sdkapi.NodePlacement{ - NodeSelector: map[string]string{hcoLabel: workloads}, - }, - } + hco.Spec.Deployment.NodePlacements = originalNodePlacement.DeepCopy() + if hco.Spec.Deployment.NodePlacements == nil { + hco.Spec.Deployment.NodePlacements = &hcov1.NodePlacements{} + } + if hco.Spec.Deployment.NodePlacements.Infra == nil { + hco.Spec.Deployment.NodePlacements.Infra = &sdkapi.NodePlacement{} + } + if hco.Spec.Deployment.NodePlacements.Workload == nil { + hco.Spec.Deployment.NodePlacements.Workload = &sdkapi.NodePlacement{} + } + hco.Spec.Deployment.NodePlacements.Infra.NodeSelector = map[string]string{hcoLabel: infra} + hco.Spec.Deployment.NodePlacements.Workload.NodeSelector = map[string]string{hcoLabel: workloads}As per coding guidelines, "When assigning slices from Kubernetes API objects into other objects (common in test setup/cleanup), use DeepCopy() to avoid shared-reference bugs (e.g., storing/restoring NodePlacements/feature-gates maps)."
🤖 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/node_placement_test.go` around lines 69 - 76, The test currently replaces hco.Spec.Deployment.NodePlacements entirely which drops any existing Affinity/Tolerations; instead, start from a copy of the original placement (use originalNodePlacement.DeepCopy()), assign that copy to hco.Spec.Deployment.NodePlacements, and then only set/override the Infra.NodeSelector and Workload.NodeSelector keys on that copy (preserving existing Affinity, Tolerations and other fields). Ensure you mutate the DeepCopy() result rather than assigning a new literal so shared-reference bugs are avoided.
🤖 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`:
- Line 81: The assertion message for the "containerPathVolumes" feature gate in
defaults_test.go contains a typo ("shouldexpectedVal"); update the message
passed to BeFalseBecause() for the "containerPathVolumes" entry to a clear,
readable phrase like "the containerPathVolumes feature gate should be disabled
by default" so test failure output is understandable.
In `@tests/func-tests/hypervisors_test.go`:
- Around line 29-35: The test captures a direct reference to
hc.Spec.Virtualization.Hypervisors into initialHypervisors, risking shared
backing array mutation across tests; fix by making a deep copy of the slice when
storing the baseline (replace the direct assignment of initialHypervisors with a
copied slice constructed from hc.Spec.Virtualization.Hypervisors), and when
restoring in AfterEach assign that copied slice back to
hc.Spec.Virtualization.Hypervisors before calling tests.UpdateHCORetry; locate
the assignment to initialHypervisors and the restore in AfterEach around the
calls to tests.GetHCO and tests.UpdateHCORetry to apply the change.
In `@tests/func-tests/role_aggregation_strategy_test.go`:
- Line 22: The cleanup JSON Patch rmRoleAgrrStrgPatch should be made idempotent
so a second remove doesn't fail; replace the single remove op with a safe two-op
patch that first "tests" the existence of
"/spec/virtualization/roleAggregationStrategy" and only then "removes" it (or
alternatively change the teardown to apply the remove and ignore "path not
found" errors). Update the rmRoleAgrrStrgPatch definition and the AfterEach
teardown that uses it (rmRoleAgrrStrgPatch) so applying cleanup twice is a no-op
rather than an error.
---
Outside diff comments:
In `@tests/func-tests/utils.go`:
- Around line 220-240: PatchHCO and PatchMergeHCO create a typed
hcov1.HyperConverged and call cli.Patch without ensuring the hcov1 types are
registered in the client scheme; update both helpers to explicitly register the
hcov1 scheme before creating/patching the object (mirror the pattern used in
UpdateHCO) by calling the hcov1.AddToScheme on the scheme used by the client (or
otherwise ensuring the client's scheme contains hcov1) so the Patch calls are
not order-dependent.
---
Duplicate comments:
In `@tests/func-tests/console_plugin_test.go`:
- Around line 105-109: The patch payload string assigned to addNodeSelectorPatch
is malformed (missing closing quotes after spec and deployment) so
tests.PatchMergeHCO fails; fix it by constructing valid JSON for the merge-patch
(e.g. ensure keys are quoted and braces balanced like
{"spec":{"deployment":{"nodePlacements":{"infra":{"nodeSelector": %s}}}}}) and
keep using expectedNodeSelectorBytes for the nodeSelector value before
converting to []byte and passing addNodeSelectorPatch to tests.PatchMergeHCO.
In `@tests/func-tests/golden_image_test.go`:
- Around line 417-420: The JSON patch string used to remove a feature gate in
tests/func-tests/golden_image_test.go contains an extra double quote
(`.../%d""`) causing invalid JSON; update the fmt.Sprintf call that sets patch
(near the PatchHCO call) to produce valid JSON, e.g. use `fmt.Sprintf(`[{ "op":
"remove", "path": "/spec/featureGates/%d"}]`, idx)` so the generated patch is
syntactically correct before calling tests.PatchHCO(ctx, cli, patch).
In `@tests/func-tests/node_placement_test.go`:
- Around line 69-76: The test currently replaces
hco.Spec.Deployment.NodePlacements entirely which drops any existing
Affinity/Tolerations; instead, start from a copy of the original placement (use
originalNodePlacement.DeepCopy()), assign that copy to
hco.Spec.Deployment.NodePlacements, and then only set/override the
Infra.NodeSelector and Workload.NodeSelector keys on that copy (preserving
existing Affinity, Tolerations and other fields). Ensure you mutate the
DeepCopy() result rather than assigning a new literal so shared-reference bugs
are avoided.
🪄 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: c8316704-f9a5-456d-8f56-4bfb929ea3b0
📒 Files selected for processing (15)
tests/func-tests/aaq_test.gotests/func-tests/cluster_eviction_strategy_test.gotests/func-tests/console_plugin_test.gotests/func-tests/conversion_test.gotests/func-tests/debugFuncs_test.gotests/func-tests/defaults_test.gotests/func-tests/golden_image_test.gotests/func-tests/hypervisors_test.gotests/func-tests/network_policy_test.gotests/func-tests/node_placement_test.gotests/func-tests/role_aggregation_strategy_test.gotests/func-tests/tuningpolicy_test.gotests/func-tests/utils.gotests/func-tests/validation_test.gotests/func-tests/wasp_agent_test.go
| initialHypervisors = hc.Spec.Virtualization.Hypervisors | ||
| }) | ||
|
|
||
| AfterEach(func(ctx context.Context) { | ||
| hc := tests.GetHCO(ctx, cli) | ||
| hc.Spec.Hypervisors = initialHypervisors | ||
| hc.Spec.Virtualization.Hypervisors = initialHypervisors | ||
| _ = tests.UpdateHCORetry(ctx, cli, hc) |
There was a problem hiding this comment.
Deep-copy the Hypervisors slice before storing/restoring test baseline.
Line 29 stores a direct slice reference from the API object; restoring that same reference in Line 34 can couple tests through shared backing data. Copy the slice when capturing initialHypervisors.
Proposed fix
- initialHypervisors = hc.Spec.Virtualization.Hypervisors
+ initialHypervisors = append([]kubevirtcorev1.HypervisorConfiguration(nil), hc.Spec.Virtualization.Hypervisors...)As per coding guidelines, "When assigning slices from Kubernetes API objects into other objects ... use DeepCopy() to avoid shared-reference bugs."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| initialHypervisors = hc.Spec.Virtualization.Hypervisors | |
| }) | |
| AfterEach(func(ctx context.Context) { | |
| hc := tests.GetHCO(ctx, cli) | |
| hc.Spec.Hypervisors = initialHypervisors | |
| hc.Spec.Virtualization.Hypervisors = initialHypervisors | |
| _ = tests.UpdateHCORetry(ctx, cli, hc) | |
| initialHypervisors = append([]kubevirtcorev1.HypervisorConfiguration(nil), hc.Spec.Virtualization.Hypervisors...) | |
| }) | |
| AfterEach(func(ctx context.Context) { | |
| hc := tests.GetHCO(ctx, cli) | |
| hc.Spec.Virtualization.Hypervisors = initialHypervisors | |
| _ = tests.UpdateHCORetry(ctx, cli, hc) |
🤖 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/hypervisors_test.go` around lines 29 - 35, The test captures
a direct reference to hc.Spec.Virtualization.Hypervisors into
initialHypervisors, risking shared backing array mutation across tests; fix by
making a deep copy of the slice when storing the baseline (replace the direct
assignment of initialHypervisors with a copied slice constructed from
hc.Spec.Virtualization.Hypervisors), and when restoring in AfterEach assign that
copied slice back to hc.Spec.Virtualization.Hypervisors before calling
tests.UpdateHCORetry; locate the assignment to initialHypervisors and the
restore in AfterEach around the calls to tests.GetHCO and tests.UpdateHCORetry
to apply the change.
|
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. |
nunnatsa
force-pushed
the
v1-func-tests
branch
2 times, most recently
from
f721d15 to
0830458
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/func-tests/debugFuncs_test.go (1)
22-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid mutating the original HCO object while preparing debug output (Line 22).
marshalHyperConvergedcurrently edits the passed object (hc.ManagedFields = nil). That can leak side effects into test state and make failures harder to reason about. Marshal a copy instead.Proposed fix
func marshalHyperConverged(hc *hcov1.HyperConverged) string { ginkgo.GinkgoHelper() if hc == nil { return "<nil>" } - hc.ManagedFields = nil // remove noise + hcCopy := hc.DeepCopy() + hcCopy.ManagedFields = nil // remove noise - hcYAML, err := yaml.Marshal(hc) + hcYAML, err := yaml.Marshal(hcCopy) Expect(err).NotTo(HaveOccurred()) return string(hcYAML) }🤖 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/debugFuncs_test.go` around lines 22 - 25, marshalHyperConverged mutates the passed HCO object by doing "hc.ManagedFields = nil"; instead make and use a copy of the HCO before clearing ManagedFields to avoid side effects. Replace the in-place mutation with creating a deep copy (e.g., call hc.DeepCopy() or otherwise clone the object into a new variable), set ManagedFields = nil on the copy, and marshal the copy (leave the original hc untouched).tests/func-tests/utils.go (2)
183-185:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeep-copy metadata collections before assignment to avoid shared-reference side effects.
Annotations/Labels(maps) andFinalizers(slice) are assigned by reference frominputinto fetched objects in both retry/update paths. That can leak mutations between retries/tests.Proposed fix
+func cloneStringMap(in map[string]string) map[string]string { + if in == nil { + return nil + } + out := make(map[string]string, len(in)) + for k, v := range in { + out[k] = v + } + return out +} + ... - hco.ObjectMeta.Annotations = input.ObjectMeta.Annotations - hco.ObjectMeta.Finalizers = input.ObjectMeta.Finalizers - hco.ObjectMeta.Labels = input.ObjectMeta.Labels + hco.ObjectMeta.Annotations = cloneStringMap(input.ObjectMeta.Annotations) + hco.ObjectMeta.Finalizers = append([]string(nil), input.ObjectMeta.Finalizers...) + hco.ObjectMeta.Labels = cloneStringMap(input.ObjectMeta.Labels) ... - hco.Annotations = input.Annotations - hco.Finalizers = input.Finalizers - hco.Labels = input.Labels + hco.Annotations = cloneStringMap(input.Annotations) + hco.Finalizers = append([]string(nil), input.Finalizers...) + hco.Labels = cloneStringMap(input.Labels)As per coding guidelines: “When assigning slices/collections from Kubernetes API objects into other objects, use DeepCopy() to prevent shared-reference bugs.”
Also applies to: 203-205
🤖 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/utils.go` around lines 183 - 185, The assignments to hco.ObjectMeta.Annotations, hco.ObjectMeta.Finalizers, and hco.ObjectMeta.Labels are copying references from input and must be deep-copied to avoid shared-reference mutations; replace direct reference assignments (hco.ObjectMeta.Annotations, hco.ObjectMeta.Finalizers, hco.ObjectMeta.Labels) with deep copies (e.g., use input.ObjectMeta.DeepCopy() to copy ObjectMeta then assign its Annotations/Labels, and copy Finalizers into a new slice via append or make+copy) and apply the same fix for the other occurrence referenced in the comment (lines 203-205).
220-240:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegister
hcov1scheme inside patch helpers before patching.
PatchHCOandPatchMergeHCOnow usehcov1.HyperConverged, but they don’t callhcov1.AddToScheme(cli.Scheme()). If these helpers run beforeGetHCO/UpdateHCO, patching can fail due to unregistered kind.Proposed fix
func PatchHCO(ctx context.Context, cli client.Client, patchBytes []byte) error { + if err := hcov1.AddToScheme(cli.Scheme()); err != nil { + return err + } patch := client.RawPatch(types.JSONPatchType, patchBytes) hco := &hcov1.HyperConverged{ ... func PatchMergeHCO(ctx context.Context, cli client.Client, patchBytes []byte) error { + if err := hcov1.AddToScheme(cli.Scheme()); err != nil { + return err + } patch := client.RawPatch(types.MergePatchType, patchBytes) hco := &hcov1.HyperConverged{🤖 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/utils.go` around lines 220 - 240, PatchHCO and PatchMergeHCO construct hcov1.HyperConverged objects but never register the hcov1 scheme with the client, causing patch failures when the kind is unregistered; before creating or patching the HCO object call hcov1.AddToScheme(cli.Scheme()) (and handle the error if any) so the HyperConverged type is registered with the client's scheme, then proceed to build hco and call cli.Patch; update both functions (PatchHCO and PatchMergeHCO) to perform this scheme registration.
♻️ Duplicate comments (1)
tests/func-tests/golden_image_test.go (1)
377-425:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the original multi-arch feature-gate state in cleanup.
This setup/cleanup pair still pollutes cluster state.
BeforeEachmay enableenableMultiArchBootImageImport, butDeferCleanupalways removes the entry if it exists, so a gate that was already present before the test gets deleted instead of restored. The remove patch is also malformed at Line 418 (%d""), so cleanup can fail before it even runs.Please capture whether the gate existed and its original state before patching, then in cleanup either remove only the entry this test added or restore the previous state. As per coding guidelines, "restore any modified HCO/KubeVirt resources via DeferCleanup/AfterEach/AfterAll to avoid test pollution."
🤖 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/golden_image_test.go` around lines 377 - 425, The cleanup currently always removes the "enableMultiArchBootImageImport" entry and uses a malformed remove patch (`/spec/featureGates/%d""`), which can delete a pre-existing gate instead of restoring state; before you patch in the test (where you call tests.PatchHCO and compute idx for enableMultiArchBootImageImport), capture whether the feature gate existed and its original state (e.g., store a boolean existed and the original state string), then in the DeferCleanup closure use that captured info to either remove only the entry the test added (if existed==false) or restore the original state value (if existed==true) by building a correct JSON patch (fix the remove patch format and use a replace patch to restore state when needed) and call tests.PatchHCO via the same Eventually pattern so the cluster is returned to its prior state.
🧹 Nitpick comments (2)
tests/func-tests/conversion_test.go (1)
87-88: ⚡ Quick winReplace raw feature-gate string literals with shared constants.
Using package constants for gate names here avoids typo drift during API migrations and keeps assertions aligned with canonical names.
As per coding guidelines: "prefer shared constants for feature-gate names/annotation keys/label values; avoid duplicated string literals where a constant exists."
🤖 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/conversion_test.go` around lines 87 - 88, Replace the raw feature-gate string literals used in the assertions (the "downwardMetrics" and "videoConfig" arguments to hcv1.Spec.FeatureGates.IsEnabled in conversion_test.go) with the canonical shared constants from the feature-gate constants package (e.g., use features.DownwardMetrics and features.VideoConfig or the equivalent constant names your repo exposes); update the test import to include that constants package and use the constants in both IsEnabled(...) calls and in the BeTrueBecause/BeFalseBecause messages so the assertions reference the single source-of-truth names.tests/func-tests/aaq_test.go (1)
101-126: ⚡ Quick winUse
GinkgoHelper()in these test helpers to anchor stack traces correctly.Helpers like
disableAAQandsetAAQEnablementshould mark themselves withGinkgoHelper()and dropWithOffsetto ensure failure stack traces point to the actual test caller, not the helper internals.Proposed changes
func disableAAQ(ctx context.Context, cli client.Client) { + GinkgoHelper() By("disable the AAQ FG") setAAQEnablement(ctx, cli, false) By("make sure the AAQ CR was removed") Eventually(func(ctx context.Context) error { _, err := getAAQ(ctx, cli) return err }).WithTimeout(10 * time.Minute). WithPolling(10 * time.Second). - WithOffset(1). WithContext(ctx). Should(MatchError(errors.IsNotFound, "not found error")) } func setAAQEnablement(ctx context.Context, cli client.Client, fgState bool) { + GinkgoHelper() patchBytes := []byte(fmt.Sprintf(setAAQFGPatchTemplate, fgState)) Eventually(tests.PatchMergeHCO). WithArguments(ctx, cli, patchBytes). WithTimeout(10 * time.Second). WithPolling(100 * time.Millisecond). WithContext(ctx). - WithOffset(2). Should(Succeed()) }🤖 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/aaq_test.go` around lines 101 - 126, The helper functions disableAAQ and setAAQEnablement need to call GinkgoHelper() at their start and remove the explicit WithOffset(...) chaining so test failures report the caller location; add a call to GinkgoHelper() as the first statement in disableAAQ and setAAQEnablement, and delete the WithOffset(1) and WithOffset(2) invocations in the Eventually chains that call getAAQ and tests.PatchMergeHCO respectively so Ginkgo will anchor stack traces to the real test code.
🤖 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/conversion_test.go`:
- Line 78: The test currently calls cli.Update(ctx, hcv1beta1) which can
overwrite concurrent changes; replace this with a merge patch operation: take a
deep copy of hcv1beta1 (e.g., orig := hcv1beta1.DeepCopy()), modify the copy as
needed, then call cli.Patch(ctx, hcv1beta1, client.MergeFrom(orig)) so the patch
merges only the intended changes instead of performing a full object replace;
ensure you import/use client.MergeFrom and keep ctx and hcv1beta1 identifiers
unchanged.
In `@tests/func-tests/wasp_agent_test.go`:
- Line 32: The JSONPatch in setMemoryOvercommitTemplate uses a "replace" on
/spec/virtualization/higherWorkloadDensity/memoryOvercommitPercentage which will
fail if higherWorkloadDensity doesn't exist; update the test to either (a) use a
JSON Merge Patch instead of JSONPatch so you can set nested fields without
requiring the parent, or (b) change the patch to first ensure the parent exists
(e.g., add/replace /spec/virtualization/higherWorkloadDensity as an object)
before setting memoryOvercommitPercentage; locate and update the
setMemoryOvercommitTemplate constant (and any helper that applies the patch) to
implement the chosen approach so the test is not flaky when
higherWorkloadDensity is missing.
---
Outside diff comments:
In `@tests/func-tests/debugFuncs_test.go`:
- Around line 22-25: marshalHyperConverged mutates the passed HCO object by
doing "hc.ManagedFields = nil"; instead make and use a copy of the HCO before
clearing ManagedFields to avoid side effects. Replace the in-place mutation with
creating a deep copy (e.g., call hc.DeepCopy() or otherwise clone the object
into a new variable), set ManagedFields = nil on the copy, and marshal the copy
(leave the original hc untouched).
In `@tests/func-tests/utils.go`:
- Around line 183-185: The assignments to hco.ObjectMeta.Annotations,
hco.ObjectMeta.Finalizers, and hco.ObjectMeta.Labels are copying references from
input and must be deep-copied to avoid shared-reference mutations; replace
direct reference assignments (hco.ObjectMeta.Annotations,
hco.ObjectMeta.Finalizers, hco.ObjectMeta.Labels) with deep copies (e.g., use
input.ObjectMeta.DeepCopy() to copy ObjectMeta then assign its
Annotations/Labels, and copy Finalizers into a new slice via append or
make+copy) and apply the same fix for the other occurrence referenced in the
comment (lines 203-205).
- Around line 220-240: PatchHCO and PatchMergeHCO construct hcov1.HyperConverged
objects but never register the hcov1 scheme with the client, causing patch
failures when the kind is unregistered; before creating or patching the HCO
object call hcov1.AddToScheme(cli.Scheme()) (and handle the error if any) so the
HyperConverged type is registered with the client's scheme, then proceed to
build hco and call cli.Patch; update both functions (PatchHCO and PatchMergeHCO)
to perform this scheme registration.
---
Duplicate comments:
In `@tests/func-tests/golden_image_test.go`:
- Around line 377-425: The cleanup currently always removes the
"enableMultiArchBootImageImport" entry and uses a malformed remove patch
(`/spec/featureGates/%d""`), which can delete a pre-existing gate instead of
restoring state; before you patch in the test (where you call tests.PatchHCO and
compute idx for enableMultiArchBootImageImport), capture whether the feature
gate existed and its original state (e.g., store a boolean existed and the
original state string), then in the DeferCleanup closure use that captured info
to either remove only the entry the test added (if existed==false) or restore
the original state value (if existed==true) by building a correct JSON patch
(fix the remove patch format and use a replace patch to restore state when
needed) and call tests.PatchHCO via the same Eventually pattern so the cluster
is returned to its prior state.
---
Nitpick comments:
In `@tests/func-tests/aaq_test.go`:
- Around line 101-126: The helper functions disableAAQ and setAAQEnablement need
to call GinkgoHelper() at their start and remove the explicit WithOffset(...)
chaining so test failures report the caller location; add a call to
GinkgoHelper() as the first statement in disableAAQ and setAAQEnablement, and
delete the WithOffset(1) and WithOffset(2) invocations in the Eventually chains
that call getAAQ and tests.PatchMergeHCO respectively so Ginkgo will anchor
stack traces to the real test code.
In `@tests/func-tests/conversion_test.go`:
- Around line 87-88: Replace the raw feature-gate string literals used in the
assertions (the "downwardMetrics" and "videoConfig" arguments to
hcv1.Spec.FeatureGates.IsEnabled in conversion_test.go) with the canonical
shared constants from the feature-gate constants package (e.g., use
features.DownwardMetrics and features.VideoConfig or the equivalent constant
names your repo exposes); update the test import to include that constants
package and use the constants in both IsEnabled(...) calls and in the
BeTrueBecause/BeFalseBecause messages so the assertions reference the single
source-of-truth names.
🪄 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: e781de81-65cf-4752-92f7-8b0bd1a2f35b
📒 Files selected for processing (15)
tests/func-tests/aaq_test.gotests/func-tests/cluster_eviction_strategy_test.gotests/func-tests/console_plugin_test.gotests/func-tests/conversion_test.gotests/func-tests/debugFuncs_test.gotests/func-tests/defaults_test.gotests/func-tests/golden_image_test.gotests/func-tests/hypervisors_test.gotests/func-tests/network_policy_test.gotests/func-tests/node_placement_test.gotests/func-tests/role_aggregation_strategy_test.gotests/func-tests/tuningpolicy_test.gotests/func-tests/utils.gotests/func-tests/validation_test.gotests/func-tests/wasp_agent_test.go
| hcv1beta1.Spec.FeatureGates.DownwardMetrics = ptr.To(true) | ||
|
|
||
| g.Expect(cli.Update(ctx, hcv1)).To(Succeed()) | ||
| g.Expect(cli.Update(ctx, hcv1beta1)).To(Succeed()) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Use patch semantics instead of full object update in this test path.
cli.Update here can clobber unrelated concurrent spec changes and make this functional test flaky; use merge patch from a deep copy instead.
Proposed change
Eventually(func(g Gomega, ctx context.Context) {
hcv1beta1 := &hcov1beta1.HyperConverged{}
g.Expect(cli.Get(ctx, hcKey, hcv1beta1)).To(Succeed())
+ original := hcv1beta1.DeepCopy()
hcv1beta1.Spec.FeatureGates.VideoConfig = ptr.To(false)
hcv1beta1.Spec.FeatureGates.DownwardMetrics = ptr.To(true)
- g.Expect(cli.Update(ctx, hcv1beta1)).To(Succeed())
+ g.Expect(cli.Patch(ctx, hcv1beta1, client.MergeFrom(original))).To(Succeed())
}).WithTimeout(60 * time.Second).As per coding guidelines: "Prefer Patch operations over full Update for API changes."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/func-tests/conversion_test.go` at line 78, The test currently calls
cli.Update(ctx, hcv1beta1) which can overwrite concurrent changes; replace this
with a merge patch operation: take a deep copy of hcv1beta1 (e.g., orig :=
hcv1beta1.DeepCopy()), modify the copy as needed, then call cli.Patch(ctx,
hcv1beta1, client.MergeFrom(orig)) so the patch merges only the intended changes
instead of performing a full object replace; ensure you import/use
client.MergeFrom and keep ctx and hcv1beta1 identifiers unchanged.
| clusterRoleName = "wasp-cluster" | ||
| clusterRoleBindingName = "wasp-cluster" | ||
| setMemoryOvercommitTemplate = `[{"op": "replace", "path": "/spec/higherWorkloadDensity/memoryOvercommitPercentage", "value": %d}]` | ||
| setMemoryOvercommitTemplate = `[{"op": "replace", "path": "/spec/virtualization/higherWorkloadDensity/memoryOvercommitPercentage", "value": %d}]` |
There was a problem hiding this comment.
Avoid JSONPatch replace on potentially missing higherWorkloadDensity path.
If spec.virtualization.higherWorkloadDensity is absent, this replace operation fails and the test can become flaky. Use merge patch (or ensure parent path exists first).
Proposed fix
- setMemoryOvercommitTemplate = `[{"op": "replace", "path": "/spec/virtualization/higherWorkloadDensity/memoryOvercommitPercentage", "value": %d}]`
+ setMemoryOvercommitTemplate = `{"spec":{"virtualization":{"higherWorkloadDensity":{"memoryOvercommitPercentage":%d}}}}`
...
- Eventually(tests.PatchHCO).
+ Eventually(tests.PatchMergeHCO).
WithArguments(ctx, cli, patchBytes).🤖 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/wasp_agent_test.go` at line 32, The JSONPatch in
setMemoryOvercommitTemplate uses a "replace" on
/spec/virtualization/higherWorkloadDensity/memoryOvercommitPercentage which will
fail if higherWorkloadDensity doesn't exist; update the test to either (a) use a
JSON Merge Patch instead of JSONPatch so you can set nested fields without
requiring the parent, or (b) change the patch to first ensure the parent exists
(e.g., add/replace /spec/virtualization/higherWorkloadDensity as an object)
before setting memoryOvercommitPercentage; locate and update the
setMemoryOvercommitTemplate constant (and any helper that applies the patch) to
implement the chosen approach so the test is not flaky when
higherWorkloadDensity is missing.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/func-tests/golden_image_test.go (1)
397-412:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMalformed JSON patch will fail feature-gate cleanup.
Line 405 contains an extra quote character before the closing brace:
%d""}]should be%d"}]. This produces invalid JSON and will cause theDeferCleanupto fail, potentially leaking state across tests.🐛 Proposed fix
DeferCleanup(func(ctx context.Context) { Eventually(func(ctx context.Context) error { cleanupHC := tests.GetHCO(ctx, cli) var patch []byte idx := slices.IndexFunc(cleanupHC.Spec.FeatureGates, func(fg featuregates.FeatureGate) bool { return fg.Name == goldenimages.EnableMultiArchFeatureGate }) if idx > -1 { - patch = []byte(fmt.Sprintf(`[{ "op": "remove", "path": "/spec/featureGates/%d""}]`, idx)) + patch = []byte(fmt.Sprintf(`[{ "op": "remove", "path": "/spec/featureGates/%d"}]`, idx)) return tests.PatchHCO(ctx, cli, patch) } return nil }).WithTimeout(hcTimeout). WithPolling(hcPolling). WithContext(ctx). Should(Succeed())🤖 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/golden_image_test.go` around lines 397 - 412, The JSON patch string in the DeferCleanup closure is malformed: when constructing patch for tests.PatchHCO inside the block that finds the goldenimages.EnableMultiArchFeatureGate (using tests.GetHCO and slices.IndexFunc) the format string contains an extra double-quote (`%d""}]`) which produces invalid JSON; update the patch construction so the format is `[{ "op": "remove", "path": "/spec/featureGates/%d"}]` (i.e., replace `%d""}]` with `%d"}]`) to produce valid JSON before calling tests.PatchHCO.
🧹 Nitpick comments (1)
tests/func-tests/role_aggregation_strategy_test.go (1)
22-22: ⚡ Quick winTypo in variable name:
rmRoleAgrrStrgPatch.The variable name contains "Aggrr" with two consecutive 'r's, which appears to be a typo. It should be "Aggr" (abbreviation of "Aggregation").
📝 Suggested fix
- rmRoleAgrrStrgPatch = []byte(`[{"op": "remove", "path": "/spec/virtualization/roleAggregationStrategy"}]`) + rmRoleAggrStrgPatch = []byte(`[{"op": "remove", "path": "/spec/virtualization/roleAggregationStrategy"}]`)And update all usages at lines 33 and 86.
🤖 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/role_aggregation_strategy_test.go` at line 22, Rename the misspelled variable rmRoleAgrrStrgPatch to rmRoleAggrStrgPatch (fix the "Aggrr" → "Aggr") in its declaration and update every usage/reference to that identifier (e.g., test assertions and patch applications that currently reference rmRoleAgrrStrgPatch) so the symbol names match; ensure no other code or comments still use the old misspelled name.
🤖 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/golden_image_test.go`:
- Around line 878-898: The function enableFG has a control path that falls
through without returning when hc.Spec.FeatureGates is non-nil and the requested
feature gate is already enabled; add an explicit return (e.g., return nil) at
the end of enableFG so every execution path returns an error or nil, ensuring
functions like tests.PatchMergeHCO and tests.PatchHCO remain unchanged and
compile succeeds.
---
Outside diff comments:
In `@tests/func-tests/golden_image_test.go`:
- Around line 397-412: The JSON patch string in the DeferCleanup closure is
malformed: when constructing patch for tests.PatchHCO inside the block that
finds the goldenimages.EnableMultiArchFeatureGate (using tests.GetHCO and
slices.IndexFunc) the format string contains an extra double-quote (`%d""}]`)
which produces invalid JSON; update the patch construction so the format is `[{
"op": "remove", "path": "/spec/featureGates/%d"}]` (i.e., replace `%d""}]` with
`%d"}]`) to produce valid JSON before calling tests.PatchHCO.
---
Nitpick comments:
In `@tests/func-tests/role_aggregation_strategy_test.go`:
- Line 22: Rename the misspelled variable rmRoleAgrrStrgPatch to
rmRoleAggrStrgPatch (fix the "Aggrr" → "Aggr") in its declaration and update
every usage/reference to that identifier (e.g., test assertions and patch
applications that currently reference rmRoleAgrrStrgPatch) so the symbol names
match; ensure no other code or comments still use the old misspelled name.
🪄 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: 9aa32227-8628-4e55-8e88-93894fbe68de
📒 Files selected for processing (15)
tests/func-tests/aaq_test.gotests/func-tests/cluster_eviction_strategy_test.gotests/func-tests/console_plugin_test.gotests/func-tests/conversion_test.gotests/func-tests/debugFuncs_test.gotests/func-tests/defaults_test.gotests/func-tests/golden_image_test.gotests/func-tests/hypervisors_test.gotests/func-tests/network_policy_test.gotests/func-tests/node_placement_test.gotests/func-tests/role_aggregation_strategy_test.gotests/func-tests/tuningpolicy_test.gotests/func-tests/utils.gotests/func-tests/validation_test.gotests/func-tests/wasp_agent_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/func-tests/tuningpolicy_test.go
- tests/func-tests/validation_test.go
- tests/func-tests/hypervisors_test.go
- tests/func-tests/wasp_agent_test.go
- tests/func-tests/console_plugin_test.go
- tests/func-tests/node_placement_test.go
- tests/func-tests/cluster_eviction_strategy_test.go
- tests/func-tests/aaq_test.go
- tests/func-tests/utils.go
- tests/func-tests/network_policy_test.go
- tests/func-tests/defaults_test.go
- tests/func-tests/debugFuncs_test.go
- tests/func-tests/conversion_test.go
nunnatsa
force-pushed
the
v1-func-tests
branch
2 times, most recently
from
d1295b3 to
fc701d9
Compare
|
hco-e2e-operator-sdk-azure, hco-e2e-operator-sdk-gcp lanes succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-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. |
|
/retest |
|
hco-e2e-upgrade-operator-sdk-aws lane succeeded. |
|
/retest |
1 similar comment
|
/retest |
Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
|
|
/retest |
|
hco-e2e-operator-sdk-sno-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-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-kv-smoke-gcp lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: ci/prow/hco-e2e-consecutive-operator-sdk-upgrades-aws, ci/prow/hco-e2e-kv-smoke-azure, ci/prow/hco-e2e-operator-sdk-aws, ci/prow/hco-e2e-operator-sdk-azure, ci/prow/hco-e2e-upgrade-operator-sdk-aws, 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. |
|
/retest |
|
hco-e2e-upgrade-prev-operator-sdk-sno-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: 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. |
|
hco-e2e-upgrade-prev-operator-sdk-sno-azure lane succeeded. |
|
@hco-bot: Overrode contexts on behalf of hco-bot: 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. |
|
/retest |
|
@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. |
4 participants
Jira Ticket:
Release note: