Skip to content

refactor(constants): final cleanup of old constants imports#5440

Merged
vsibirsk merged 6 commits into
RedHatQE:mainfrom
vsibirsk:vk-constants-cleanup
Jul 2, 2026
Merged

refactor(constants): final cleanup of old constants imports#5440
vsibirsk merged 6 commits into
RedHatQE:mainfrom
vsibirsk:vk-constants-cleanup

Conversation

@vsibirsk

@vsibirsk vsibirsk commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator
What this PR does / why we need it:

Remove all re-exports from utilities/constants/init.py except Images. Migrate remaining callers to direct submodule imports, move NODE_STR from aaq.py to cluster.py, and update docs and analyzer test fixtures.
Updated docs for the new project modules structure guidelines

Assisted-by: Cursor Agent (Composer 2.5)

Which issue(s) this PR fixes:
Special notes for reviewer:

Final PR in refactor series https://redhat.atlassian.net/browse/CNV-80952
The only remaining constant in __init__.py is Images due to circular imports issue (planned to be fixed in follow up
refactors of remaining utilities/ modules.

jira-ticket:

https://redhat.atlassian.net/browse/CNV-89568

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features
    • Standardized the node dictionary key to node via a shared constant.
    • Updated multi-architecture image handling guidance to align with the new constants layout.
  • Bug Fixes
    • Refreshed constants usage across runtime and tests to consistently use the correct topic-specific sources.
  • Documentation
    • Updated project structure, architecture, and contributor guidance (including code organization and quarantine examples) for the constants package reorganization.
  • Tests
    • Updated unit, analyzer, and configuration tests to use the revised constants import locations.

Remove all re-exports from utilities/constants/__init__.py except Images.
Migrate remaining callers to direct submodule imports, move NODE_STR from
aaq.py to cluster.py, and update docs and analyzer test fixtures.

Assisted-by: Cursor Agent (Composer 2.5)
Signed-off-by: vsibirsk <vsibirsk@redhat.com>
@openshift-virtualization-qe-bot-5

Copy link
Copy Markdown

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

  • Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
  • Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
  • Issue Creation: A tracking issue is created for this PR and will be closed when the PR is merged or closed
  • Branch Labeling: Branch-specific labels are applied to track the target branch
  • Auto-verification: Auto-verified users have their PRs automatically marked as verified
  • Labels: Enabled categories: branch, can-be-merged, cherry-pick, has-conflicts, hold, needs-rebase, size, verified, wip

📋 Available Commands

PR Status Management

  • /wip - Mark PR as work in progress (adds WIP: prefix to title)
  • /wip cancel - Remove work in progress status
  • /hold - Block PR merging (approvers only)
  • /hold cancel - Unblock PR merging
  • /verified - Mark PR as verified
  • /verified cancel - Remove verification status
  • /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
  • /regenerate-welcome - Regenerate this welcome message
  • /security-override - Set security check runs to pass (maintainers only)
  • /security-override cancel - Re-run security checks

Review & Approval

  • /lgtm - Approve changes (looks good to me)
  • /approve - Approve PR (approvers only)
  • /assign-reviewers - Assign reviewers based on OWNERS file
  • /assign-reviewer @username - Assign specific reviewer
  • /check-can-merge - Check if PR meets merge requirements

Testing & Validation

  • /retest tox - Run Python test suite with tox
  • /retest build-container - Rebuild and test container image
  • /retest verify-bugs-are-open - verify-bugs-are-open
  • /retest all - Run all available tests

Container Operations

  • /build-and-push-container - Build and push container image (tagged with PR number)
    • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

  • /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
    • Multiple branches: /cherry-pick branch1 branch2 branch3
  • /cherry-pick-retry <branch> - Retry a failed cherry-pick (merged PRs only)

Branch Management

  • /rebase - Rebase this PR branch onto its base branch

Custom Commands

  • /test-plan - Triggers CodeRabbit to analyze the PR's changed files and post a test execution plan
  • /rerun-smoke - Adds `retest-smoke` label on the PR to trigger smoke tests exectuion

Label Management

  • /<label-name> - Add a label to the PR
  • /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

  1. Approval: /approve from at least one approver
  2. LGTM Count: Minimum 2 /lgtm from reviewers
  3. Status Checks: All required status checks must pass
  4. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
  5. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

  • EdDev
  • dshchedr
  • jpeimer
  • myakove
  • rnetser
  • vsibirsk

Reviewers:

  • Acedus
  • Ahmad-Hafe
  • Anatw
  • Dsanatar
  • EdDev
  • OhadRevah
  • RoniKishner
  • acinko-rh
  • akalenyu
  • albarker-rh
  • awels
  • azhivovk
  • dalia-frank
  • dshchedr
  • ema-aka-young
  • frenzyfriday
  • geetikakay
  • guchen11
  • hmeir
  • josemacassan
  • jpeimer
  • kgoldbla
  • kshvaika
  • nirdothan
  • orelmisan
  • rlobillo
  • rnetser
  • sarahbx
  • servolkov
  • vsibirsk
  • yossisegev
Available Labels
  • hold
  • verified
  • wip
  • lgtm
  • approve
AI Features
  • Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6-1m)
