feat: select validations by suite and capability - #560
Open
abegnoche wants to merge 34 commits into
Open
Conversation
Introduce declarable capability vocabulary, expand_capabilities() for compute inheritance from vm/bare_metal, per-check requires filtering with explicit capability_requirement skip reasons, and validation for requires values on plain-suite checks. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Reject tests.module, tests.platform=compute, requires on platform suites, and legacy per-check platforms fields. Rewrite validate_suite_wiring for plain vs platform suite rules and globally unique wiring names. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Resolve platform or plain suite configs via unified --suite selection, parse optional --capabilities filter context (rejecting compute), apply capability filtering in orchestration with human-readable dry-run plans, and skip setup steps when all bound checks are filtered out. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Build catalog entries from canonical suite YAML with source, suite, platform, and requires metadata. Bump schemaVersion to 2, replace platforms with capabilities in the envelope, and remove the legacy catalog_platforms registry module. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Remove tests.platform from plain capability suites, add per-check requires from requires-worksheet, move kubernetes storage checks into storage.yaml with ensure_cluster wiring, add my-isv demo stub and AWS reference setup, and drop provider platform keys from plain-suite command groups. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Align merger, nico provider, and capacity config tests with plain-suite capability resolution and catalog schema v2 entry shape. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
my-isv storage demo exercises compute-path checks only; pass --capabilities vm so kubernetes requires checks are skipped with explicit capability_requirement reasons. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Emit declarable capabilities as `platforms` and plain suite names as `suites` in catalog documents and upload payloads so backend catalog ingestion matches the feat/test-structure contract.
Both flags now build and save the catalog locally without uploading, using --dry-run as the primary name for consistency with other commands. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com> (cherry picked from commit 1550697)
K8S23-04..07 gained the storage label in the suite YAML; the test-plan coverage guardrail requires the doc to carry the same label union. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The four capabilities (kubernetes, slurm, vm, bare_metal) are mutually exclusive execution environments — you run on one, never a combination — so a comma-separated multi-value context never made sense. - Rename the flag --capabilities -> --capability (single value); reject comma-separated input with an explanatory error. - parse_capabilities -> parse_capability (returns str | None). - requirements_satisfied / resolve_entries take one capability instead of a set: `not requires or capability in requires`. - Thread a single capability through the orchestrator and dry-run. `requires` (per-check) stays a list: any-match applicability across the isolated contexts, not a request to combine them. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
A plain suite named after a declarable capability (vm/bare_metal/kubernetes/slurm) would collapse the merged frontend test-target list and cross-wire the backend's flat-selection re-split (selection ∩ catalog.platforms). Reject the collision at the upload chokepoint (catalog_document) and in the wiring lint (validate_suite_wiring.py), reusing DECLARABLE_CAPABILITIES, so the capability-vs-suite distinction stays recoverable by construction. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
test validate now rejects unknown/duplicate requires values via the Pydantic suite-shape validator, matching the run-time and wiring checks. validate_suite_wiring flags a requires naming a capability with no platform suite (no ISV can declare it, so the check is unreachable). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Capability gating skips a step when it carries an explicit `requires:` or when every validation bound to it is requirement-filtered. Teardown steps have no bound validations, so inference cannot reach them: under the core-by-default `--suite` path they kept running against fixtures that were never created. Only storage carried explicit gates; three other suites had the same hole. - image-registry: teardown deleted both the core upload_image resources and the vm-only launch_instance ones, so it referenced a skipped step and was abandoned whole with "missing step reference" - leaking the image, disks and bucket. Split by owner into teardown_instance (requires: [vm]) and teardown_image (core); the scripts already guard each resource independently, so no script change was needed. Rebind the suite's teardown check to teardown_image, the half that always runs. - observability: gate launch_host/teardown_host on [vm, bare_metal] so a core run no longer launches the reference metal host only to skip every check that needed it. - security: gate topology_block_teardown/capacity_teardown on [vm, bare_metal], mirroring the gate inferred on the test-phase steps that allocate those resources. StorageCapacityTelemetryCheck moves from core to [vm, bare_metal]. It probes the volumes attached to a running instance via the same script and --instance-id as its StoragePerformanceTelemetryCheck twin, which already declared that gate; with no instance it reports "no volumes attached", so it could never pass as a core check. Needs a catalog re-upload. Add test_capability_step_gating.py: instead of three point tests it walks every plain-suite provider config across every context and asserts no surviving step reads a gated-off step's output without a `default(...)`, so new suites inherit the guarantee. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
my-isv is the folder ISVs copy, so a guarantee that holds only in aws/ teaches the wrong lifecycle. Three divergences, all in the capability-gated paths added on this branch. - storage: setup_cluster carried no `requires: [kubernetes]` and had no teardown_cluster at all, so the scaffold demonstrated acquiring a cluster and never releasing it - the opposite of the setup_cluster/teardown_cluster contract aws/ has carried since 607c121. Add both, with STORAGE_SKIP_TEARDOWN mirroring AWS_SKIP_TEARDOWN, and a teardown_cluster.py stub in the house style. A no-op teardown is a valid answer when the stub reuses a long-lived cluster; the step still has to exist so a standalone storage run cannot leak a cluster it provisioned. - kubernetes/slurm: no config existed, so `--suite kubernetes --provider my-isv` errored - and that is exactly what the frontend emits for its default provider. The scripts were already there; only the wiring was missing. Steps are overridden rather than inherited because the canonical suites point at ../providers/my-isv/scripts/*, which resolves wrong from a provider config directory. Neither joins `make demo-test`: both drive real kubectl/sinfo, so a dummy-success stub has nothing to return. Also fixes a pre-existing scaffold break found while verifying the above: `--suite storage --capability kubernetes` failed with "'cluster_name' is a required property". The step name auto-detects the platform-suite `cluster` schema, which demands cluster_name/node_count - inventory no storage check reads. Set `output_schema: generic` on both providers; scoping it to my-isv would have recreated the divergence this commit removes, and per the JSON contract discipline storage must not require cluster inventory of an ISV. test_storage_cleanup_steps_have_explicit_capability_gates encoded the divergence as `if provider == "aws":`; both providers now assert the same gates. Add coverage that every declarable capability resolves to a my-isv platform suite, so a UI-emitted `--suite <capability>` cannot silently become an error again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
…path The same config ran a different set of checks depending on how it was named: `--suite network` gave 26 checks (core default), `-f .../network.yaml` gave 35 (unfiltered), and `--provider X --label network` gave 35 as well. The label path is the one requirements.md NVIDIA#4 calls out by name - label selection must not run checks gated on a capability the ISV does not support - and it did. Resolve the context after config validation instead of inside the `--suite` branch, so one rule covers `--suite`, `-f` and `--label` discovery alike: a plain suite with no `--capability` runs its core checks. Platform suites declare no `requires:`, so they keep the unfiltered context. Label discovery inherits this through its recursive call, which closes NVIDIA#4 without special casing it. This drops the "no filtering" pseudo-context as a default. It modelled no real ISV situation - nobody runs on vm and kubernetes at once - so a plain suite now always carries exactly one context. Warn when `--capability` names something no check in the suite requires (`--suite iam --capability kubernetes`): the run proceeds with core checks, but a flag that silently does nothing reads as a typo. UI-composed commands are unaffected. `-f` used to hand the demos their gated checks for free, so the Makefile now names a capability per suite to keep exercising those stubs; without it `make demo-test` would quietly shrink by ~29 checks and stop covering the launch_instance / install_image_bm / install_config_bm scripts while still printing all-green. image-registry splits its gated checks between vm and bare_metal, so it runs once per context. Document the rule in configs/suites/README.md, which described neither `requires:` nor the selection grammar. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
`requires:`, `--suite` and `--capability` appeared in no user-facing doc. The model existed only in the code and in decision.md, an internal design record, so an ISV had no way to learn what governs which checks they owe. - configs/suites/README.md already gained the rule when the default changed; this adds the full model to the two guides an ISV actually reads. - configuration.md: a "Capabilities and requires" section (the four mutually exclusive capabilities, platform vs plain suites, any-match semantics and why AND is deliberately inexpressible, the one selection rule, opt-in scope, the shared namespace), plus `requires` in the step field table with the gate-both-halves-of-a-fixture guidance. - external-validation-guide.md: the same model stated for an ISV audience, and a Running Validations section that shows the --suite/--capability grammar, --dry-run, label composition and single-check rerun by pytest passthrough. - my-isv scaffold README: real invocations rather than only the -f form, plus what governs which checks run. Also corrects drift found in the scaffold's Domains table: 7 of 11 script counts were wrong, and k8s/slurm listed no provider YAML - true until those configs landed in 3735674. Both now note they need a real cluster and so sit outside `make demo-test`. Drops a stale MY_ISV_DOMAINS reference for the MY_ISV_SUITES / DEMO_CAP_<suite> variables that replaced it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
A run's signal is the pair, not the suite alone: `network` and `network --capability vm` execute different checks, so recording only one axis loses the distinction at the point where it matters most. Neither axis was transmitted at all -- the service inferred a single test_target_type from the first command, which collapsed both. Two client-side spellings have to be translated before they leave the process. CORE_REQUIREMENT_CONTEXT is this module's word for "no capability", which the service records as NULL rather than a sentinel. And a platform suite carries no explicit context because its own platform *is* the capability it runs under, so report that instead of dropping the axis for every platform-suite run. resolve_suite_name recovers the suite from every entry path, including `-f lab.yaml -f commands.yaml -f suites/k8s.yaml` where the first config is not the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… suites The pre-commit validator carried a hand-copied DECLARABLE_CAPABILITIES literal, so the hook that exists to enforce the vocabulary was the one place that could silently drift from the schema it enforces. It now imports the shared constant, and the REQUIREMENT_VOCABULARY alias (a pure alias with no external importers) is gone. Each suite YAML was also read three to four times and parsed two to three times per run: _build_suite_map parsed a file and then called iter_config_checks, which re-read and re-parsed the same file, and wiring_errors globbed the directory twice while reading each file twice more in its main loop. Both now expose iter_checks_from_data so a caller that has already parsed the document reuses it, and wiring_errors reads and parses each file exactly once. The suite vocabularies are cached: resolve_suite_name alone scanned the suites directory twice per invocation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
Two rules had each been written out four times. `requires` validation (a list, drawn from DECLARABLE_CAPABILITIES, no duplicates) lived in StepConfig.validate_requires, in ValidationConfig.validate_suite_shape, in _validate_entry_shape, and in the wiring script -- with four separately maintained message strings, so "requires must contain only" and "requires must be a list containing only" were the same verdict spelled two ways. requires_error() states it next to the vocabulary it validates; each caller adds only its own prefix. The pre-commit hook and the runtime validator can no longer disagree about what a valid suite file is. Suite-name canonicalization was worse, because suite name is the join key between a test run and its catalog entries: `_normalize_name` did lower + `-`/`_` + the k8s alias, `catalog.py` did the replace alone in two places, and the wiring script hardcoded `if stem == "k8s"`. Adding a second aliased filename meant finding all four. canonical_suite_name() in isvtest.core.resolution is now the single authority -- isvctl already depends on isvtest, so nothing new is coupled. Alongside those, three smaller consolidations: iter_checks_from_data existed twice, differing only in whether the category is yielded and in a branch the script's copy had dropped (dict-form categories without a `checks` key). The three-tuple version in isvtest.catalog is now the only walker of the wiring shape; no suite YAML uses the missing branch today, so the script only gains coverage. catalog_document parsed the suites directory three times -- once via _build_suite_map, then again in each vocabulary builder. _iter_suite_docs makes it one pass, and suite_vocabularies returns both lists from it. CONFIGS_ROOT moves from cli/test.py to config/suite_resolution.py, so `deploy` no longer imports the whole `test` command module -- typer app, orchestrator and all -- to obtain one Path. _suite_name gains @cache, since classifying a config merges it with the suite it imports and a single run asks both resolve_suite and resolve_suite_name for it. test_capability_step_gating copied the run/skip cascade out of _apply_capability_step_gates into its own _gated_step_names, so it would have kept passing against the old rule if the real gate changed. It now calls the gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
🔐 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-07-27 20:43:51 UTC | Commit: 7a012db |
📝 WalkthroughWalkthroughThe change replaces platform-based suite selection with capability-aware suite resolution and step gating, adds suite and capability metadata to catalogs and test runs, updates provider lifecycle configurations, and expands CLI, documentation, and validation coverage. ChangesCapability suite execution
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/validate_suite_wiring.py (1)
219-226: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
--checkhelp text still advertises removed validators.Line 229 now runs only
wiring_errors(); the catalog-platform registration and isvreporter consistency checks are gone, so "unregistered suites, or isvreporter platform mismatches" no longer describes anything this flag can report.📝 Proposed help-text update
parser.add_argument( "--check", action="store_true", - help=( - "Exit 1 on wiring violations (missing test_id/labels, unregistered suites, " - "or isvreporter platform mismatches)." - ), + help=( + "Exit 1 on wiring violations (missing test_id/labels/suite label, " + "invalid tests.platform, or invalid/unreachable requires)." + ), )🤖 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 `@scripts/validate_suite_wiring.py` around lines 219 - 226, Update the --check argument help text in the validate_suite_wiring parser to describe only the validations currently performed by wiring_errors(), removing references to unregistered suites and isvreporter platform mismatches.isvreporter/src/isvreporter/main.py (1)
306-322: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winLegacy/incomplete catalog JSON silently uploads with an empty v2 axis vocabulary.
catalog_data.get("schemaVersion", 1),.get("platforms", []),.get("suites", [])let a stale or malformedtest_catalog.json(missing these v2 keys) sail through as a "valid" upload withschemaVersion: 1but the new v2-shaped entry fields (source/suite/platform/requires). This undercuts the intent behind making these three params required (no defaults) onupload_test_catalogitself — the guard is bypassed here via optimistic defaults instead of failing loudly on an incomplete envelope.🐛 Proposed fix: fail fast on missing v2 catalog metadata
- upload_test_catalog( - endpoint=endpoint, - jwt_token=jwt_token, - isv_test_version=catalog_version, - entries=catalog_entries, - schema_version=catalog_data.get("schemaVersion", 1), - platforms=catalog_data.get("platforms", []), - suites=catalog_data.get("suites", []), - ) + if "schemaVersion" not in catalog_data or "platforms" not in catalog_data or "suites" not in catalog_data: + typer.echo( + f"Warning: {test_catalog} is missing schemaVersion/platforms/suites; skipping upload", err=True + ) + else: + upload_test_catalog( + endpoint=endpoint, + jwt_token=jwt_token, + isv_test_version=catalog_version, + entries=catalog_entries, + schema_version=catalog_data["schemaVersion"], + platforms=catalog_data["platforms"], + suites=catalog_data["suites"], + )🤖 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 `@isvreporter/src/isvreporter/main.py` around lines 306 - 322, Update the test catalog upload block around upload_test_catalog to validate that catalog_data contains the required v2 metadata keys schemaVersion, platforms, and suites before uploading. Remove the optimistic fallback defaults for these fields, and fail loudly through the existing error-handling path when any required key is missing.
🧹 Nitpick comments (2)
isvtest/src/isvtest/catalog.py (1)
216-243: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
_declared_platformclassifies differently thansuite_vocabulariesfor a non-declarable platform value.
_build_suite_map(line 233) accepts any non-emptytests.platformstring as the suite name, whilesuite_vocabularies(line 342) only treats it as a capability when it is inDECLARABLE_CAPABILITIESand otherwise falls back to the filename stem. A suite declaringtests.platform: fooinbar.yamlwould emit entries withsuite: "foo"while the envelope advertisesbar— andsuiteis the documented join key between a test run and its catalog rows.isvctl/src/isvctl/config/schema.pyandscripts/validate_suite_wiring.pyboth reject non-declarable platform values, so this is unreachable today; folding the same filter into_declared_platformwould keep the two readers in this file from ever disagreeing.♻️ Proposed change
def _declared_platform(data: dict[str, Any]) -> str | None: - """Return the platform key a suite document declares, or None for a plain suite.""" + """Return the declarable capability a suite document declares, or None for a plain suite.""" tests = data.get("tests") or {} platform = tests.get("platform") if isinstance(tests, dict) else None - return platform if isinstance(platform, str) and platform else None + return platform if isinstance(platform, str) and platform in DECLARABLE_CAPABILITIES else None
suite_vocabulariesthen simplifies toif platform is not None:.Also applies to: 330-346
🤖 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 `@isvtest/src/isvtest/catalog.py` around lines 216 - 243, Update _declared_platform to return a platform only when tests.platform is a non-empty string included in DECLARABLE_CAPABILITIES; otherwise return None so _build_suite_map falls back to the filename stem. Then simplify suite_vocabularies to rely on the filtered platform and only check whether it is not None, keeping both readers’ suite keys consistent.isvctl/src/isvctl/config/suite_resolution.py (1)
43-51: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilently skipping unreadable suite YAML turns a parse error into a confusing "unknown capability".
If
suites/k8s.yamlfails to parse,kubernetesdrops out of the vocabulary andparse_capability("kubernetes", ...)reportsUnknown or non-declarable capability: kubernetesinstead of the actual YAML error. Surfacing the parse failure (as_raw_importson line 90 already does) would keep the diagnosis on the real cause.♻️ Proposed change
platforms: set[str] = set() for path in (configs_root / "suites").glob("*.yaml"): try: data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} - except (OSError, yaml.YAMLError): - continue + except (OSError, yaml.YAMLError) as exc: + raise SuiteResolutionError(f"Failed to read suite {path}: {exc}") from exc platform = (data.get("tests") or {}).get("platform") if isinstance(data, dict) else None🤖 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/src/isvctl/config/suite_resolution.py` around lines 43 - 51, Update the suite-platform discovery loop in the function containing `platforms` and `yaml.safe_load` so YAML parse failures are surfaced rather than silently skipped, matching the error propagation behavior used by `_raw_imports`. Continue handling unreadable files as appropriate, but let `yaml.YAMLError` propagate with its original context so callers report the actual suite YAML failure.
🤖 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/configs/providers/aws/config/observability.yaml`:
- Around line 64-70: Update the requires gates so core-only runs also skip
capability-specific fixtures and cleanup steps. Apply this to launch_host in
isvctl/configs/providers/aws/config/observability.yaml:64-70; instance teardown
in isvctl/configs/providers/aws/config/image-registry.yaml:110-113; host
teardown in isvctl/configs/providers/aws/config/observability.yaml:347-352;
topology-block and capacity cleanup in
isvctl/configs/providers/aws/config/security.yaml:337-343 and 369-372; EKS
setup, block-fixture teardown, and EKS teardown in
isvctl/configs/providers/aws/config/storage.yaml:62-75, 213-228, and 242-249;
instance teardown in
isvctl/configs/providers/my-isv/config/image-registry.yaml:119-122; and cluster
setup, VM-volume teardown, and cluster teardown in
isvctl/configs/providers/my-isv/config/storage.yaml:55-64, 250-265, and 279-282.
Ensure each step’s requires configuration is interpreted when capability is
None, while preserving normal capability filtering.
In `@isvctl/configs/suites/README.md`:
- Around line 14-16: Update the platform suite capability values documented in
the README to use `kubernetes` instead of `k8s`, while retaining `k8s` only as
the suite filename where applicable. Ensure the documented `tests.platform`
value matches capability validation.
In `@isvctl/src/isvctl/cli/test.py`:
- Around line 125-159: The _resolve_capability_context and _reported_capability
helpers must recognize legacy root-level platform configurations in addition to
config.tests.platform. Reuse the same platform-detection behavior as
_detect_platform, including model_extra["platform"], so legacy platform suites
bypass core-only filtering and report their platform capability correctly.
In `@isvctl/src/isvctl/config/schema.py`:
- Around line 394-415: The ValidationConfig.platform description is stale and
lists unsupported capabilities. Update the platform Field description near
validate_suite_shape in isvctl/src/isvctl/config/schema.py to list only
lowercase vm, bare_metal, kubernetes, and slurm, and state that plain suites
omit platform and use per-check requires; then regenerate
isvctl/schemas/config.schema.json so ValidationConfig.platform receives the same
description, without hand-editing the generated schema.
In `@isvctl/src/isvctl/reporting.py`:
- Line 220: Add a brief comment explaining the lazy import of
client_upload_catalog from isvreporter.client inside update_test_run(), or move
it into the existing optional-import block if that preserves behavior. Keep the
import’s current lazy-loading behavior unchanged.
In `@isvctl/tests/test_capability_step_gating.py`:
- Line 34: Update the PROVIDERS definition used by the all-provider gating
invariant to include nico, or replace the hard-coded list with dynamic discovery
of provider configuration directories. Ensure plain nico suites are included
when validating capability-gated references.
In `@isvctl/tests/test_reporting.py`:
- Around line 187-192: Update isvctl/tests/test_reporting.py lines 187-192 in
_config with a concise PEP 257 docstring and annotate raw as dict[str, Any]
instead of bare dict; also add a concise PEP 257 docstring to the method at
lines 208-211.
In `@isvreporter/tests/test_client.py`:
- Around line 97-111: Update the helper methods _posted_payload and _response
with concise PEP 257 docstrings, and add a concise PEP 257 docstring to
test_sends_suite_and_capability. Change _posted_payload’s return annotation from
dict to dict[str, Any], importing Any if needed, while preserving the existing
behavior.
---
Outside diff comments:
In `@isvreporter/src/isvreporter/main.py`:
- Around line 306-322: Update the test catalog upload block around
upload_test_catalog to validate that catalog_data contains the required v2
metadata keys schemaVersion, platforms, and suites before uploading. Remove the
optimistic fallback defaults for these fields, and fail loudly through the
existing error-handling path when any required key is missing.
In `@scripts/validate_suite_wiring.py`:
- Around line 219-226: Update the --check argument help text in the
validate_suite_wiring parser to describe only the validations currently
performed by wiring_errors(), removing references to unregistered suites and
isvreporter platform mismatches.
---
Nitpick comments:
In `@isvctl/src/isvctl/config/suite_resolution.py`:
- Around line 43-51: Update the suite-platform discovery loop in the function
containing `platforms` and `yaml.safe_load` so YAML parse failures are surfaced
rather than silently skipped, matching the error propagation behavior used by
`_raw_imports`. Continue handling unreadable files as appropriate, but let
`yaml.YAMLError` propagate with its original context so callers report the
actual suite YAML failure.
In `@isvtest/src/isvtest/catalog.py`:
- Around line 216-243: Update _declared_platform to return a platform only when
tests.platform is a non-empty string included in DECLARABLE_CAPABILITIES;
otherwise return None so _build_suite_map falls back to the filename stem. Then
simplify suite_vocabularies to rely on the filtered platform and only check
whether it is not None, keeping both readers’ suite keys consistent.
🪄 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: 28248da6-e2fa-40c9-bca4-4ac414e597a1
📒 Files selected for processing (59)
Makefiledocs/guides/configuration.mddocs/guides/external-validation-guide.mddocs/test-plan.yamlisvctl/configs/providers/aws/config/image-registry.yamlisvctl/configs/providers/aws/config/observability.yamlisvctl/configs/providers/aws/config/security.yamlisvctl/configs/providers/aws/config/storage.yamlisvctl/configs/providers/my-isv/config/image-registry.yamlisvctl/configs/providers/my-isv/config/k8s.yamlisvctl/configs/providers/my-isv/config/observability.yamlisvctl/configs/providers/my-isv/config/slurm.yamlisvctl/configs/providers/my-isv/config/storage.yamlisvctl/configs/providers/my-isv/scripts/README.mdisvctl/configs/providers/my-isv/scripts/storage/setup_cluster.pyisvctl/configs/providers/my-isv/scripts/storage/teardown_cluster.pyisvctl/configs/providers/nico/config/control-plane.yamlisvctl/configs/providers/nico/config/iam.yamlisvctl/configs/providers/nico/config/network.yamlisvctl/configs/providers/nico/config/observability.yamlisvctl/configs/suites/README.mdisvctl/configs/suites/control-plane.yamlisvctl/configs/suites/iam.yamlisvctl/configs/suites/image-registry.yamlisvctl/configs/suites/k8s.yamlisvctl/configs/suites/network.yamlisvctl/configs/suites/observability.yamlisvctl/configs/suites/security.yamlisvctl/configs/suites/storage.yamlisvctl/schemas/config.schema.jsonisvctl/src/isvctl/cli/catalog.pyisvctl/src/isvctl/cli/deploy.pyisvctl/src/isvctl/cli/test.pyisvctl/src/isvctl/config/schema.pyisvctl/src/isvctl/config/suite_resolution.pyisvctl/src/isvctl/orchestrator/loop.pyisvctl/src/isvctl/reporting.pyisvctl/tests/providers/nico/test_nico_provider.pyisvctl/tests/test_capability_step_gating.pyisvctl/tests/test_capacity_config.pyisvctl/tests/test_catalog_cli.pyisvctl/tests/test_cli_streams.pyisvctl/tests/test_merger.pyisvctl/tests/test_orchestrator_loop.pyisvctl/tests/test_reporting.pyisvctl/tests/test_schema.pyisvctl/tests/test_suite_resolution.pyisvctl/tests/test_test_cli_labels.pyisvreporter/src/isvreporter/client.pyisvreporter/src/isvreporter/main.pyisvreporter/tests/test_catalog_upload.pyisvreporter/tests/test_client.pyisvtest/src/isvtest/catalog.pyisvtest/src/isvtest/catalog_platforms.pyisvtest/src/isvtest/core/resolution.pyisvtest/tests/test_catalog.pyisvtest/tests/test_resolution.pyscripts/tests/test_validate_suite_wiring.pyscripts/validate_suite_wiring.py
💤 Files with no reviewable changes (7)
- isvctl/configs/providers/nico/config/iam.yaml
- isvtest/src/isvtest/catalog_platforms.py
- isvctl/configs/providers/nico/config/network.yaml
- isvctl/configs/providers/nico/config/control-plane.yaml
- isvctl/configs/providers/nico/config/observability.yaml
- isvctl/configs/providers/my-isv/config/observability.yaml
- isvctl/configs/suites/k8s.yaml
Typer forces a terminal under GITHUB_ACTIONS, so the reported option name arrives spliced with escape codes and the substring match fails in CI while passing locally. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
tests.platform now rejects anything outside vm, bare_metal, kubernetes and slurm, but its field description (and the generated JSON schema) still listed the old vocabulary, and four config examples declared values that raise at load. Plain suites omit the key entirely. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The gating invariant listed aws and my-isv by name, so nico's plain suites escaped it; discovering the config directories keeps a new provider covered. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
The explicit two-capability rule overrode the static pattern rule, so every make invocation printed an overriding-recipe warning. Signed-off-by: Alexandre Begnoche <abegnoche@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
isvctl/schemas/config.schema.json (1)
235-246: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEncode the platform capability allowlist in the schema.
The description restricts platform suites to
vm,bare_metal,kubernetes, andslurm, but the actual schema still accepts any string. Independent JSON-schema consumers can therefore validate unsupported values such asnetworkorgpu. Add the same allowlist as anenum(while preservingnull) so the schema enforces the runtime contract.🤖 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/schemas/config.schema.json` around lines 235 - 246, Update the “platform” schema property to restrict string values to the documented capabilities “vm”, “bare_metal”, “kubernetes”, and “slurm” by adding an enum, while preserving null as an accepted value and retaining the existing default and metadata.
🤖 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.
Outside diff comments:
In `@isvctl/schemas/config.schema.json`:
- Around line 235-246: Update the “platform” schema property to restrict string
values to the documented capabilities “vm”, “bare_metal”, “kubernetes”, and
“slurm” by adding an enum, while preserving null as an accepted value and
retaining the existing default and metadata.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3fe83980-74d3-4edb-8952-bec979090974
📒 Files selected for processing (12)
Makefiledocs/guides/configuration.mddocs/guides/external-validation-guide.mddocs/guides/local-development.mdisvctl/configs/providers/aws/scripts/control-plane/docs/aws-control-plane.mdisvctl/configs/suites/README.mdisvctl/schemas/config.schema.jsonisvctl/src/isvctl/config/schema.pyisvctl/tests/test_capability_step_gating.pyisvctl/tests/test_reporting.pyisvctl/tests/test_test_cli_labels.pyisvreporter/tests/test_client.py
💤 Files with no reviewable changes (4)
- isvctl/configs/providers/aws/scripts/control-plane/docs/aws-control-plane.md
- docs/guides/local-development.md
- docs/guides/external-validation-guide.md
- docs/guides/configuration.md
🚧 Files skipped from review as they are similar to previous changes (7)
- isvreporter/tests/test_client.py
- isvctl/tests/test_capability_step_gating.py
- isvctl/src/isvctl/config/schema.py
- isvctl/tests/test_reporting.py
- isvctl/configs/suites/README.md
- Makefile
- isvctl/tests/test_test_cli_labels.py
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Introduces the suite / capability model for selecting which validations an
ISV runs, replacing the previous per-check
platforms:axis.kubernetes,vm,bare_metal,slurm) declaretests.platform; plain suites (storage,iam,network, …) declare per-checkrequires: [...].requiresis any-match: a check runs when its list is empty (core, runsfor everyone) or names the capability in play. AND across capabilities is
deliberately inexpressible — the four capabilities are mutually exclusive
execution environments, so a check cannot need two at once.
isvctl test run --provider P --suite S [--capability C].--capabilityis single-valued for the same reason.schemaVersion: 2, carryingsuite/requiresper entry and the
platforms/suitesvocabularies at the top level.(suite, capability)pair rather than a single inferredtest_target_type.Docs:
configuration.mdandexternal-validation-guide.mdgain the full model,and the
my-isvscaffold README shows the real invocations.Depends on the service side
The catalog envelope and the run-reporting payload both require the
corresponding ISV Lab Service change (V18: catalog
suite/requires, test-runsuite/capability). That must be deployed first — a v2 catalog uploaded to anolder service will lose the new fields.
Known deferrals
Consciously deferred, so reviewers do not have to guess:
ISVCTL_ENFORCE_UNIQUE_WIRINGgates a global wiring-name uniqueness checkthat is intentionally inert. The check is kept, not deleted, so it can be
switched on in the PR that actually enforces it without rediscovering it.
CORE_REQUIREMENT_CONTEXT = "core"is a CLI-local sentinel meaning "filter tocore only", because
resolve_entries(capability=None)already means "do notfilter". It is translated back to
Nonebefore upload. An explicitthree-state requirement context in
isvtestwould remove the pun.--dry-runrenders its own run/skip decision chain rather than reportingresolve_entries'SkipReason, so it under-reports some terminal skips(
UNRELEASED,EXCLUDED,STEP_NOT_CONFIGURED).Testing
make test(1279 isvtest + isvctl + 113 script tests),make lint,make demo-testend-to-end across allmy-isvsuites, andscripts/validate_suite_wiring.py --checkall pass. Every commit is DCOsigned off.
🤖 Generated with Claude Code
Summary by CodeRabbit
requires, with consistent skip behavior and label include/exclude filtering.isvctldry-runs and reporting now generate/forward suite and capability context; test catalogs use suite/capability requirement metadata (schema v2).requires-based gating, suite vs platform behavior, dry-run planning, and updated demo/testing examples.