WIP: Align automation with hco v1 api#5363
Open
hmeir wants to merge 8 commits into
Open
Conversation
- Switch get_hyperconverged_resource() from V1BETA1 to V1 - Add get_hyperconverged_resource_v1beta1() for backward-compat tests - Update apply_np_changes() for v1 node placement paths (spec.deployment.nodePlacements.infra/workload) - Update enableCommonBootImageImport path to spec.workloadSources.* - Update dataImportCronTemplates path to spec.workloadSources.* - Update enableApplicationAwareQuota to spec.deployment.applicationAwareConfig.enable Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- HCO_DEFAULT_FEATUREGATES: change from dict to empty list (v1 default) - Keep old dict as HCO_V1BETA1_DEFAULT_FEATUREGATES for compat tests - Add FG_STATE_ENABLED/FG_STATE_DISABLED constants for v1 format - Update expected_value fixture to handle v1 list format - Update hyperconverged_with_node_placement for v1 paths (spec.deployment.nodePlacements.infra/workload) Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update all test files that patch or read HCO spec fields to use
v1 hierarchical paths:
- featureGates: dict format → list of {name, state} objects
- liveMigrationConfig, permittedHostDevices, virtualMachineOptions,
ksmConfiguration, higherWorkloadDensity, workloadUpdateStrategy,
obsoleteCPUs→obsoleteCPUModels, evictionStrategy, defaultCPUModel
→ spec.virtualization.*
- scratchSpaceStorageClass, vmStateStorageClass, storageImport
→ spec.storage.*
- certConfig, tlsSecurityProfile → spec.security.*
- kubeMacPoolConfiguration → spec.networking.*
- dataImportCronTemplates, enableCommonBootImageImport,
commonBootImageNamespace, commonTemplatesNamespace
→ spec.workloadSources.*
- uninstallStrategy, deployVmConsoleProxy, logVerbosityConfig,
nodePlacements (infra/workload) → spec.deployment.*
- workloads (plural) → workload (singular) in v1 node placement
- Remove xfail quarantine from test_update_featuregate_hco
- Update API version test to assert V1 instead of V1BETA1
Signed-off-by: Harel Meir <hmeir@redhat.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Update mock HCO spec structures in test_hco.py to match v1 API: - Node placement mocks: spec.deployment.nodePlacements.infra/workload - enableCommonBootImageImport: under spec.workloadSources - enableApplicationAwareQuota: spec.deployment.applicationAwareConfig.enable - workloads_placement param → workload_placement Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add "HCO API Version (v5.0+)" section with: - v1 API mandatory for all HCO CR access - v1 feature gate list format (never dict) - Complete v1 hierarchical field path mapping table - Node placement: workload (singular) in v1 - Explicit carve-out for v1beta1 backward-compat tests CodeRabbit picks up AGENTS.md via code_guidelines, so this enforces v1 API usage on all future PRs automatically. Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Wrap certConfig patches under spec.security and liveMigrationConfig patches under spec.virtualization. Update base_path for HCO spec assertions to use v1 hierarchical paths. Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- test_hco_cr_modify_defaults: wrap certConfig under security, liveMigrationConfig under virtualization, update base_path - test_hco_cr_nondefault_fields: wrap storageImport under storage, infra/workloads under deployment.nodePlacements Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR migrates all HCO CR interactions from the flat v1beta1-style spec layout to the hierarchical HCO v1 API spec structure. ChangesHCO v1 API spec path migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot-4
requested review from
acinko-rh,
albarker-rh and
mijankow
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
openshift-virtualization-qe-bot-4
requested review from
Acedus,
Anatw,
OhadRevah,
josemacassan,
jpeimer,
kshvaika,
orelmisan,
servolkov and
vsibirsk
- tests/virt/cluster/aaq/conftest.py: wrap applicationAwareConfig patches under spec.deployment (Critical - missed in initial migration) - csv/test_hco_api_version.py: use exact equality check for apiVersion instead of substring containment (would pass for v1beta1 too) - strict_reconciliation/conftest.py: use FG_STATE_ENABLED constant instead of hardcoded "Enabled" string - strict_reconciliation/test_hco_cr_nondefault_fields.py: fix test ID from "workloads" to "workload" (v1 singular) - constants.py: add skip-unused-code annotations to forward-provision symbols for upcoming v1beta1 backward-compat tests - infra.py: add skip-unused-code annotation to get_hyperconverged_resource_v1beta1() (no callers yet) Signed-off-by: Harel Meir <hmeir@redhat.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
openshift-virtualization-qe-bot-4
requested review from
RoniKishner,
SamAlber,
akalenyu,
awels,
azhivovk and
ema-aka-young
hmeir
force-pushed
the
align-automation-with-hco-v1-api
branch
from
f75bac3 to
15046e9
Compare
Contributor
Author
|
/wip |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot-4
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/infra.py (1)
611-634: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winHIGH: Type and document the public HCO getters.
get_hyperconverged_resource_v1beta1()is a new public utility underutilities/, so it needs a typed signature. Both helpers now also carry non-obvious behavior (defaulting to v1 vs explicit v1beta1 compatibility, plus raising on miss), so a short Google-style docstring will keep callers from picking the wrong helper.Example
-def get_hyperconverged_resource(client, hco_ns_name): +def get_hyperconverged_resource(client: DynamicClient, hco_ns_name: str) -> HyperConverged: + """Return the v1 HyperConverged resource from the given namespace. + + Args: + client: Cluster client. + hco_ns_name: Namespace containing the HyperConverged resource. + + Returns: + The v1 HyperConverged resource. + + Raises: + ResourceNotFoundError: If the resource does not exist. + """ ... -def get_hyperconverged_resource_v1beta1(client, hco_ns_name): # skip-unused-code +def get_hyperconverged_resource_v1beta1( # skip-unused-code + client: DynamicClient, hco_ns_name: str +) -> HyperConverged: + """Return the v1beta1 HyperConverged resource for compatibility tests. + + Args: + client: Cluster client. + hco_ns_name: Namespace containing the HyperConverged resource. + + Returns: + The v1beta1 HyperConverged resource. + + Raises: + ResourceNotFoundError: If the resource does not exist. + """ ...As per coding guidelines, "Type hints are mandatory for new public Python functions" and "Write Google-format docstrings for public functions that have non-obvious return values or side effects." Based on learnings, functions under
utilities/are treated as public APIs.🤖 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 `@utilities/infra.py` around lines 611 - 634, Both public HCO getter helpers need explicit typing and documentation: add type hints to get_hyperconverged_resource and get_hyperconverged_resource_v1beta1 (including parameter and return types), and add short Google-style docstrings explaining that they resolve a HyperConverged resource for the requested API version and raise ResourceNotFoundError when missing. Keep the behavior in sync with the symbols get_hyperconverged_resource, get_hyperconverged_resource_v1beta1, HyperConverged, and ResourceNotFoundError so callers can clearly choose between the default v1 and compatibility v1beta1 variants.Sources: Coding guidelines, Learnings
🤖 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/install_upgrade_operators/cert_renewal/conftest.py`:
- Around line 42-46: The patch path in the cert renewal fixture hardcodes the
"security" key instead of using the shared HCO_SECURITY_KEY constant. Update the
fixture in conftest.py where the patches dict is built to reference
HCO_SECURITY_KEY in the spec path, matching the existing contract used elsewhere
and keeping the patch aligned with shared patterns.
In `@tests/install_upgrade_operators/csv/test_hco_api_version.py`:
- Around line 13-14: Add a failure message to the apiVersion assertion in the
test that compares hyperconverged_resource_scope_function.instance.apiVersion
against expected_api_version. Update the assertion so it clearly states the
expected/actual contract for this check, following the pytest-with-message
convention used under tests/**, and keep the change localized to the test case
involving HyperConverged.ApiGroup.HCO_KUBEVIRT_IO and Resource.ApiVersion.V1.
In
`@tests/install_upgrade_operators/launcher_updates/test_reset_custom_values.py`:
- Around line 28-47: The parametrized test ids in test_reset_custom_values.py do
not match the fields being patched in the pytest.param cases. Update the id
values in the relevant pytest.param entries so they clearly reflect the actual
reset targets in the WORKLOAD_UPDATE_STRATEGY_KEY_NAME patch, specifically
batchEvictionInterval and batchEvictionSize, to keep pytest node names and
failure output accurate.
In `@tests/install_upgrade_operators/strict_reconciliation/conftest.py`:
- Around line 124-130: The feature-gate patching in the strict-reconciliation
fixture only appends missing names, so any requested gate already present in HCO
featureGates but set to Disabled is left unchanged. Update the logic around
existing_fg_names and updated_fgs so every fg_name in new_fgs is forced to state
FG_STATE_ENABLED, whether it already exists or not. Use the featureGates patch
construction in this fixture to replace the matching entry instead of skipping
it, so ResourceEditorValidateHCOReconcile always applies the requested enabled
state.
In
`@tests/install_upgrade_operators/strict_reconciliation/test_hco_cr_defaults_on_stanza_deletion.py`:
- Around line 272-273: Add descriptive pytest failure messages to the nested
spec assertions in the strict reconciliation test, especially the check in
test_hco_cr_defaults_on_stanza_deletion and the other referenced spec
comparisons. Update the assertions that read from
hyperconverged_resource_scope_function.instance.to_dict()["spec"] so they
include an explicit message showing the expected value and the actual nested
value when HCO_SECURITY_KEY/HCO_CR_CERT_CONFIG_KEY do not match. Keep the
assertion logic the same, but make the failure output explain which spec field
was wrong to speed up reconciliation debugging.
In
`@tests/install_upgrade_operators/strict_reconciliation/test_hco_default_cpu_model.py`:
- Around line 21-24: The post-patch assertions in test_hco_default_cpu_model
should fail fast instead of hiding missing schema data, so replace the chained
.get(..., {}) lookup for spec.virtualization.virtualMachineOptions with direct
indexing through the HCO spec path in the test helper and in the later assertion
block. Use the existing symbols HCO_VIRTUALIZATION_KEY,
VIRTUAL_MACHINE_OPTIONS_KEY, and HCO_DEFAULT_CPU_MODEL_KEY to access the
required fields directly so any reconcile/schema break surfaces immediately and
precisely.
---
Outside diff comments:
In `@utilities/infra.py`:
- Around line 611-634: Both public HCO getter helpers need explicit typing and
documentation: add type hints to get_hyperconverged_resource and
get_hyperconverged_resource_v1beta1 (including parameter and return types), and
add short Google-style docstrings explaining that they resolve a HyperConverged
resource for the requested API version and raise ResourceNotFoundError when
missing. Keep the behavior in sync with the symbols get_hyperconverged_resource,
get_hyperconverged_resource_v1beta1, HyperConverged, and ResourceNotFoundError
so callers can clearly choose between the default v1 and compatibility v1beta1
variants.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a69238a0-7ada-4780-93c6-5d69f2ac7312
📒 Files selected for processing (57)
AGENTS.mdtests/conftest.pytests/infrastructure/golden_images/update_boot_source/conftest.pytests/infrastructure/vhostmd/test_downwardmetrics_virtio.pytests/infrastructure/vhostmd/test_vhostmd.pytests/infrastructure/vm_console_proxy/conftest.pytests/install_upgrade_operators/cert_renewal/conftest.pytests/install_upgrade_operators/conftest.pytests/install_upgrade_operators/constants.pytests/install_upgrade_operators/crypto_policy/test_crypto_policy_default.pytests/install_upgrade_operators/crypto_policy/test_hco_crypto_policy_propagation.pytests/install_upgrade_operators/crypto_policy/test_hco_custom_profile_negative.pytests/install_upgrade_operators/crypto_policy/utils.pytests/install_upgrade_operators/csv/test_hco_api_version.pytests/install_upgrade_operators/feature_gates/test_default_featuregates.pytests/install_upgrade_operators/feature_gates/test_update_featuregate_hco.pytests/install_upgrade_operators/hco_enablement_golden_image_updates/test_custom_golden_images_namespace.pytests/install_upgrade_operators/hco_enablement_golden_image_updates/test_enable_common_boot_image_import.pytests/install_upgrade_operators/launcher_updates/conftest.pytests/install_upgrade_operators/launcher_updates/constants.pytests/install_upgrade_operators/launcher_updates/test_default_launcher_updates.pytests/install_upgrade_operators/launcher_updates/test_negative_kubevirt_update.pytests/install_upgrade_operators/launcher_updates/test_reset_custom_values.pytests/install_upgrade_operators/launcher_updates/test_update_default_workload_strategy.pytests/install_upgrade_operators/launcher_updates/test_update_workload_strategy.pytests/install_upgrade_operators/must_gather/conftest.pytests/install_upgrade_operators/node_component/conftest.pytests/install_upgrade_operators/node_component/test_deploy_cnv_on_subset_of_nodes_sanity.pytests/install_upgrade_operators/node_component/test_hco_vm.pytests/install_upgrade_operators/node_component/test_node_placement_cr.pytests/install_upgrade_operators/product_uninstall/test_remove_hco.pytests/install_upgrade_operators/product_uninstall/test_remove_kubevirt.pytests/install_upgrade_operators/strict_reconciliation/conftest.pytests/install_upgrade_operators/strict_reconciliation/constants.pytests/install_upgrade_operators/strict_reconciliation/test_hco_cr_defaults_on_stanza_deletion.pytests/install_upgrade_operators/strict_reconciliation/test_hco_cr_modify_defaults.pytests/install_upgrade_operators/strict_reconciliation/test_hco_cr_nondefault_fields.pytests/install_upgrade_operators/strict_reconciliation/test_hco_default_cpu_model.pytests/network/kubemacpool/explicit_range/conftest.pytests/storage/cdi_config/test_cdi_config.pytests/storage/cross_cluster_live_migration/utils.pytests/storage/hpp/utils.pytests/storage/test_hotplug.pytests/storage/upgrade/conftest.pytests/utils.pytests/virt/cluster/aaq/conftest.pytests/virt/cluster/common_templates/custom_namespace/conftest.pytests/virt/node/gpu/gpu_pci_passthrough/conftest.pytests/virt/node/gpu/vgpu/conftest.pytests/virt/node/log_verbosity/conftest.pytests/virt/node/workload_density/test_free_page_reporting.pytests/virt/node/workload_density/test_kernel_samepage_merging.pytests/virt/upgrade/conftest.pytests/virt/utils.pyutilities/hco.pyutilities/infra.pyutilities/unittests/test_hco.py
Comment on lines
+42
to
+46
| patches={ | ||
| hyperconverged_resource_scope_class: { | ||
| "spec": {"security": {HCO_CR_CERT_CONFIG_KEY: target_certconfig_stanza}} | ||
| } | ||
| }, |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
LOW: Prefer shared HCO_SECURITY_KEY over hardcoded "security" in patch path.
Using the shared constant keeps this fixture aligned with central path contracts and reduces drift risk.
Suggested patch
- "spec": {"security": {HCO_CR_CERT_CONFIG_KEY: target_certconfig_stanza}}
+ "spec": {HCO_SECURITY_KEY: {HCO_CR_CERT_CONFIG_KEY: target_certconfig_stanza}}As per coding guidelines, "REUSE existing code and patterns — only write new when nothing exists."
📝 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.
Suggested change
| patches={ | |
| hyperconverged_resource_scope_class: { | |
| "spec": {"security": {HCO_CR_CERT_CONFIG_KEY: target_certconfig_stanza}} | |
| } | |
| }, | |
| patches={ | |
| hyperconverged_resource_scope_class: { | |
| "spec": {HCO_SECURITY_KEY: {HCO_CR_CERT_CONFIG_KEY: target_certconfig_stanza}} | |
| } | |
| }, |
🤖 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/install_upgrade_operators/cert_renewal/conftest.py` around lines 42 -
46, The patch path in the cert renewal fixture hardcodes the "security" key
instead of using the shared HCO_SECURITY_KEY constant. Update the fixture in
conftest.py where the patches dict is built to reference HCO_SECURITY_KEY in the
spec path, matching the existing contract used elsewhere and keeping the patch
aligned with shared patterns.
Source: Coding guidelines
Comment on lines
+13
to
+14
| expected_api_version = f"{HyperConverged.ApiGroup.HCO_KUBEVIRT_IO}/{Resource.ApiVersion.V1}" | ||
| assert hyperconverged_resource_scope_function.instance.apiVersion == expected_api_version |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
MEDIUM: Add a failure message to the apiVersion assertion.
When this fails in CI, the current assertion gives less context than the repo standard expects for tests/**. Please include the expected/actual contract in the message.
Suggested change
expected_api_version = f"{HyperConverged.ApiGroup.HCO_KUBEVIRT_IO}/{Resource.ApiVersion.V1}"
-assert hyperconverged_resource_scope_function.instance.apiVersion == expected_api_version
+assert (
+ hyperconverged_resource_scope_function.instance.apiVersion == expected_api_version
+), (
+ f"Expected HyperConverged apiVersion to be {expected_api_version}, "
+ f"got {hyperconverged_resource_scope_function.instance.apiVersion}"
+)As per coding guidelines, tests under tests/ must "Use pytest assertions with failure messages".
📝 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.
Suggested change
| expected_api_version = f"{HyperConverged.ApiGroup.HCO_KUBEVIRT_IO}/{Resource.ApiVersion.V1}" | |
| assert hyperconverged_resource_scope_function.instance.apiVersion == expected_api_version | |
| expected_api_version = f"{HyperConverged.ApiGroup.HCO_KUBEVIRT_IO}/{Resource.ApiVersion.V1}" | |
| assert ( | |
| hyperconverged_resource_scope_function.instance.apiVersion == expected_api_version | |
| ), ( | |
| f"Expected HyperConverged apiVersion to be {expected_api_version}, " | |
| f"got {hyperconverged_resource_scope_function.instance.apiVersion}" | |
| ) |
🤖 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/install_upgrade_operators/csv/test_hco_api_version.py` around lines 13
- 14, Add a failure message to the apiVersion assertion in the test that
compares hyperconverged_resource_scope_function.instance.apiVersion against
expected_api_version. Update the assertion so it clearly states the
expected/actual contract for this check, following the pytest-with-message
convention used under tests/**, and keep the change localized to the test case
involving HyperConverged.ApiGroup.HCO_KUBEVIRT_IO and Resource.ApiVersion.V1.
Source: Coding guidelines
Comment on lines
28
to
47
| pytest.param( | ||
| { | ||
| "patch": {"spec": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionInterval": None}}}, | ||
| "patch": { | ||
| "spec": {"virtualization": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionInterval": None}}} | ||
| }, | ||
| }, | ||
| MOD_CUST_DEFAULT_BATCH_EVICTION_INTERVAL, | ||
| marks=pytest.mark.polarion("CNV-6929"), | ||
| id="test_hyperconverged_reset_workload_update_strategy_batch_eviction_size", | ||
| ), | ||
| pytest.param( | ||
| { | ||
| "patch": {"spec": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionSize": None}}}, | ||
| "patch": { | ||
| "spec": {"virtualization": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionSize": None}}} | ||
| }, | ||
| }, | ||
| MOD_CUST_DEFAULT_BATCH_EVICTION_SIZE, | ||
| marks=pytest.mark.polarion("CNV-6930"), | ||
| id="test_hyperconverged_reset_workload_update_strategy_workload_update_methods", | ||
| ), |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
LOW: Fix the parametrized ids so they match the patched field.
Line 36 resets batchEvictionInterval, but the id says batch_eviction_size. Line 46 resets batchEvictionSize, but the id says workload_update_methods. Pytest uses these ids in node names and failure output, so the current labels make triage and -k targeting misleading.
Proposed fix
- id="test_hyperconverged_reset_workload_update_strategy_batch_eviction_size",
+ id="test_hyperconverged_reset_workload_update_strategy_batch_eviction_interval",
...
- id="test_hyperconverged_reset_workload_update_strategy_workload_update_methods",
+ id="test_hyperconverged_reset_workload_update_strategy_batch_eviction_size",📝 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.
Suggested change
| pytest.param( | |
| { | |
| "patch": {"spec": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionInterval": None}}}, | |
| "patch": { | |
| "spec": {"virtualization": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionInterval": None}}} | |
| }, | |
| }, | |
| MOD_CUST_DEFAULT_BATCH_EVICTION_INTERVAL, | |
| marks=pytest.mark.polarion("CNV-6929"), | |
| id="test_hyperconverged_reset_workload_update_strategy_batch_eviction_size", | |
| ), | |
| pytest.param( | |
| { | |
| "patch": {"spec": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionSize": None}}}, | |
| "patch": { | |
| "spec": {"virtualization": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionSize": None}}} | |
| }, | |
| }, | |
| MOD_CUST_DEFAULT_BATCH_EVICTION_SIZE, | |
| marks=pytest.mark.polarion("CNV-6930"), | |
| id="test_hyperconverged_reset_workload_update_strategy_workload_update_methods", | |
| ), | |
| pytest.param( | |
| { | |
| "patch": { | |
| "spec": {"virtualization": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionInterval": None}}} | |
| }, | |
| }, | |
| MOD_CUST_DEFAULT_BATCH_EVICTION_INTERVAL, | |
| marks=pytest.mark.polarion("CNV-6929"), | |
| id="test_hyperconverged_reset_workload_update_strategy_batch_eviction_interval", | |
| ), | |
| pytest.param( | |
| { | |
| "patch": { | |
| "spec": {"virtualization": {WORKLOAD_UPDATE_STRATEGY_KEY_NAME: {"batchEvictionSize": None}}} | |
| }, | |
| }, | |
| MOD_CUST_DEFAULT_BATCH_EVICTION_SIZE, | |
| marks=pytest.mark.polarion("CNV-6930"), | |
| id="test_hyperconverged_reset_workload_update_strategy_batch_eviction_size", | |
| ), |
🤖 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/install_upgrade_operators/launcher_updates/test_reset_custom_values.py`
around lines 28 - 47, The parametrized test ids in test_reset_custom_values.py
do not match the fields being patched in the pytest.param cases. Update the id
values in the relevant pytest.param entries so they clearly reflect the actual
reset targets in the WORKLOAD_UPDATE_STRATEGY_KEY_NAME patch, specifically
batchEvictionInterval and batchEvictionSize, to keep pytest node names and
failure output accurate.
Comment on lines
+124
to
+130
| existing_fg_names = {fg["name"] for fg in hco_fgs} | ||
| updated_fgs = list(hco_fgs) | ||
| for fg_name in new_fgs: | ||
| if fg_name not in existing_fg_names: | ||
| updated_fgs.append({"name": fg_name, "state": FG_STATE_ENABLED}) | ||
| with ResourceEditorValidateHCOReconcile( | ||
| patches={hyperconverged_resource_scope_function: {"spec": {"featureGates": hco_fgs}}}, | ||
| patches={hyperconverged_resource_scope_function: {"spec": {"featureGates": updated_fgs}}}, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
HIGH: Force requested feature gates to Enabled, don’t only append missing names.
If spec.featureGates already contains one of new_fgs with state: "Disabled", this loop skips it because the name is already in existing_fg_names, so the fixture never actually enables the requested gate. That makes the fixture state-dependent and can leave strict-reconciliation tests patching the wrong HCO spec.
Suggested fix
- existing_fg_names = {fg["name"] for fg in hco_fgs}
- updated_fgs = list(hco_fgs)
- for fg_name in new_fgs:
- if fg_name not in existing_fg_names:
- updated_fgs.append({"name": fg_name, "state": FG_STATE_ENABLED})
+ updated_fgs_by_name = {fg["name"]: dict(fg) for fg in hco_fgs}
+ for fg_name in new_fgs:
+ updated_fgs_by_name[fg_name] = {"name": fg_name, "state": FG_STATE_ENABLED}
+
+ updated_fgs = list(updated_fgs_by_name.values())
with ResourceEditorValidateHCOReconcile(
patches={hyperconverged_resource_scope_function: {"spec": {"featureGates": updated_fgs}}},📝 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.
Suggested change
| existing_fg_names = {fg["name"] for fg in hco_fgs} | |
| updated_fgs = list(hco_fgs) | |
| for fg_name in new_fgs: | |
| if fg_name not in existing_fg_names: | |
| updated_fgs.append({"name": fg_name, "state": FG_STATE_ENABLED}) | |
| with ResourceEditorValidateHCOReconcile( | |
| patches={hyperconverged_resource_scope_function: {"spec": {"featureGates": hco_fgs}}}, | |
| patches={hyperconverged_resource_scope_function: {"spec": {"featureGates": updated_fgs}}}, | |
| updated_fgs_by_name = {fg["name"]: dict(fg) for fg in hco_fgs} | |
| for fg_name in new_fgs: | |
| updated_fgs_by_name[fg_name] = {"name": fg_name, "state": FG_STATE_ENABLED} | |
| updated_fgs = list(updated_fgs_by_name.values()) | |
| with ResourceEditorValidateHCOReconcile( | |
| patches={hyperconverged_resource_scope_function: {"spec": {"featureGates": updated_fgs}}}, |
🧰 Tools
🪛 Ruff (0.15.18)
[warning] 128-128: Use list.extend to create a transformed list
(PERF401)
🤖 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/install_upgrade_operators/strict_reconciliation/conftest.py` around
lines 124 - 130, The feature-gate patching in the strict-reconciliation fixture
only appends missing names, so any requested gate already present in HCO
featureGates but set to Disabled is left unchanged. Update the logic around
existing_fg_names and updated_fgs so every fg_name in new_fgs is forced to state
FG_STATE_ENABLED, whether it already exists or not. Use the featureGates patch
construction in this fixture to replace the matching entry instead of skipping
it, so ResourceEditorValidateHCOReconcile always applies the requested enabled
state.
Comment on lines
+272
to
+273
| hco_spec = hyperconverged_resource_scope_function.instance.to_dict()["spec"] | ||
| assert hco_spec.get(HCO_SECURITY_KEY, {}).get(HCO_CR_CERT_CONFIG_KEY) == expected |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
MEDIUM: Add explicit assertion failure messages on nested spec checks.
These assertions currently fail without contextual diagnostics, which slows reconciliation triage.
Suggested patch
- assert hco_spec.get(HCO_SECURITY_KEY, {}).get(HCO_CR_CERT_CONFIG_KEY) == expected
+ assert hco_spec.get(HCO_SECURITY_KEY, {}).get(HCO_CR_CERT_CONFIG_KEY) == expected, (
+ f"Expected hco.spec.{HCO_SECURITY_KEY}.{HCO_CR_CERT_CONFIG_KEY}={expected}, "
+ f"actual={hco_spec.get(HCO_SECURITY_KEY, {}).get(HCO_CR_CERT_CONFIG_KEY)}"
+ )
@@
- assert hco_spec.get(HCO_VIRTUALIZATION_KEY, {}).get(LIVE_MIGRATION_CONFIG_KEY) == expected
+ assert hco_spec.get(HCO_VIRTUALIZATION_KEY, {}).get(LIVE_MIGRATION_CONFIG_KEY) == expected, (
+ f"Expected hco.spec.{HCO_VIRTUALIZATION_KEY}.{LIVE_MIGRATION_CONFIG_KEY}={expected}, "
+ f"actual={hco_spec.get(HCO_VIRTUALIZATION_KEY, {}).get(LIVE_MIGRATION_CONFIG_KEY)}"
+ )As per coding guidelines, "Use pytest assertions with failure messages." Based on learnings, integration tests under tests/ should include descriptive assertion messages.
Also applies to: 493-494
🤖 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/install_upgrade_operators/strict_reconciliation/test_hco_cr_defaults_on_stanza_deletion.py`
around lines 272 - 273, Add descriptive pytest failure messages to the nested
spec assertions in the strict reconciliation test, especially the check in
test_hco_cr_defaults_on_stanza_deletion and the other referenced spec
comparisons. Update the assertions that read from
hyperconverged_resource_scope_function.instance.to_dict()["spec"] so they
include an explicit message showing the expected value and the actual nested
value when HCO_SECURITY_KEY/HCO_CR_CERT_CONFIG_KEY do not match. Keep the
assertion logic the same, but make the failure output explain which spec field
was wrong to speed up reconciliation debugging.
Sources: Coding guidelines, Learnings
Comment on lines
+21
to
+24
| hco_spec = hco_resource.instance.to_dict()["spec"] | ||
| hco_cpu_model = ( | ||
| hco_spec.get(HCO_VIRTUALIZATION_KEY, {}).get(VIRTUAL_MACHINE_OPTIONS_KEY, {}).get(HCO_DEFAULT_CPU_MODEL_KEY) | ||
| ) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
MEDIUM: Prefer direct indexing for the required HCO path.
Line 21-24 and Line 38-40 are assertion helpers for the post-patch state, so spec.virtualization.virtualMachineOptions should exist here. Chained .get(..., {}) hides a broken reconcile/schema change by collapsing it to None, which makes the test fail later with a less precise signal.
Suggested change
- hco_cpu_model = (
- hco_spec.get(HCO_VIRTUALIZATION_KEY, {}).get(VIRTUAL_MACHINE_OPTIONS_KEY, {}).get(HCO_DEFAULT_CPU_MODEL_KEY)
- )
+ hco_cpu_model = hco_spec[HCO_VIRTUALIZATION_KEY][VIRTUAL_MACHINE_OPTIONS_KEY][HCO_DEFAULT_CPU_MODEL_KEY]As per coding guidelines, "Do not use defensive programming unless it falls under an allowed exception; prefer fail-fast behavior and trust schema-guaranteed data."
Also applies to: 38-40
🤖 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/install_upgrade_operators/strict_reconciliation/test_hco_default_cpu_model.py`
around lines 21 - 24, The post-patch assertions in test_hco_default_cpu_model
should fail fast instead of hiding missing schema data, so replace the chained
.get(..., {}) lookup for spec.virtualization.virtualMachineOptions with direct
indexing through the HCO spec path in the test helper and in the later assertion
block. Use the existing symbols HCO_VIRTUALIZATION_KEY,
VIRTUAL_MACHINE_OPTIONS_KEY, and HCO_DEFAULT_CPU_MODEL_KEY to access the
required fields directly so any reconcile/schema break surfaces immediately and
precisely.
Source: Coding guidelines
geetikakay
reviewed
Jun 25, 2026
Comment on lines
+77
to
+79
| hyperconverged_resource_scope_function: { | ||
| "spec": {"workloadSources": {"dataImportCronTemplates": [data_import_cron_dict]}} | ||
| } |
Contributor
There was a problem hiding this comment.
use constants + hyperconverged_resource_scope_function: {
-
"spec": {HCO_SPEC_WORKLOAD_SOURCES: {SSP_CR_COMMON_TEMPLATES_LIST_KEY_NAME: [data_import_cron_dict]}}
Comment on lines
+169
to
+171
| "workloadSources": { | ||
| "dataImportCronTemplates": [updated_hco_with_custom_data_import_cron_scope_function] | ||
| } |
| hyperconverged_resource_scope_class: { | ||
| "spec": { | ||
| "deployVmConsoleProxy": True, | ||
| "deployment": {"deployVmConsoleProxy": True}, |
Contributor
There was a problem hiding this comment.
-
HCO_SPEC_DEPLOYMENT: { -
HCO_DEPLOY_VM_CONSOLE_PROXY: True, -
},
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it:
HCO CRD storage version changes to v1 in the 4.23/5.0 release.
All existing automation must be updated to use the v1 apiVersion so all teams align with the new storage version and avoid continuing to generate or maintain automation against the older API version.
Which issue(s) this PR fixes:
Existing automation must use HCO v1 api.
Special notes for reviewer:
Modified AGENTS.md to force it for new incoming automation,
jira-ticket:
https://redhat.atlassian.net/browse/CNV-87694
Summary by CodeRabbit
New Features
Bug Fixes