Security Checks
  • Suspicious Path Detection: Monitors paths: .claude/, .vscode/, .cursor/, .devcontainer/, .pi/, .github/workflows/, .github/actions/
  • Committer Identity Check: Verifies last committer matches PR author
  • Mandatory: Security checks block merge (use /security-override to bypass — maintainers only)

💡 Tips

  • WIP Status: Use /wip when your PR is not ready for review
  • Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
  • Cherry-picking: Cherry-pick labels are processed when the PR is merged
  • Container Builds: Container images are automatically tagged with the PR number
  • Permission Levels: Some commands require approver permissions
  • Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 92b6a228-4800-4d19-b365-7bf43335d02b

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Shared constants moved into dedicated submodules, utilities.constants.__init__ now computes and exports only Images, and NODE_STR was relocated to utilities.constants.cluster. Import sites, tests, and docs were updated to use the new module layout.

Changes

Constants package restructuring and import migration

Layer / File(s) Summary
Constants shim and NODE_STR move
utilities/constants/__init__.py, utilities/constants/aaq.py, utilities/constants/cluster.py
__init__.py now exports only computed Images, and NODE_STR is removed from aaq.py and added to cluster.py.
Library constant imports
libs/storage/config.py, libs/vm/factory.py, libs/vm/vm.py, utilities/architecture.py
Library code now imports constants from storage, architecture, images, virt, and cluster submodules.
Conftest and global config imports
conftest.py, tests/conftest.py, tests/global_config*.py
Shared test setup and environment-specific config files switch from bulk utilities.constants imports to targeted submodule imports.
Test, analyzer, and helper import updates
utilities/unittests/test_constants.py, utilities/unittests/test_pytest_utils.py, tests/utils.py, tests/after_cluster_deploy_sanity/..., tests/observability/..., tests/scale/..., tests/storage/..., scripts/tests_analyzer/...
Test helpers, assertions, and analyzer fixtures now import constants directly from cluster, images, instance_types, storage, timeouts, virt, monitoring, and pytest.
Docs and policy updates
docs/*.md, utilities/unittests/README.md, AGENTS.md
Documentation and contribution guidance now reference the constants package layout and fixture organization rules.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

Suggested reviewers: rnetser, dshchedr, hmeir, RoniKishner

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise, under 120 characters, and clearly matches the constants-import refactor.
Description check ✅ Passed The description follows the required template and covers the main sections, with only the issue list left blank.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stp Link Required ✅ Passed The two new test files carry required traceability docstrings, and no existing test file gained a new test name in this PR.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Linked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped RedHatQE/openshift-virtualization-tests-design-docs.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-virtualization-qe-bot-3

Copy link
Copy Markdown
Contributor

@coderabbitai

Test execution plan request details

CRITICAL: You MUST post an inline review comment on the first changed line of the first file.
The inline comment should contain the full Test Execution Plan (smoke decision, gating decision, and specific affected tests).
Do NOT submit a blocking review event (REQUEST_CHANGES/APPROVE).
Post a single inline PR comment on Files Changed (non-blocking COMMENT flow).

As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan.
You will post an inline review comment with the test execution plan on the first changed file.
If you fail to run or post a comment, retry.

Analysis Requirements:

  1. Examine code changes in each modified file

  2. Identify affected code paths, functions, and classes

  3. Analyze pytest-specific elements: fixtures (scope, dependencies), parametrization, markers, conftest changes

  4. Trace test dependencies through imports, shared utilities, fixture inheritance, fixture teardown, and yield from cleanup in conftest

  5. Detect new tests introduced in the PR

  6. Utilities and libs impact (when utilities/ or libs/ changes):
    You MUST use shell scripts (rg, git diff) to trace the full impact.
    Follow these sub-steps in order:

    6a. Identify modified symbols: For each changed file under utilities/ or libs/,
    list every modified function or method.
    Example: git diff HEAD~1 --unified=0 -- utilities/hco.py | grep '^[+-]def '

    6b. Find direct callers: Search tests and conftest for each symbol from 6a.
    Example: rg -l 'get_hco_version' tests/

    6c. Trace fixture teardown and cleanup: Find fixtures that reach
    the modified symbol through yield from or context-manager wrappers.
    Example: rg -l 'yield from.*enable_common_boot|def.*enable_common_boot' tests/

    6d. Trace same-file callers: In each changed file, find other functions
    whose body calls a modified symbol (including code after yield
    in @contextmanager helpers).
    Example: rg 'get_hco_version|enable_common_boot' utilities/hco.py

    6e. Expand transitively: If function A calls modified B, then
    tests/fixtures that call A are affected — even when the test body
    never imports B directly.

    Do NOT limit impact to tests that import the modified symbol only.

  7. Smoke test impact: Intersect the affected set from step 6 with smoke-marked tests.
    Run: rg -l '@pytest.mark.smoke' tests/
    VERIFY the above command returned actual file paths before concluding False.
    Set True if either condition is met:

    • a smoke-marked file appears in the affected set from 6b-6e, OR
    • any conftest.py in the smoke test's parent-directory hierarchy (up to repo root)
      imports or calls a modified utilities/libs symbol — including autouse fixtures
      that depend on modified functions. ALL tests in that directory and below are affected.
      Example check: for each smoke_file, scan dirname(smoke_file)/conftest.py,
      dirname(dirname(smoke_file))/conftest.py, etc. for modified symbol imports
      and autouse fixtures that depend on modified symbols.
  8. Gating test impact: Intersect the affected set from step 6 with gating-marked tests.
    Run: rg -l '@pytest.mark.gating' tests/
    Set True if a gating-marked file also appears in the affected set from 6b-6e.
    Utilities/libs changes often affect gating tests without affecting smoke tests.
    Do NOT stop analysis after concluding Run smoke tests: False.

Output rules:
Do NOT include analysis step numbers (1-8) in your visible output.

Your deliverable:
Your inline informational comment will be based on the following requirements:

Test Execution Plan

  • Run smoke tests: True / False — If True, state the dependency path (test → fixture → changed symbol). True ONLY with a verified path.
  • Run gating tests: True / False — If True, state the dependency path. True if any gating-marked test is in the affected set.
  • Affected tests to run (required when utilities/, libs/, or shared conftest changes — list concrete paths even when smoke is False)

Use these formats:

  • path/to/test_file.py - When the entire test file needs verification
  • path/to/test_file.py::TestClass::test_method - When specific test(s) needed
  • path/to/test_file.py::test_function - When specific test(s) needed
  • -m marker - When a marker covers multiple affected tests (e.g. -m gating only if ALL gating tests in scope need run)
  • Tag each listed test or group with its marker when not obvious, e.g. (gating) or (smoke)

Real test commands (MANDATORY when changes affect session/runtime code):

When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
config hooks, session-scoped setup) or modifies runtime behavior that unit tests mock away,
you MUST include concrete pytest commands the PR author must run on a real cluster
to verify the change works end-to-end. Include:

  • A command for the error/fix path (the scenario the PR fixes)
  • A command for the happy path (regression: the normal case still works)
  • Use lightweight tests (e.g., --collect-only for startup failures,
    a single small test for runtime behavior)
    If the PR only changes test logic (not utilities/libs/conftest), the affected test
    paths themselves serve as the real test commands — no separate section needed.

Example output for a session-startup fix:

**Real tests (cluster required)**
Error path (the fix):
`pytest tests/storage/.../test_foo.py --storage-class-matrix=nonexistent-sc --collect-only`
Expected: ValueError with clear message, not IndexError

Happy path (regression):
`pytest tests/storage/.../test_foo.py --storage-class-matrix=<valid-sc> -k test_bar`
Expected: session starts normally

Guidelines:

  • Include tests affected directly OR via fixture setup/teardown, yield from cleanup, or transitive utility call chains (caller calls modified helper)
  • Use a full file path only if ALL tests in that file require verification
  • Use file path + test name when only specific tests use an affected fixture or utility wrapper (preferred for partial file impact)
  • If a test marker can cover multiple files/tests, provide the marker
  • Balance coverage vs over-testing - Keep descriptions minimal
  • Example: if leaf helper foo() changes, include tests whose fixture teardown calls wrapper bar() where bar() calls foo(), even when the test body only imports an unrelated symbol from the same utilities module

Hardware-Related Checks (SR-IOV, GPU, DPDK):

When PR modifies fixtures for hardware-specific resources:

  • Collection Safety: Fixtures MUST have existence checks (return None when hardware unavailable)
  • Test Plan: MUST verify both WITH and WITHOUT hardware:
    • Run affected tests on cluster WITH hardware
    • Verify collection succeeds on cluster WITHOUT hardware

CRITICAL WORKFLOW COMPLETION RULES:

When responding to this test execution plan request, you MUST follow these rules EXACTLY:

  1. YOUR ONLY DELIVERABLE: Post one non-blocking inline comment containing the test execution plan on the first changed line
  2. THEN STOP IMMEDIATELY - Do NOT generate any additional response
  3. FALLBACK ONLY: If inline comment API calls fail after retrying, post as a regular PR comment
  4. SILENCE = SUCCESS: After successfully submitting the review, your task is complete. No confirmation needed.

ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):

  • ❌ Do NOT post acknowledgment messages like "Test execution plan posted", "Review posted successfully", "I've successfully posted"
  • ❌ Do NOT mention review IDs, URLs, or confirmation of posting in the PR thread
  • ❌ Do NOT add any follow-up comments after submitting the review
  • ❌ Do NOT reply to confirm task completion
  • ❌ Do NOT explain what you did - just do it and stop

Remember: The pull request review is visible to users. Additional comments are redundant noise.

CRITICAL — Verification rules for smoke and gating decisions:
- For each True decision, you must have traced a concrete path: test file → fixture → utility function → modified symbol.
- Do NOT assume impact based on scope, file proximity, or infrastructure-sounding names.
Only set True when you trace a concrete path through steps 6a-6e, then 7 or 8.
WRONG: "utilities/hco.py changed → run smoke" (no traced dependency path)
WRONG: "Run smoke tests: False" with no affected-test list when gating tests use the changed utility
RIGHT: "Run smoke tests: False, Run gating tests: True" plus tests/.../test_foo.py::TestBar::test_baz (gating)
RIGHT: "Smoke test X uses fixture Y → wrapper Z → changed function W in utilities/hco.py"

@coderabbitai

coderabbitai Bot commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

@openshift-virtualization-qe-bot-3, I don't have a capability to force a raw inline review comment pinned to an arbitrary diff line outside the normal review workflow — my available actions are limited to things like , , , etc. I can trigger a full re-review with , which will run CodeRabbit's standard walkthrough + comments, but I can't fabricate a stand-alone non-blocking inline comment on demand like this. I'll give you the test execution plan directly here instead.

Test Execution Plan

  • Run smoke tests: False — No traced dependency path found. This PR is almost entirely import-path changes (from utilities.constants import Xfrom utilities.constants.<submodule> import X); no function logic changed in utilities/ or libs/ files touched (libs/storage/config.py, libs/vm/factory.py, libs/vm/vm.py, utilities/architecture.py, utilities/os_utils.py).
  • Run gating tests: Trueutilities/pytest_utils.py::update_cpu_arch_related_config() (called during session config, feeding global_config/Images used across the suite) now sources ArchImages from utilities.constants.images instead of the aggregate module, and utilities/constants/__init__.py now computes Images eagerly at import time via get_cluster_architecture() + ArchImages. Any import ordering/circular-import regression here would break collection for the entire gating set, since Images is consumed broadly (e.g. libs/vm/factory.py::fedora_image).

Affected tests to run

  • --collect-only across tests/ (verifies no ImportError/circular-import regression from the utilities/constants/__init__.py rewrite and NODE_STR relocation)
  • tests/observability/metrics/utils.py consumers — NODE_STR moved from utilities.constants.aaqutilities.constants.cluster; run:
    • tests/storage/hpp/test_hpp_node_placement.py (gating, uses NODE_STR)
    • any test exercising tests/observability/metrics/utils.py helpers
  • utilities/unittests/test_pytest_utils.py::TestUpdateCpuArchRelatedConfig — already updated for ArchImages mock path, run to confirm patch target correctness
  • utilities/unittests/test_constants.py — full file, given the __init__.py rewrite
  • libs/vm/factory.py::fedora_image consumers, e.g. any VM-creation test in tests/virt/ using multiarch fedora images

Real tests (cluster required)
Error/fix path (import resolution across arches):
pytest --tc-file=tests/global_config_arm64.py --tc-format=python tests/ --collect-only
Expected: clean collection, no ImportError for Images/ArchImages/NODE_STR

Happy path (regression, amd64 default):
pytest --tc-file=tests/global_config_amd64.py --tc-format=python tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py -k test_create_vm
Expected: passes as before, confirming Images computed correctly post-refactor

Note: your manual smoke run in this thread (22 passed) plus full collect-only (2156/2234 collected) already covers the primary regression risk for the amd64 path.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@rnetser

rnetser commented Jul 1, 2026

Copy link
Copy Markdown
Collaborator

/approve
/lgtm

@jpeimer

jpeimer commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

/approve
/lgtm

@vsibirsk
vsibirsk merged commit 93d8ab2 into RedHatQE:main Jul 2, 2026
28 checks passed
@vsibirsk
vsibirsk deleted the vk-constants-cleanup branch July 2, 2026 11:53
rnetser added a commit to rnetser/openshift-virtualization-tests that referenced this pull request Jul 2, 2026
- Fix broken links to QUARANTINE_GUIDELINES.md and SOFTWARE_TEST_DESCRIPTION.md
- Remove nonexistent tier1 from tier marker semantics
- Fix typo: "commiting" → "committing"
- Generalize utility module placement (no explicit module list)
- Document gating marker and tier2 exclusion markers reference
- Document quarantined marker auto-add behavior (with run=False)
- Add Scripts Directory section
- Add missing Related Documentation links (AI policy, maintainer guidelines, arch support)
- Remove Constants and conftest Architecture sections (covered by CODE_ORGANIZATION.md from PR RedHatQE#5440)

Signed-off-by: Ruth Netser <rnetser@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>

Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev>
Signed-off-by: rnetser <rnetser@redhat.com>
rnetser added a commit to rnetser/openshift-virtualization-tests that referenced this pull request Jul 2, 2026
- Fix broken links to QUARANTINE_GUIDELINES.md and SOFTWARE_TEST_DESCRIPTION.md
- Remove nonexistent tier1 from tier marker semantics
- Fix typo: "commiting" → "committing"
- Generalize utility module placement (no explicit module list)
- Document gating marker and tier2 exclusion markers reference
- Document quarantined marker auto-add behavior (with run=False)
- Add Scripts Directory section
- Add missing Related Documentation links (AI policy, maintainer guidelines, arch support)
- Remove Constants and conftest Architecture sections (covered by CODE_ORGANIZATION.md from PR RedHatQE#5440)

Signed-off-by: Ruth Netser <rnetser@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>

Co-authored-by: PI (claude-opus-4-6) <noreply@pi.dev>
Signed-off-by: rnetser <rnetser@redhat.com>
@openshift-virtualization-qe-bot-4

Copy link
Copy Markdown

New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants