Conversation
Add --device-filter CLI option to filter devices during Direct-to-Device (D2D) testing: - Implement device filter expression parser supporting equality (=), regex (=~), negation (!=), and nested paths in nac_test.utils.device_filter - Add --device-filter argument to Typer CLI with custom syntax validator - Serialize filters via NAC_TEST_DEVICE_FILTER_JSON environment variable - Support filtering in NACTestBase via filter_devices() and device_filters property - Forward filter diagnostics from pyATS resolver to orchestrator and log summary warnings - Warn when --device-filter is supplied without active D2D/API pyATS tests - Add comprehensive unit, integration, and E2E test coverage - Add temporary git dependency for nac-test-pyats-common branch feat/60-device-filter
Draft
12 tasks
…#932) PR review follow-ups on the --device-filter implementation. Documentation corrections — the README described an API that did not exist: - Operator table listed `==` and `!~`. Neither works: `!~` raises "missing operator", and `role==spine` parses as value "=spine", silently matching nothing. Table now lists the four real operators (=, !=, =~, !=~). - Env var was documented as "comma or whitespace separated". Click splits multiple-value env vars on whitespace only, so a comma-separated value parses as a single filter and silently matches nothing. - Claims of int/float/bool/None coercion and case-insensitive boolean literals were unimplemented. Replaced with a String Comparison bullet describing the actual semantics, plus a Missing Fields bullet documenting the positive/negative operator asymmetry. Behaviour changes: - Drop the "Available fields" listing from the unknown-field error, along with extract_available_keys() and FilterResult.keys_seen. Key discovery only walked two levels while _resolve_path traverses arbitrary depth, so the hint was misleading on nested data models. - filter_devices() no longer fails open. Previously, if every filter referenced an unknown field it returned the full unfiltered device list, so a typo could widen the device set. Unknown fields now raise when strict=True and are warned-but-still-applied when strict=False. - Parse the operator with a leftmost-longest regex derived from VALID_OPERATORS instead of a substring-anywhere scan, so values containing operator characters no longer steal the split point (e.g. 'hostname=a=~b' -> field 'hostname', value 'a=~b'). - Replace print() with typer.echo() for the filter summary line. Tests: - e2e credential injection is scoped to requires_testbed scenarios again, restoring isolation for non-D2D scenarios, while still covering every mock device so filtered-out devices resolve during testbed generation. - test_excluded_device_absent_from_reports asserted against a summary.html that is never produced, guarded by `if exists()` so it always passed vacuously. Now asserts on combined_summary.html and xunit.xml. - Add parse regression cases for operator characters inside values. Noted for follow-up: TODO(#932) on the exit-code semantics of the two early returns (both yield an empty PyATSResults, so "filter matched no devices" is indistinguishable from "nothing to run"), and on the shape of the filter_devices/get_device_filters public API, which has no in-tree callers yet.
--device-filter had two failure modes collapsed into one outcome: both an unknown filter field and a valid filter matching no devices returned an empty PyATSResults(), making a typo indistinguishable from a clean run. Separate them: - Unknown field (absent from every device) is a filter definition error. The user's intent is unknowable, so raise DeviceFilterError and abort before anything executes. The CLI maps it to EXIT_INVALID_ARGS (2), the same code as a malformed expression caught by the --device-filter callback, so both filter failures report identically. - Zero matches is a legitimate outcome. Warn on the console and skip D2D, exactly as for an empty inventory, letting API/Robot tests run and determine the exit code. Device inventory resolution now precedes task assembly. Aborting after the API coroutine was created left it un-awaited, silently cancelling the API suite; the unknown-field test asserts this ordering. run_tests() re-raises DeviceFilterError ahead of the generic handler, which would otherwise bury it in an api-slot error result (exit 255). Documents both behaviours in the README, including the previously undocumented no-matching-devices case.
base_test exposed filter_devices(), get_device_filters() and a device_filters property so test authors could apply the active --device-filter to devices retrieved from a controller API. Remove them: the API cannot be made to work as intended. The filter's namespace is three canonical aliases (hostname, ip, os) overlaid on the raw data model device dict, so a user may filter on any dotted path in their model -- bgp.asn, site, tags, and so on. Controller payloads use a different schema. For the helper to be useful, a test author would have to translate controller fields into data model field names for a set of names that is open-ended and chosen by the user at runtime. Passing controller dicts through unmodified instead yields either a DeviceFilterError or a silently empty device list, so the signature promised something the semantics could not deliver. Nothing is lost: the helpers had no callers and no tests. Exposing the parsed filter spec instead was considered and rejected, as authors would then reimplement the matching rules (missing fields versus negative operators, boolean lowercase normalisation, null-as-absent, string coercion, re.search semantics) and produce API tests that silently disagree with D2D. The underlying requirement is real and tracked separately: scope an API test to the device set D2D resolved, by intersecting on a stable key such as hostname, rather than by re-evaluating the filter against a foreign schema. Document the filter's scope in the README, which previously left it implicit.
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.
Description
Introduces the
--device-filterCLI option to filter devices during Direct-to-Device (D2D) testing across pyATS test execution. It adds a versatile filter engine innac_test.utils.device_filter, CLI validation, orchestrator coordination, and resolver integration via environment serialization (NAC_TEST_DEVICE_FILTER_JSON). The filter scopes D2D device selection only; API and Robot Framework tests are unaffected by it.Closes
Type of Change
Test Framework Affected
Network as Code (NaC) Architecture Affected
Platform Tested
Key Capabilities
Flexible Filter Expressions:
hostname=leaf1,role=spinehostname=~^sd-dc-.*,role=~(spine|leaf)role!=spine,site!=labbgp.asn=65001--device-filterCLI options evaluated as a logical AND (all criteria must match)Full Pipeline Integration:
validate_device_filter()rejecting malformed expressions before test execution.NAC_TEST_DEVICE_FILTER_JSONfor cross-process and subprocess communication.BaseDeviceResolver(innac-test-pyats-common) to filter inventory prior to SSH credential injection and device dictionary creation.User Experience & Diagnostics:
--device-filteris passed without active D2D/API pyATS tests.Key Design Decisions
Canonical Attributes vs. Raw Data Model Attributes (
ChainMap):mgmt_ip,system_ip,ipAddress).BaseDeviceResolverlazily extracts canonical attributes (hostname,ip,os) and overlays them onto the raw device dictionary usingcollections.ChainMap.hostname=foo,ip=10.1.1.1) as well as arbitrary raw model keys (tags=production,role=spine,bgp.asn=65001).Authoritative Filter Engine:
nac_test.utils.device_filter.nac-test-pyats-commondelegates filter evaluation to this engine, ensuring uniform semantics and backwards compatibility.Two Distinct Failure Modes, Deliberately Treated Differently:
DeviceFilterErrorbefore anything executes and the CLI exits withEXIT_INVALID_ARGS(2) — the same code a malformed expression gets from the--device-filtercallback. Both filter failures therefore surface identically to the user and to CI.0, matching pre-existing empty-inventory behaviour rather than inventing a new rule for this feature.Validation Split: Syntax at the CLI, Semantics at the Orchestrator:
--render-onlyrun.Inventory Resolution Precedes Task Assembly:
RuntimeWarning. The ordering is now an invariant covered by a dedicated regression test.No In-Test Filter API (helpers removed before merge):
NACTestBase.filter_devices(),get_device_filters()and adevice_filtersproperty so an API test could re-apply the active filter to devices fetched from a controller. These were removed.hostname,ip,os) overlaid on the raw data model device dict, so users may filter on any dotted path in their model. Controller payloads use a different schema, so the helper would require a controller-field-to-data-model-field translation for a set of names that is open-ended and chosen by the user at runtime. Passing controller dicts straight through yields either aDeviceFilterErroror a silently empty list — the signature promised what the semantics could not deliver. The helpers had no callers and no tests.null-as-absent, string coercion,re.searchvsfullmatch) and produce API tests that silently disagree with D2D.Temporary Lock-Step Dependency:
pyproject.tomltemporarily referencesnac-test-pyats-commonon branchfeat/60-device-filtervia[tool.uv.sources]so that E2E test suites run against the updated resolver before package release.Testing Done
tests/unit/utils/test_device_filter.py)tests/unit/pyats_core/test_orchestrator_device_filter.py)tests/integration/test_cli_device_filter.py)PYATS_D2D_ONLY_SCENARIOandDEVICE_FILTER_TAG_SCENARIOverifying filtering on real testbed generation and HTML report output--device-filtersection toREADME.mdexplaining canonical attributes, operators, case sensitivity, and syntax examples.pytest/pre-commit run -a)Test Commands Used
.venv/bin/pytest -n auto --dist loadscope tests/unit/utils/test_device_filter.py tests/unit/pyats_core/test_orchestrator_device_filter.py tests/integration/test_cli_device_filter.py tests/e2e/test_e2e_scenarios.py -k "device_filter or d2d" .venv/bin/pre-commit run --all-filesChecklist
pre-commit run -apasses)Additional Notes
nac-test-pyats-commonwith support forBaseDeviceResolverfiltering (netascode/nac-test-pyats-common#61, PR to be merged).uv.sourcesgit pointer will be replaced withnac-test-pyats-common>=0.4.0b3.nac-test-pyats-commonto branchfeat/60-device-filter, and that PR's CI in turn installsnac-testfrom branchfeat/932-device-filter-d2d. The pins are symmetric, so whichever merges first will redden the other until its ref is retargeted. The second PR needs a retarget commit before it can go green. Both pins are taggedTEMPORARY - REMOVE BEFORE RELEASE.