chore: bump pre-commit hooks and apply lint cleanup#4701
chore: bump pre-commit hooks and apply lint cleanup#4701rnetser wants to merge 2 commits intoRedHatQE:mainfrom
Conversation
📝 WalkthroughWalkthroughModernized Python typings/imports and lint configs across the repository (PEP 604 unions, built-in generics, move ChangesType & Lint Modernization
Network Sanity & Fixtures
Behavioral & Quality Adjustments (scattered)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
github-actions
Bot
added
sig-network
sig-iuo
sig-chaos
sig-infra
sig-storage
sig-virt
labels
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4701 +/- ##
==========================================
+ Coverage 98.63% 98.67% +0.03%
==========================================
Files 25 25
Lines 2420 2483 +63
==========================================
+ Hits 2387 2450 +63
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
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
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
qwang1 can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
openshift-virtualization-qe-bot
requested review from
SamAlber,
albarker-rh,
azhivovk,
ema-aka-young,
jpeimer and
kgoldbla
and removed request for
ema-aka-young
|
qwang1 can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
|
qwang1 can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
1 similar comment
|
qwang1 can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
rnetser
force-pushed
the
chore/pre-commit-lint-cleanup
branch
from
367e096 to
30faf1d
Compare
|
qwang1 can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
1 similar comment
|
qwang1 can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
rnetser
force-pushed
the
chore/pre-commit-lint-cleanup
branch
from
5dffd2b to
3bc0616
Compare
|
qwang1 can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4584. Overlapping filestests/global_config_multiarch.py |
Bump pre-commit hooks: - ruff v0.15.1 → v0.15.12 - mypy v1.20.0 → v1.20.2 Add ignore rules for ruff preview rules needing separate fixes (DTZ, BLE001, PYI, SIM102/114/117, PLW, F821, etc) and flake8 rules now handled by ruff (N815, N802, E501, F821, E201, W503, N818). Apply safe fixes repo-wide: - Remove obsolete utf-8 coding headers (UP009) - Remove stale noqa comments (including F821 in global_config files) - Simplify dict patterns (SIM118, PLC0206, PERF102) - Simplify comparisons (SIM201) - Remove unnecessary wrappers (C403, C408, C414) - Use f-strings (UP031) - Extract default arg function call (B008) - Fix root logger usage (LOG015) - Ruff formatting Assisted-by: Claude <noreply@anthropic.com> Signed-off-by: rnetser <rnetser@redhat.com>
for more information, see https://pre-commit.ci
|
@coderabbitai[bot] The following review comments were reviewed and a decision was made:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Tip For best results, initiate chat on the files or code changes.
✅ Resolved (Addressed):
🔁 Deferred (Pre-existing, out of scope):
[resolve] ✏️ Learnings added
|
|
qwang1 can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
1 similar comment
|
qwang1 can not be added as reviewer. Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.: 422 {"message": "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the RedHatQE/openshift-virtualization-tests repository.", "documentation_url": "https://docs.github.com/rest/pulls/review-requests#request-reviewers-for-a-pull-request", "status": "422"} |
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 (8)
tests/global_config_s390x.py (1)
28-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Initialize
configbefore first read to prevent undefined-name failure
if not config:on Line 28 readsconfigbefore it is defined in-module, which triggers Ruff F821 and can raiseNameErrordepending on import context. This breaks lint stability for this cleanup PR.Proposed fix
for _dir in dir(): - if not config: - config: dict[str, Any] = {} +config: dict[str, Any] = globals().get("config", {}) + +for _dir in dir(): val = locals()[_dir] if type(val) not in [bool, list, dict, str]: continue @@ - config[_dir] = locals()[_dir] + config[_dir] = valAs per coding guidelines "
**/*.py: NEVER add# noqa... If a linter complains, fix the code instead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/global_config_s390x.py` around lines 28 - 37, The code reads the variable config before it's defined causing a NameError/linter F821; initialize config at the top of this block and remove the pre-read check: ensure config is declared as an empty dict (e.g., config: dict[str, Any] = {}) before any use, then replace the `if not config:` guard with logic that uses the existing config (or a simple existence check like `if config is None:` only if you intend None as a sentinel), and keep the rest of the loop that inspects locals()[_dir], the type check, the skip for `encoding`/`py_file`, and the assignment `config[_dir] = locals()[_dir]` intact so subsequent uses of config are safe.tests/global_config.py (1)
252-262:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Fix undefined
configaccess at module loadLine 252 references
configbefore guaranteed initialization, which triggers F821 and can break when no injected global exists.Proposed fix
+config: dict[str, Any] = {} for _dir in dir(): - if not config: - config: dict[str, Any] = {} val = locals()[_dir] if type(val) not in [bool, list, dict, str, int, set]: continue @@ if _dir in ["encoding", "py_file"]: continue config[_dir] = locals()[_dir]As per coding guidelines: "
**/*.py: NEVER add# noqa... If a linter complains, fix the code instead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/global_config.py` around lines 252 - 262, The code reads the name `config` before ensuring it exists (the `if not config:` check can raise NameError/F821); initialize or reference it safely before use: check presence via `"config" in locals()` or use `locals().get("config")` and only assign `config = {}` if missing/falsey, then proceed to use `config` when populating entries for `_dir`; update the block around `_dir`/locals() to use a safe lookup (e.g., use locals().get("_dir") and locals().get("config")) and ensure `config` is defined as a dict prior to adding keys so symbols like `config`, `_dir`, and the locals() access are robust at module load.tests/global_config_ibm_spectrum_sc.py (1)
27-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH:
configis read before guaranteed initializationLine 27 hits an undefined-name path (
if not config) before a reliable assignment. This is lint-visible (F821) and environment-sensitive.Proposed fix
+config: dict[str, Any] = {} for _dir in dir(): - if not config: - config: dict[str, Any] = {} val = locals()[_dir] if type(val) not in [bool, list, dict, str]: continue @@ if _dir in ["encoding", "py_file"]: continue config[_dir] = locals()[_dir]As per coding guidelines: "
**/*.py: NEVER add# noqa... If a linter complains, fix the code instead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/global_config_ibm_spectrum_sc.py` around lines 27 - 36, The code reads variable config before it's guaranteed to exist, causing a potential NameError; initialize config before the "if not config" check (e.g., ensure config = {} is assigned earlier) or replace the check with a safe existence test like "'config' in locals() and config" so the subsequent use of config and assignments to config[_dir] are always valid; update the block that references config, locals()[_dir], and _dir (the surrounding loop/section) to ensure config is defined prior to any reads and then proceed with the existing type checks and assignments.tests/global_config_rh_it.py (1)
30-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Initialize
configexplicitly before first readLine 30 reads
configbefore guaranteed initialization, which is why Ruff reports F821 and why this pattern is brittle outside injected contexts.Proposed fix
+config: dict[str, Any] = {} for _dir in dir(): - if not config: - config: dict[str, Any] = {} val = locals()[_dir] if type(val) not in [bool, list, dict, str]: continue @@ if _dir in ["encoding", "py_file"]: continue config[_dir] = locals()[_dir]As per coding guidelines: "
**/*.py: NEVER add# noqa... If a linter complains, fix the code instead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/global_config_rh_it.py` around lines 30 - 39, The code reads the variable config before it is guaranteed to exist, causing F821; fix by ensuring config is initialized before any reads/conditionals that reference it (e.g., assign a default dict to config if it's not already defined in locals() or use config = globals().get('config', {}) before the existing if-not-config check). Update references around the config variable and the loop using _dir and locals() so that config is always a dict before you do "if not config" or "config[_dir] = ..."; ensure you remove the inline type-only assignment pattern and instead create a real initialized dict for config.tests/global_config_interop.py (1)
10-19:⚠️ Potential issue | 🟠 MajorHIGH:
configis read before initialization (line 10), causing NameError.The
global configdeclaration doesn't create the variable—it only marks it as global. Sinceconfigis never assigned before the loop starts, line 10'sif not config:will raise aNameErrorat runtime. Initializeconfigonce before the loop:Proposed fix
global config global_config = pytest_testconfig.load_python(py_file="tests/global_config.py", encoding="utf-8") +config: dict[str, Any] = {} for _dir in dir(): - if not config: - config: dict[str, Any] = {} val = locals()[_dir]Per coding guidelines: NEVER add
# noqaor linter suppressions—fix the code instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/global_config_interop.py` around lines 10 - 19, The code reads config before it's initialized causing a NameError; initialize config once before the loop (ensure a top-level assignment like config = {} before any use) and then remove the conditional `if not config:` inside the loop, keeping the rest of the logic that checks `val = locals()[_dir]`, the type filter `if type(val) not in [bool, list, dict, str]: continue`, the exclusions `if _dir in ["encoding", "py_file"]: continue`, and the assignment `config[_dir] = locals()[_dir]` so that `config` is always defined when referenced.tests/global_config_multiarch.py (1)
57-66:⚠️ Potential issue | 🟠 MajorHIGH:
configis accessed at line 57 before initialization, causingNameErrorat runtime.The global variable
configis declared at line 17 but never initialized. The conditional checkif not config:at line 57 attempts to access an undefined name on the first loop iteration. Move the initialization before the loop to ensureconfigexists when the condition is evaluated.Proposed fix
global config storage_class_matrix = [ ... ] storage_class_a = StorageClassNames.IO2_CSI storage_class_b = StorageClassNames.IO2_CSI os_matrix = { ... } +config: dict[str, Any] = globals().get("config", {}) + for _dir in dir(): - if not config: - config: dict[str, Any] = {} val = locals()[_dir] if type(val) not in [bool, list, dict, str]: continue if _dir in ["encoding", "py_file"]: continue config[_dir] = locals()[_dir]This eliminates the undefined-name hazard per the guideline: Never use linter suppressions—fix the code directly. The initialization uses
globals().get()to safely default to an empty dict while respecting any external configuration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/global_config_multiarch.py` around lines 57 - 66, The code accesses config before it’s guaranteed to exist; initialize or fetch it from globals before the loop so the check `if not config:` cannot raise NameError—e.g., ensure `config = globals().get("config", {})` is executed prior to using `config`, then keep the rest of the loop that reads `val = locals()[_dir]`, checks `type(val)`, skips `["encoding", "py_file"]`, and assigns `config[_dir] = locals()[_dir]`; this preserves external `config` if present and avoids the undefined-name error for the `config` symbol.tests/global_config_arm64.py (1)
57-66:⚠️ Potential issue | 🔴 CriticalHIGH:
configis not defined and will causeNameErroron line 57.The
global configstatement at line 17 is only a scope declaration—it does not initialize the variable. The loop at line 56 immediately accessesconfigon line 57 without prior assignment, which will fail on the first iteration because the name does not exist in the module scope.Initialize
configbefore the loop starts:Proposed fix
global config Images.Cirros.RAW_IMG_XZ = "cirros-0.4.0-aarch64-disk.raw.xz" @@ -54,8 +54,7 @@ instance_type_fedora_os_list = [OS_FLAVOR_FEDORA] instance_type_rhel_os_list = [RHEL10_PREFERENCE] instance_type_fedora_os_list = [OS_FLAVOR_FEDORA] +config: dict[str, Any] = {} for _dir in dir(): - if not config: - config: dict[str, Any] = {} val = locals()[_dir]Per coding guidelines: NEVER suppress linter warnings with
# noqaor# type: ignore—fix the code instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/global_config_arm64.py` around lines 57 - 66, The code references the module variable config inside the loop (check "if not config:" and assignment to config[_dir]) but config is never initialized, causing a NameError; before the loop that iterates over _dir and uses locals()[_dir], initialize config to an empty dict (e.g., set config: dict[str, Any] = {} in the same scope where global config is declared) or replace the conditional with a direct assignment so that config exists before any access; update the symbol "config" accordingly and keep the rest of the loop logic (checks on locals()[_dir], skipping "encoding" and "py_file", and assigning config[_dir] = locals()[_dir]) unchanged.tests/observability/metrics/test_vms_metrics.py (1)
47-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMEDIUM: Fail fast when READY condition is missing
get_last_transition_timehas an implicitNonepath. That turns a missing READY condition into a later timeout, which obscures the real failure source. Make the fallback explicit.Suggested fix
-def get_last_transition_time(vm): +def get_last_transition_time(vm) -> int: for condition in vm.instance.get("status", {}).get("conditions"): if condition.get("type") == vm.Condition.READY: last_transition_time = condition.get("lastTransitionTime") return int((datetime.strptime(last_transition_time, "%Y-%m-%dT%H:%M:%SZ").replace(tzinfo=UTC)).timestamp()) + raise AssertionError(f"READY condition not found for VM {vm.name}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/observability/metrics/test_vms_metrics.py` around lines 47 - 51, get_last_transition_time silently returns None when no READY condition is present; change it to fail fast by explicitly checking the conditions for vm.Condition.READY and raising a clear exception (e.g., ValueError or RuntimeError) if the READY condition is missing. Locate the function get_last_transition_time and the loop that inspects vm.instance.get("status", {}).get("conditions"), keep the existing parsing and timestamp conversion for the READY condition, but after the loop raise an error (including identifying info from vm such as an id or name if available) so callers immediately see the missing READY condition.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.flake8:
- Around line 6-13: Remove F821 from the global ignore list in the ignore block
and instead add a per-file-ignores section that scopes F821 to only the legacy
files that genuinely need undefined-name suppression; specifically, edit the
ignore block to drop "F821" and add a "per-file-ignores =" mapping (e.g.,
"legacy_module.py: F821" or "path/to/legacy/*.py: F821") so undefined-name
checks run everywhere else while preserving F821 only for identified legacy
files.
In `@pyproject.toml`:
- Around line 34-37: Update the per-file-ignores entry so only F821 is scoped to
the dynamic-loading files (e.g., tests/global_config*.py) and remove PLE0704
from that per-file list; specifically, add or change the per-file-ignores rule
to map tests/global_config*.py -> F821 (keeping PLE0704 in the global
ignore/violation list), and ensure any references to
pytest_testconfig.load_python() and use of locals() remain uncovered by the
PLE0704 per-file exception so bare-raise violations elsewhere (in test
utilities) remain detected.
In `@utilities/virt.py`:
- Line 2817: Replace the ambiguous boolean chain flagged by RUF021 by making the
intent explicit: change the condition that uses migration_network and
migration_network_on_pod (currently written as "migration_network and
migration_network_on_pod or not migration_network and not
migration_network_on_pod") to an equality comparison using migration_network ==
migration_network_on_pod so the precedence is unambiguous and ruff no longer
complains; alternatively, if you prefer the original dual-branch logic, add
parentheses around each conjunction (e.g., (migration_network and
migration_network_on_pod) or (not migration_network and not
migration_network_on_pod)) to satisfy RUF021.
---
Outside diff comments:
In `@tests/global_config_arm64.py`:
- Around line 57-66: The code references the module variable config inside the
loop (check "if not config:" and assignment to config[_dir]) but config is never
initialized, causing a NameError; before the loop that iterates over _dir and
uses locals()[_dir], initialize config to an empty dict (e.g., set config:
dict[str, Any] = {} in the same scope where global config is declared) or
replace the conditional with a direct assignment so that config exists before
any access; update the symbol "config" accordingly and keep the rest of the loop
logic (checks on locals()[_dir], skipping "encoding" and "py_file", and
assigning config[_dir] = locals()[_dir]) unchanged.
In `@tests/global_config_ibm_spectrum_sc.py`:
- Around line 27-36: The code reads variable config before it's guaranteed to
exist, causing a potential NameError; initialize config before the "if not
config" check (e.g., ensure config = {} is assigned earlier) or replace the
check with a safe existence test like "'config' in locals() and config" so the
subsequent use of config and assignments to config[_dir] are always valid;
update the block that references config, locals()[_dir], and _dir (the
surrounding loop/section) to ensure config is defined prior to any reads and
then proceed with the existing type checks and assignments.
In `@tests/global_config_interop.py`:
- Around line 10-19: The code reads config before it's initialized causing a
NameError; initialize config once before the loop (ensure a top-level assignment
like config = {} before any use) and then remove the conditional `if not
config:` inside the loop, keeping the rest of the logic that checks `val =
locals()[_dir]`, the type filter `if type(val) not in [bool, list, dict, str]:
continue`, the exclusions `if _dir in ["encoding", "py_file"]: continue`, and
the assignment `config[_dir] = locals()[_dir]` so that `config` is always
defined when referenced.
In `@tests/global_config_multiarch.py`:
- Around line 57-66: The code accesses config before it’s guaranteed to exist;
initialize or fetch it from globals before the loop so the check `if not
config:` cannot raise NameError—e.g., ensure `config = globals().get("config",
{})` is executed prior to using `config`, then keep the rest of the loop that
reads `val = locals()[_dir]`, checks `type(val)`, skips `["encoding",
"py_file"]`, and assigns `config[_dir] = locals()[_dir]`; this preserves
external `config` if present and avoids the undefined-name error for the
`config` symbol.
In `@tests/global_config_rh_it.py`:
- Around line 30-39: The code reads the variable config before it is guaranteed
to exist, causing F821; fix by ensuring config is initialized before any
reads/conditionals that reference it (e.g., assign a default dict to config if
it's not already defined in locals() or use config = globals().get('config', {})
before the existing if-not-config check). Update references around the config
variable and the loop using _dir and locals() so that config is always a dict
before you do "if not config" or "config[_dir] = ..."; ensure you remove the
inline type-only assignment pattern and instead create a real initialized dict
for config.
In `@tests/global_config_s390x.py`:
- Around line 28-37: The code reads the variable config before it's defined
causing a NameError/linter F821; initialize config at the top of this block and
remove the pre-read check: ensure config is declared as an empty dict (e.g.,
config: dict[str, Any] = {}) before any use, then replace the `if not config:`
guard with logic that uses the existing config (or a simple existence check like
`if config is None:` only if you intend None as a sentinel), and keep the rest
of the loop that inspects locals()[_dir], the type check, the skip for
`encoding`/`py_file`, and the assignment `config[_dir] = locals()[_dir]` intact
so subsequent uses of config are safe.
In `@tests/global_config.py`:
- Around line 252-262: The code reads the name `config` before ensuring it
exists (the `if not config:` check can raise NameError/F821); initialize or
reference it safely before use: check presence via `"config" in locals()` or use
`locals().get("config")` and only assign `config = {}` if missing/falsey, then
proceed to use `config` when populating entries for `_dir`; update the block
around `_dir`/locals() to use a safe lookup (e.g., use locals().get("_dir") and
locals().get("config")) and ensure `config` is defined as a dict prior to adding
keys so symbols like `config`, `_dir`, and the locals() access are robust at
module load.
In `@tests/observability/metrics/test_vms_metrics.py`:
- Around line 47-51: get_last_transition_time silently returns None when no
READY condition is present; change it to fail fast by explicitly checking the
conditions for vm.Condition.READY and raising a clear exception (e.g.,
ValueError or RuntimeError) if the READY condition is missing. Locate the
function get_last_transition_time and the loop that inspects
vm.instance.get("status", {}).get("conditions"), keep the existing parsing and
timestamp conversion for the READY condition, but after the loop raise an error
(including identifying info from vm such as an id or name if available) so
callers immediately see the missing READY condition.
🪄 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: 52882623-8261-4408-b6d7-4e7feb2b372d
⛔ Files ignored due to path filters (3)
AGENTS.mdis excluded by!**/*.mddocs/QUARANTINE_GUIDELINES.mdis excluded by!**/*.mddocs/SOFTWARE_TEST_DESCRIPTION.mdis excluded by!**/*.md
📒 Files selected for processing (127)
.flake8.pre-commit-config.yamlconftest.pylibs/net/netattachdef.pylibs/net/traffic_generator.pylibs/storage/config.pylibs/vm/spec.pypyproject.tomlscripts/quarantine_stats/generate_dashboard.pyscripts/tests_analyzer/pytest_marker_analyzer.pytests/chaos/oadp/conftest.pytests/conftest.pytests/global_config.pytests/global_config_amd64.pytests/global_config_arm64.pytests/global_config_aro.pytests/global_config_aws.pytests/global_config_ibm_spectrum_sc.pytests/global_config_interop.pytests/global_config_interop_sno.pytests/global_config_lvms.pytests/global_config_multiarch.pytests/global_config_nfs.pytests/global_config_oci.pytests/global_config_rh_it.pytests/global_config_s390x.pytests/global_config_sno.pytests/global_config_sno_hpp.pytests/infrastructure/golden_images/update_boot_source/test_ssp_data_sources.pytests/infrastructure/golden_images/update_boot_source/utils.pytests/infrastructure/instance_types/test_common_vm_preference.pytests/infrastructure/vm_console_proxy/utils.pytests/infrastructure/workload_availability/remediation_fencing_mhc/test_ha_vm.pytests/install_upgrade_operators/crypto_policy/test_hco_override_api_server_crypto_policy.pytests/install_upgrade_operators/crypto_policy/test_update_specific_component_crypto_policy.pytests/install_upgrade_operators/csv/csv_permissions_audit/test_csv_permissions_audit.pytests/install_upgrade_operators/json_patch/test_json_patch_annotation_cdi.pytests/install_upgrade_operators/json_patch/test_json_patch_annotation_cnao.pytests/install_upgrade_operators/json_patch/test_json_patch_annotation_kubevirt.pytests/install_upgrade_operators/json_patch/test_json_patch_annotation_multiple_updates.pytests/install_upgrade_operators/json_patch/test_json_patch_annotation_ssp.pytests/install_upgrade_operators/must_gather/test_must_gather.pytests/install_upgrade_operators/must_gather/utils.pytests/install_upgrade_operators/node_component/utils.pytests/install_upgrade_operators/resource_params/utils.pytests/install_upgrade_operators/security/scc/test_cnv_deployment_required_scc.pytests/install_upgrade_operators/security/scc/test_cnv_pods_scc.pytests/install_upgrade_operators/security/scc/test_default_scc.pytests/install_upgrade_operators/security/selinux/test_selinux_launcher_type.pytests/network/conftest.pytests/network/general/test_bridge_marker.pytests/network/kubemacpool/explicit_range/conftest.pytests/network/kubemacpool/utils.pytests/network/l2_bridge/libl2bridge.pytests/network/libs/cluster_user_defined_network.pytests/network/libs/label_selector.pytests/network/localnet/liblocalnet.pytests/network/migration/test_migration.pytests/network/nmstate/conftest.pytests/network/nmstate/test_connectivity_after_nmstate_changes.pytests/network/provider_migration/libprovider.pytests/network/sriov/libsriov.pytests/network/user_defined_network/ip_specification/conftest.pytests/network/user_defined_network/test_user_defined_network_passt.pytests/observability/metrics/test_vms_metrics.pytests/observability/metrics/utils.pytests/scale/test_scale_benchmark.pytests/storage/cdi_config/conftest.pytests/storage/cdi_upload/test_upload.pytests/storage/cdi_upload/test_upload_virtctl.pytests/storage/cdi_upload/utils.pytests/storage/conftest.pytests/storage/cross_cluster_live_migration/utils.pytests/storage/golden_image/test_cached_snapshots.pytests/storage/hpp/test_hostpath.pytests/storage/hpp/test_hpp_node_placement.pytests/storage/online_resize/conftest.pytests/storage/online_resize/test_online_resize.pytests/storage/online_resize/utils.pytests/storage/snapshots/conftest.pytests/storage/snapshots/test_snapshots.pytests/storage/storage_migration/utils.pytests/storage/test_cdi_certificate.pytests/storage/test_cdi_resources.pytests/storage/test_data_protection.pytests/storage/test_disk_preallocation.pytests/storage/test_local_storage.pytests/storage/test_wffc.pytests/storage/utils.pytests/storage/vm_export/utils.pytests/utils.pytests/virt/cluster/common_templates/centos/test_centos_os_support.pytests/virt/cluster/common_templates/custom_namespace/utils.pytests/virt/cluster/common_templates/fedora/test_fedora_os_support.pytests/virt/cluster/common_templates/general/test_base_template.pytests/virt/cluster/common_templates/general/test_template_validator.pytests/virt/cluster/common_templates/rhel/test_rhel_os_support.pytests/virt/cluster/common_templates/utils.pytests/virt/cluster/general/mass_machine_type_transition_tests/test_mass_machine_type_transition.pytests/virt/cluster/longevity_tests/utils.pytests/virt/cluster/migration_and_maintenance/test_evictionstrategy.pytests/virt/node/descheduler/conftest.pytests/virt/node/general/test_windows_vtpm_bitlocker.pytests/virt/node/gpu/vgpu/test_rhel_vm_with_vgpu.pytests/virt/node/high_performance_vm/test_numa.pytests/virt/node/hyperv_support/test_hyperv_features_in_vm.pytests/virt/upgrade/utils.pytests/virt/utils.pyutilities/console.pyutilities/constants.pyutilities/infra.pyutilities/logger.pyutilities/network.pyutilities/oadp.pyutilities/operator.pyutilities/sanity.pyutilities/storage.pyutilities/unittests/test_data_collector.pyutilities/unittests/test_database.pyutilities/unittests/test_guest_support.pyutilities/unittests/test_hco.pyutilities/unittests/test_oadp.pyutilities/unittests/test_operator.pyutilities/unittests/test_pytest_matrix_utils.pyutilities/unittests/test_pytest_utils.pyutilities/unittests/test_ssp.pyutilities/virt.py
💤 Files with no reviewable changes (27)
- tests/install_upgrade_operators/security/scc/test_cnv_deployment_required_scc.py
- tests/network/nmstate/conftest.py
- tests/storage/online_resize/conftest.py
- tests/storage/hpp/test_hpp_node_placement.py
- tests/storage/online_resize/utils.py
- tests/storage/cdi_upload/test_upload.py
- tests/network/general/test_bridge_marker.py
- tests/storage/snapshots/conftest.py
- tests/storage/test_local_storage.py
- tests/storage/conftest.py
- tests/network/migration/test_migration.py
- tests/infrastructure/workload_availability/remediation_fencing_mhc/test_ha_vm.py
- tests/storage/test_wffc.py
- tests/install_upgrade_operators/security/scc/test_cnv_pods_scc.py
- tests/storage/snapshots/test_snapshots.py
- tests/install_upgrade_operators/must_gather/test_must_gather.py
- tests/virt/cluster/common_templates/general/test_template_validator.py
- tests/install_upgrade_operators/security/selinux/test_selinux_launcher_type.py
- tests/storage/test_data_protection.py
- tests/install_upgrade_operators/security/scc/test_default_scc.py
- tests/storage/cdi_config/conftest.py
- utilities/unittests/test_oadp.py
- tests/storage/online_resize/test_online_resize.py
- tests/storage/hpp/test_hostpath.py
- tests/storage/test_disk_preallocation.py
- tests/chaos/oadp/conftest.py
- tests/network/conftest.py
| ignore = | ||
| N815, | ||
| N802, | ||
| E501, | ||
| F821, | ||
| E201, | ||
| W503, | ||
| N818, |
There was a problem hiding this comment.
HIGH: Avoid globally disabling F821
Adding F821 to global ignore suppresses undefined-name detection repo-wide, which can mask real defects. Limit this to explicit legacy files via per-file-ignores instead of blanket ignore.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.flake8 around lines 6 - 13, Remove F821 from the global ignore list in the
ignore block and instead add a per-file-ignores section that scopes F821 to only
the legacy files that genuinely need undefined-name suppression; specifically,
edit the ignore block to drop "F821" and add a "per-file-ignores =" mapping
(e.g., "legacy_module.py: F821" or "path/to/legacy/*.py: F821") so
undefined-name checks run everywhere else while preserving F821 only for
identified legacy files.
| "F821", # undefined name — used in global_config files (dynamic loading) | ||
| "PIE810", # call startswith/endswith once with tuple | ||
| "PLE0704", # bare raise not inside exception handler | ||
| "SIM103", # return condition directly |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify which files currently trigger these rules before narrowing scope.
# Expected: findings are limited to intended legacy global_config files.
uv run ruff check --select F821,PLE0704 --output-format grouped . || trueRepository: RedHatQE/openshift-virtualization-tests
Length of output: 124
🏁 Script executed:
#!/bin/bash
# Check if ruff is available and try to run it directly
which ruff && ruff check --select F821,PLE0704 --output-format grouped . 2>/dev/null || echo "ruff check failed, trying alternative approach"Repository: RedHatQE/openshift-virtualization-tests
Length of output: 192
🏁 Script executed:
#!/bin/bash
# Explore the project structure to understand the codebase layout
find . -type f -name "*.py" | grep -E "(global_config|test)" | head -20Repository: RedHatQE/openshift-virtualization-tests
Length of output: 905
🏁 Script executed:
#!/bin/bash
# Check the current pyproject.toml to see the exact ignore configuration
cat -n pyproject.toml | head -50Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2716
🏁 Script executed:
#!/bin/bash
# Search for files with "global_config" in their name
find . -type f -name "*global_config*" -o -name "*global*config*"Repository: RedHatQE/openshift-virtualization-tests
Length of output: 645
🏁 Script executed:
#!/bin/bash
# Search for bare raise statements in the codebase to understand PLE0704 usage
rg "^\s*raise\s*$" --type py -l 2>/dev/null | head -20Repository: RedHatQE/openshift-virtualization-tests
Length of output: 1009
🏁 Script executed:
#!/bin/bash
# Search for undefined name patterns (F821 typically appears in files with dynamic loading)
# Look for eval, exec, locals(), globals() patterns
rg "(eval|exec|locals\(\)|globals\(\)|__getattr__|__import__|importlib.import)" --type py -l 2>/dev/null | head -20Repository: RedHatQE/openshift-virtualization-tests
Length of output: 627
🏁 Script executed:
#!/bin/bash
# Examine one of the global_config files to understand the F821 violations
head -50 ./tests/global_config.pyRepository: RedHatQE/openshift-virtualization-tests
Length of output: 1384
🏁 Script executed:
#!/bin/bash
# Search for undefined name patterns in global_config files
rg "F821" ./tests/global_config*.py 2>/dev/null || echo "No explicit F821 markers found"
# Try to understand what might trigger F821 - look for variable usage that's not defined locally
rg "^\s*[a-zA-Z_][a-zA-Z0-9_]*\s*=" ./tests/global_config.py | head -30Repository: RedHatQE/openshift-virtualization-tests
Length of output: 1566
🏁 Script executed:
#!/bin/bash
# Check if there are OTHER files outside global_config that might have F821 violations
# that would be affected by the global ignore rule
rg "^[a-zA-Z_][a-zA-Z0-9_]*\s*=" --type py -A 2 | grep -v "global_config" | head -50Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2368
🏁 Script executed:
#!/bin/bash
# Get more of the global_config.py file to see F821 violations
tail -100 ./tests/global_config.py | head -80Repository: RedHatQE/openshift-virtualization-tests
Length of output: 3259
🏁 Script executed:
#!/bin/bash
# Search for bare raise statements specifically in global_config files
echo "=== Bare raise in global_config files ==="
rg "^\s*raise\s*$" ./tests/global_config*.py -A 2 -B 2 2>/dev/null | head -40Repository: RedHatQE/openshift-virtualization-tests
Length of output: 125
🏁 Script executed:
#!/bin/bash
# Count bare raise statements in global_config vs other test files
echo "=== Bare raise in global_config files ==="
rg "^\s*raise\s*$" ./tests/global_config*.py 2>/dev/null | wc -l
echo "=== Bare raise in OTHER test files (non-global_config) ==="
rg "^\s*raise\s*$" ./tests --glob='!global_config*.py' 2>/dev/null | wc -lRepository: RedHatQE/openshift-virtualization-tests
Length of output: 190
🏁 Script executed:
#!/bin/bash
# Check for undefined name (F821) in global_config files - these would trigger without the ignore
echo "=== Looking for undefined variables in global_config.py ==="
cat ./tests/global_config.py | grep -E "^\s+[a-zA-Z_][a-zA-Z0-9_]*\s*=" | tail -50Repository: RedHatQE/openshift-virtualization-tests
Length of output: 168
🏁 Script executed:
#!/bin/bash
# Search for variable references that aren't defined locally in global_config
# This is what F821 would catch - variables used but not defined
echo "=== Variables used in global_config that might trigger F821 ==="
rg "^[a-zA-Z_][a-zA-Z0-9_]*\s*=" ./tests/global_config.py -A 1 | grep -v "^--$" | head -60Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2404
🏁 Script executed:
#!/bin/bash
# Check what PLW0604 (the other global_config-specific ignore) is about
# Search for "global" keyword usage
echo "=== Global statements in global_config files ==="
rg "^global\s+" ./tests/global_config*.py 2>/dev/nullRepository: RedHatQE/openshift-virtualization-tests
Length of output: 946
🏁 Script executed:
#!/bin/bash
# Let's check the exact content around the bare raise statements in other test files
echo "=== Sample of bare raise in test utilities ==="
rg "^\s*raise\s*$" ./tests -B 3 -A 1 --max-count=10 2>/dev/nullRepository: RedHatQE/openshift-virtualization-tests
Length of output: 45491
🏁 Script executed:
#!/bin/bash
# Double-check: are there ANY undefined names (F821) currently in global_config files?
# Let's look at the actual dynamic loading pattern
grep -n "pytest_testconfig.load_python\|locals()\|globals()" ./tests/global_config.pyRepository: RedHatQE/openshift-virtualization-tests
Length of output: 267
🏁 Script executed:
#!/bin/bash
# Final verification: List all currently ignored rules and confirm which ones are truly needed for global_config
echo "=== Checking if PLE0704 is actually used in global_config files ==="
rg "except|try|raise" ./tests/global_config.py | head -20Repository: RedHatQE/openshift-virtualization-tests
Length of output: 318
Scope F821 correctly, but PLE0704 is not present in global_config files.
F821 violations in tests/global_config*.py are due to dynamic loading patterns (pytest_testconfig.load_python(), locals()) and should be scoped per-file. However, PLE0704 (bare raise) does not occur in these files—the 106 bare raise violations are in other test utilities and are correctly inside exception handlers. Scoping PLE0704 to global_config would not address the actual violations elsewhere and could mask issues if moved later.
Only F821 needs per-file scoping:
Proposed scoped configuration
[tool.ruff.lint]
ignore = [
"SIM102", # collapsible-if
...
- "F821", # undefined name — used in global_config files
- "PLE0704", # bare raise not inside exception handler
...
]
+[tool.ruff.lint.per-file-ignores]
+"tests/global_config*.py" = ["F821"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 34 - 37, Update the per-file-ignores entry so
only F821 is scoped to the dynamic-loading files (e.g., tests/global_config*.py)
and remove PLE0704 from that per-file list; specifically, add or change the
per-file-ignores rule to map tests/global_config*.py -> F821 (keeping PLE0704 in
the global ignore/violation list), and ensure any references to
pytest_testconfig.load_python() and use of locals() remain uncovered by the
PLE0704 per-file exception so bare-raise violations elsewhere (in test
utilities) remain detected.
| if migration_network and migration_network_on_pod: | ||
| verified_pods_list.append(pod) | ||
| elif not migration_network and not migration_network_on_pod: | ||
| if migration_network and migration_network_on_pod or not migration_network and not migration_network_on_pod: |
There was a problem hiding this comment.
HIGH — Fix ruff RUF021: unparenthesized and/or chain makes precedence ambiguous
Ruff 0.15.12 (the version this PR is bumping to) fires RUF021 on this line:
if migration_network and migration_network_on_pod or not migration_network and not migration_network_on_pod:Python's precedence evaluates this as (migration_network and migration_network_on_pod) or (not migration_network and not migration_network_on_pod), which is logically equivalent to migration_network == migration_network_on_pod. The behavior is correct, but ruff flags the missing parentheses as a readability hazard — and the PR's own goal is to make pre-commit run --all-files pass. Since the coding guidelines say "If a linter complains, fix the code instead" and RUF021 is not listed in the project's pyproject.toml ignores, this will keep failing CI.
Simplest fix that satisfies both the linter and the readability intent:
🔧 Proposed fix
- if migration_network and migration_network_on_pod or not migration_network and not migration_network_on_pod:
+ if migration_network == migration_network_on_pod:If you'd prefer to keep the explicit double-branch form, at minimum parenthesize to silence RUF021:
Alternative: parenthesized form
- if migration_network and migration_network_on_pod or not migration_network and not migration_network_on_pod:
+ if (migration_network and migration_network_on_pod) or (not migration_network and not migration_network_on_pod):🧰 Tools
🪛 Ruff (0.15.12)
[warning] 2817-2817: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
[warning] 2817-2817: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@utilities/virt.py` at line 2817, Replace the ambiguous boolean chain flagged
by RUF021 by making the intent explicit: change the condition that uses
migration_network and migration_network_on_pod (currently written as
"migration_network and migration_network_on_pod or not migration_network and not
migration_network_on_pod") to an equality comparison using migration_network ==
migration_network_on_pod so the precedence is unambiguous and ruff no longer
complains; alternatively, if you prefer the original dual-branch logic, add
parentheses around each conjunction (e.g., (migration_network and
migration_network_on_pod) or (not migration_network and not
migration_network_on_pod)) to satisfy RUF021.
6 participants
What this PR does / why we need it:
Replaces #4424 with a cleaner set of pre-commit and lint changes.
Hook bumps:
Ignore rules added to prevent CI failures from pre-existing code:
# noqacommentsCode fixes applied (safe, auto-fixable):
# -*- coding: utf-8 -*-headers (UP009)# noqacomments no longer needed by ruff v0.15.12logging.error()→LOGGER.error()(LOG015)Assisted-by: Claude noreply@anthropic.com
Which issue(s) this PR fixes:
Replaces #4424
Special notes for reviewer:
The ignored rules represent pre-existing issues exposed by ruff v0.15.12 preview mode — each should be addressed in dedicated follow-up PRs. pre-commit.ci runs on ALL files (not just changed), so all pre-existing violations must be either fixed or ignored.
jira-ticket:
Signed-off-by: Ruth Netser rnetser@redhat.com
Summary by CodeRabbit
Bug Fixes
Chores