Repository navigation
feat: allow configurable providers.json file path - #323
Merged
Merged
Conversation
Add --providers-json pytest CLI arg and PROVIDERS_JSON_PATH env var to override the default .providers.json path. - Add resolve_providers_json_path() with priority: CLI > env > default - Add ProviderEmptyContentError for empty provider files - Move module-level provider-type skip logic to pytest_collection_modifyitems - Register copyoffload_snapshots marker for vSphere-only snapshot tests - Update copy-offload error messages to use resolved path - Guard collection hook with is_dry_run for --collect-only support - Update CLAUDE.md with provider loading and marker naming rules - Update README.md with new configuration options Closes #322
Contributor
|
Caution Review failedFailed to post review comments WalkthroughAdds configurable providers JSON path resolution (CLI arg > env var > default) with validation and new exceptions; moves provider-loading to fixtures and collection-time gating via pytest markers; adds Changes
Sequence Diagram(s)sequenceDiagram
participant User as Test Runner (pytest CLI)
participant Pytest as Pytest Collector
participant Conftest as conftest.py hooks
participant Utils as utilities/utils.py
participant FS as Filesystem
User->>Pytest: start collection (--providers-json?)
Pytest->>Conftest: call pytest_addoption / collection hooks
Conftest->>Utils: resolve_providers_json_path(cli_path)
Utils->>FS: check CLI path → env → default `.providers.json`
FS-->>Utils: return resolved path / error
Utils-->>Conftest: return providers dict (or raise)
Conftest->>Pytest: pytest_collection_modifyitems applies markers/skips based on providers
Pytest->>Tests: run marked/skipped tests accordingly
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
Review Summary by QodoAllow configurable providers.json file path with CLI and environment variable support
WalkthroughsDescription• Add configurable providers JSON file path via CLI arg and environment variable • Move provider-type skip logic from module level to pytest collection hook • Register copyoffload_snapshots marker for vSphere-only snapshot tests • Update error messages to display resolved provider file path • Remove module-level provider loading from test files to respect CLI arguments Diagramflowchart LR
CLI["--providers-json CLI arg"]
ENV["PROVIDERS_JSON_PATH env var"]
DEFAULT[".providers.json default"]
RESOLVE["resolve_providers_json_path()"]
LOAD["load_source_providers()"]
HOOK["pytest_collection_modifyitems hook"]
SKIP["Apply skip markers"]
CLI --> RESOLVE
ENV --> RESOLVE
DEFAULT --> RESOLVE
RESOLVE --> LOAD
LOAD --> HOOK
HOOK --> SKIP
File Changes2. exceptions/exceptions.py
|
|
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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Around line 612-620: The document currently mandates class-name rules with
"MUST" under "Test class naming for marker-based features (MUST)"; change the
wording to a recommendation or add an enforcement mechanism: either replace
"MUST" with "SHOULD" (or "Recommended") and rephrase the bullets (e.g., Warm →
include `Warm`, Copy-offload snapshots → include `Copyoffload` and `Snapshot`,
Copy-offload → include `Copyoffload`) to make them discoverability guidance, or
keep "MUST" and add a clear enforcement note and link to the suite check (e.g.,
add a test/linter that validates class names like TestSanityWarmMtvMigration,
TestCopyoffloadThinSnapshotsMigration, TestCopyoffloadThinMigration); ensure the
header and examples are updated accordingly so the intent is unambiguous.
In `@conftest.py`:
- Around line 308-315: The collection hook currently only skips items with the
"copyoffload_snapshots" keyword, so copy-offload tests marked with
pytest.mark.copyoffload or using the copyoffload_config fixture still get
collected and later fail; update the hook to also detect and skip items with the
"copyoffload" marker and/or items that list the copyoffload_config fixture
(check item.keywords for "copyoffload" and/or "copyoffload_config") when
source_provider_type != Provider.ProviderType.VSPHERE, and apply the same
snapshot_skip marker (rename to a generic copyoffload_skip if you like) to avoid
runtime pytest.fail() in the copyoffload_config fixture.
- Line 1313: Add full type annotations to the changed fixture signatures:
annotate copyoffload_config(source_provider: str, source_provider_data:
Dict[str, Any], request: pytest.FixtureRequest) -> Dict[str, Any] and likewise
annotate copyoffload_storage_secret(source_provider: str, source_provider_data:
Dict[str, Any], request: pytest.FixtureRequest) -> Dict[str, Any]; import typing
types (from typing import Any, Dict) and pytest.FixtureRequest (or
_pytest.fixtures.FixtureRequest) as needed so both parameters and the return
types are fully typed per the guideline.
In `@README.md`:
- Around line 146-153: The README currently reintroduces a hardcoded expectation
by instructing users to "Create a `.providers.json` file in your current
directory"; change this to clarify that the default is a file named
`.providers.json` in the current working directory but that it can be overridden
by the CLI flag `--providers-json` or the environment variable
`PROVIDERS_JSON_PATH` (with priority CLI > ENV > default), and add a short note
for container/OpenShift users to ensure the mounted file path matches the
runtime path provided to `--providers-json` or `PROVIDERS_JSON_PATH` rather than
assuming the repository root.
In `@utilities/utils.py`:
- Around line 90-94: The function currently returns parsed JSON from
json.loads(content) without validating its shape, which can lead to later
failures when callers expect a mapping; after calling json.loads(content) (near
resolved_path and ProviderEmptyContentError), verify the result is a dict
(isinstance(result, dict)) and if not raise a ValueError with a clear message
that the provider config must be a JSON object (include the resolved_path and
the actual top-level type in the message) so callers like conftest.py don't
receive lists/null/strings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b8bc5d53-f4e4-4f28-aa56-a8b076c11853
📒 Files selected for processing (9)
CLAUDE.mdREADME.mdconftest.pyexceptions/exceptions.pypytest.initests/test_copyoffload_migration.pytests/test_mtv_warm_migration.pytests/test_warm_migration_comprehensive.pyutilities/utils.py
redhat-qe-bot1
added
commented-coderabbitai[bot]
lgtm-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
- Add type annotations to copyoffload fixtures - Skip all copyoffload tests for non-vSphere providers (not just snapshots) - Add JSON shape validation in load_source_providers() - Add docstrings to custom exception classes - Add inline comment explaining py_config.get() default usage - Change test class naming rule from MUST to SHOULD - Improve README provider file documentation wording
|
Persistent review updated to latest commit 6ed8f10 |
|
Persistent review updated to latest commit 6ed8f10 |
|
Failed to build and push ghcr.io/redhatqe/mtv-api-tests:latest |
5 tasks
This was referenced Jul 8, 2026
1 of 3 tasks
This was referenced Aug 19, 2026
This was referenced Oct 8, 2026
This was referenced Oct 15, 2026
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
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.
Summary
Closes #322
Allow users to configure the providers JSON file path instead of hardcoding
.providers.json.New configuration options (priority order)
--providers-json /path/to/file.json— pytest CLI argumentPROVIDERS_JSON_PATH=/path/to/file.json— environment variable.providers.json— default (unchanged)Changes
utilities/utils.py: Addedresolve_providers_json_path()helper with file validation (is_file()check).load_source_providers()now accepts optional path parameter and raisesProviderEmptyContentErrorfor empty files,FileNotFoundErrorfor missing files.conftest.py: Added--providers-jsonCLI option. Updatedsource_providersandsource_provider_datafixtures. Moved module-level provider-type skip logic intopytest_collection_modifyitemshook (guarded byis_dry_runfor--collect-onlysupport). Added documented pattern for adding new skip rules.exceptions/exceptions.py: AddedProviderEmptyContentError. UpdatedMissingProvidersFileErrorto accept path parameter.pytest.ini: Registeredcopyoffload_snapshotsmarker._SOURCE_PROVIDER_TYPEfrom 3 test files. Replacedskipifwith@pytest.mark.copyoffload_snapshotsmarker.CLAUDE.md: Added "No Module-Level Provider Loading" rule, updated Test Markers table, added marker and naming conventions.README.md: Documented new configuration options.Test plan
pre-commit run --all-filespassesuv run pytest --collect-only -m warm— correctly collects warm testsuv run pytest --collect-only -m copyoffload_snapshots— correctly collects snapshot testsuv run pytest --collect-only -m warm --tc=source_provider:openstack-psi— skip markers applied for unsupported providersSummary by CodeRabbit
New Features
Documentation
Bug Fixes