feat: provider label discovery and catalog test_id metadata#485
Merged
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Changestest_ids field in catalog pipeline
Provider label discovery for isvctl test run
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
abegnoche
force-pushed
the
feat/provider-label-discovery
branch
from
b3575fc to
3e8b822
Compare
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-06-29 14:36:40 UTC | Commit: 3e8b822 |
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
`isvctl test run --label <x>` with neither --provider nor -f previously emitted "At least one --config/-f config file is required", omitting the --provider discovery path. Name both options so the label-discovery entry point is discoverable from the error. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Provider label discovery now applies the same released_tests.json filter as execution, so a config is not selected solely on an unreleased check that would be skipped at runtime (honoring ISVTEST_INCLUDE_UNRELEASED). Discovery failures are now distinguishable: an unknown --provider lists the available providers, and a valid provider with no label match lists the labels that provider exposes (surfacing typos like control-plane vs control_plane). Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Aggregates the catalog into the set of labels with a per-label test count, giving a first-class way to discover valid label values for label filtering and provider discovery. Honors the same release gate as `catalog list`. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Each catalog entry now carries the union of test-plan ids declared on its
suite/provider YAML wiring ("N/A" excluded), with a variant's ids propagated
to its base class. The entry identity stays the validation class name, so
result correlation and coverage are unchanged; test_ids is purely additive
metadata. isvreporter forwards the field on upload, and `isvctl catalog list`
shows a Test IDs column alongside a merged "Labels (Platforms)" column.
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The shared _TenantSanitizationCheck base class was surfacing as its own catalog entry. Mark it catalog_exclude and re-enable the flag on the four concrete subclasses (which would otherwise inherit the exclusion), so only the real sanitization checks appear in the catalog. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
- Generalize build_label_map/build_test_id_map over a shared _build_check_attribute_map(extract_fn) helper (scan + variant->base propagation written once). - Extract _iter_config_validations() in label_discovery so available_labels and discover_provider_label_configs share one merge+parse walk. - Sort catalog labels counts once in labels_cmd instead of per output branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
abegnoche
force-pushed
the
feat/provider-label-discovery
branch
from
3e8b822 to
d09a269
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
isvctl/tests/test_catalog_cli.py (1)
80-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the per-label counts in the table-path test too.
This only proves the labels are rendered. If the
Testscolumn disappeared or showed the wrong numbers, the new table output would still pass. Add assertions for the rendered counts so the non-JSON path verifies the command’s main behavior as well.🤖 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 `@isvctl/tests/test_catalog_cli.py` around lines 80 - 93, The `test_catalog_labels_table` coverage only checks that label names appear, so it can miss regressions in the `Tests` column output from `catalog labels`. Update this test to also assert the rendered per-label counts for the table path, using the existing `runner.invoke(app, ["labels"])` result from `test_catalog_labels_table` and the patched `build_catalog` entries to verify the expected numbers are shown alongside each label.
🤖 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 `@isvctl/src/isvctl/cli/catalog.py`:
- Around line 92-100: The combined labels/platforms display in catalog rendering
adds empty parentheses when an entry has platforms but no labels. Update the
formatting in the catalog table builder around the labels_platforms logic so the
code in the catalog listing only wraps platforms in parentheses when labels are
present, and otherwise renders just the platform text; use the existing symbols
entry, labels, platforms, and labels_platforms in the catalog.py table row
construction to locate the fix.
In `@isvctl/src/isvctl/cli/test.py`:
- Around line 286-305: Provider-discovery runs in test.py are generating a
per-config JUnit path via _junitxml_for_discovered_config in the recursive run()
call, but the upload path still only resolves junit-validation.xml and can miss
the actual artifact. Update the upload flow in run() and the discovered-config
loop to carry through the resolved junitxml for each match (including
provider-discovered runs) so the lab upload uses the per-config report instead
of falling back to a missing file. Refer to run(),
_junitxml_for_discovered_config(), and the provider-discovery branch that
iterates over matches.
In `@isvctl/tests/test_test_cli_labels.py`:
- Around line 98-110: Add PEP 257 docstrings to the new methods on
_FakeOrchestrator. Update both _FakeOrchestrator.__init__ and
_FakeOrchestrator.run in the test helper to include brief docstrings describing
their purpose and behavior, matching the style used elsewhere in the test suite.
---
Nitpick comments:
In `@isvctl/tests/test_catalog_cli.py`:
- Around line 80-93: The `test_catalog_labels_table` coverage only checks that
label names appear, so it can miss regressions in the `Tests` column output from
`catalog labels`. Update this test to also assert the rendered per-label counts
for the table path, using the existing `runner.invoke(app, ["labels"])` result
from `test_catalog_labels_table` and the patched `build_catalog` entries to
verify the expected numbers are shown alongside each label.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c8806135-d507-412b-9832-053a20ebc1bd
📒 Files selected for processing (11)
isvctl/src/isvctl/cli/catalog.pyisvctl/src/isvctl/cli/test.pyisvctl/src/isvctl/config/label_discovery.pyisvctl/tests/test_catalog_cli.pyisvctl/tests/test_provider_label_discovery.pyisvctl/tests/test_test_cli_labels.pyisvreporter/src/isvreporter/client.pyisvreporter/tests/test_catalog_upload.pyisvtest/src/isvtest/catalog.pyisvtest/src/isvtest/validations/sanitization.pyisvtest/tests/test_catalog.py
Add an opt-in --files flag to `isvctl catalog labels` that lists, per label, the config file(s) declaring it (via a new build_label_file_map config scan). Off by default so the table/JSON stay compact; --files adds a Files column and a per-label `files` array in --json. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
- Upload the per-config JUnit report from provider label discovery: the results upload now prefers the requested --junitxml path before the default name, so discovered runs no longer upload a null report. - catalog list: render platforms-only entries as "PLATFORM" instead of " (PLATFORM)" when a check has platforms but no labels. - Add PEP 257 docstrings to the _FakeOrchestrator test double methods. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@isvctl/src/isvctl/cli/catalog.py`:
- Around line 136-137: The helper files_for() is missing the required PEP 257
docstring even though it has a return type annotation. Add a concise docstring
directly above files_for() that describes what label it takes and that it
returns the sorted list of files for that label, following the repo’s docstring
convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 121797ad-b8e1-4d41-ad33-ed68690abab4
📒 Files selected for processing (5)
isvctl/src/isvctl/cli/catalog.pyisvctl/src/isvctl/cli/test.pyisvctl/tests/test_catalog_cli.pyisvctl/tests/test_test_cli_labels.pyisvtest/src/isvtest/catalog.py
🚧 Files skipped from review as they are similar to previous changes (3)
- isvctl/src/isvctl/cli/test.py
- isvtest/src/isvtest/catalog.py
- isvctl/tests/test_test_cli_labels.py
Address review: PEP 257 docstring required on the nested files_for() helper. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
mresvanis
approved these changes
Jun 30, 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
2 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.
Summary
isvctl test run --provider <name> --label <x>scansisvctl/configs/providers/<name>/config/*.yaml, matches configs whose resolved wiring carries all requested labels (release-gated, honoringISVTEST_INCLUDE_UNRELEASED), and runs each matching config as its own lifecycle.--dry-runprints the planned configs and matched checks.--labelwithout--provider/-fnames both options; an unknown--providerlists the available providers; a valid provider with no label match lists the labels that provider exposes (surfacing typos likecontrol-planevscontrol_plane).isvctl catalog labelsto list every catalog label with a per-label test count.test_ids(union of thetest_ids declared on each check's suite/provider wiring,"N/A"excluded, variant ids propagated to the base class). Surfaced inisvctl catalog listand forwarded by the isvreporter catalog upload. Entry identity stays the validation class name, so result correlation and coverage are unchanged._TenantSanitizationCheckbase from the catalog (the four concrete subclasses remain).Test plan
make testmake demo-testuv run isvctl test run --provider aws --label iam --dry-runuv run isvctl catalog labels/uv run isvctl catalog listSummary by CodeRabbit
catalog labelsto browse labels with per-label test counts, with optional--jsonand--filesoutput.test runwith provider-based label discovery (including--dry-rundiscovery plans).test_ids;catalog listnow shows a dedicated “Test IDs” column and consolidates labels/platforms formatting.--junitxmlwhen present.test_idsin the sent catalog payload.