T8989: static_routes module#13
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🧰 Additional context used🔍 Remote MCP Context7Summary of Additional Context for PR ReviewBased on the search results, here are key findings relevant to reviewing this 1. Ansible Module Idempotency Best PracticesThe module implementation should follow standard idempotency patterns documented in Ansible:
2. Check Mode Support RequirementsModules supporting
3. HTTPAPI Plugin ArchitectureFor REST API-based modules like
4. Standard Integration Test PatternNetwork resource module integration tests should:
5. Expected Test CoveragePer Ansible documentation, new modules should be tested:
Recommended Code Review Focus Areas
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds the Changesvyos_static_routes module
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 invalid-task-id label must be absent to mergeWaiting for
This rule is failing.Block merge while the invalid-task-id label is present. Set by the per-repo product T-ID rule (product repos only); dormant where the label is never applied.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
tests/integration/targets/vyos_static_routes/tests/httpapi.yaml (1)
2-21: ⚡ Quick winDeduplicate the HTTPAPI runner to prevent drift.
Line 2–21 duplicates
tests/integration/targets/vyos_static_routes/tasks/httpapi.yaml. Keep one canonical runner and include it here to avoid subtle divergence over time.Proposed refactor
--- -- name: Collect all httpapi test cases - ansible.builtin.find: - paths: "{{ role_path }}/tests/httpapi" - patterns: "{{ testcase }}.yaml" - use_regex: true - register: test_cases - delegate_to: localhost - -- name: Set test_items - ansible.builtin.set_fact: - test_items: "{{ test_cases.files | map(attribute='path') | list }}" - -- name: Run test case (connection=httpapi) - ansible.builtin.include_tasks: "{{ test_case_to_run }}" - vars: - ansible_connection: ansible.netcommon.httpapi - ansible_network_os: vyos.rest.vyos - with_items: "{{ test_items }}" - loop_control: - loop_var: test_case_to_run +- name: Run shared httpapi runner + ansible.builtin.include_tasks: ../tasks/httpapi.yaml🤖 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 `@tests/integration/targets/vyos_static_routes/tests/httpapi.yaml` around lines 2 - 21, The HTTPAPI test runner code in tests/integration/targets/vyos_static_routes/tests/httpapi.yaml (the ansible.builtin.find task for collecting test cases, the ansible.builtin.set_fact task for setting test_items, and the ansible.builtin.include_tasks loop) duplicates the canonical version in tests/integration/targets/vyos_static_routes/tasks/httpapi.yaml. Remove the entire duplicated block and replace it with a single ansible.builtin.include_tasks directive that references the canonical tasks/httpapi.yaml file to maintain a single source of truth and prevent future divergence.tests/integration/targets/vyos_static_routes/tests/httpapi/deleted.yaml (1)
22-26: ⚡ Quick winStrengthen idempotency assertion to verify no commands are produced.
At Line 22-26, the test only checks
result.changed == false. Addresult.commands == []here too, so idempotency also guarantees zero command generation (consistent with merged/replaced scenarios).🤖 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 `@tests/integration/targets/vyos_static_routes/tests/httpapi/deleted.yaml` around lines 22 - 26, The "Assert idempotent" task in the deleted.yaml test only verifies that result.changed equals false, but does not verify that no commands were generated. Add an additional assertion to the that list checking that result.commands equals an empty list to ensure complete idempotency verification, making this assertion consistent with the merged and replaced test scenarios.tests/integration/targets/vyos_static_routes/tests/httpapi/gathered.yaml (1)
14-18: ⚡ Quick winGathered assertions are too broad for regression safety.
At Line 14-18, asserting only “any ipv4 exists” can miss normalization/field regressions. Consider asserting specific expected destinations/next-hop or blackhole fields (and IPv6 if
_populate_config.yamlseeds it).🤖 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 `@tests/integration/targets/vyos_static_routes/tests/httpapi/gathered.yaml` around lines 14 - 18, The gathered.yaml test assertions are too permissive and only verify that at least one IPv4 entry exists in the result.gathered list. Replace or extend these assertions to validate specific field values such as destinations, next-hop addresses, and blackhole settings in the gathered output to catch normalization regressions. Additionally, verify IPv6 entries in the assertions if the _populate_config.yaml test data includes IPv6 static routes. This ensures the test validates actual data correctness, not just the presence of generic entries.plugins/modules/vyos_static_routes.py (2)
324-329: 💤 Low valueConsider validating distance range (1-255).
Documentation states admin distance is 1-255, but
ARGUMENT_SPECdoesn't enforce this. Invalid values would reach the VyOS API.If desired, Ansible supports inline validation via a
typecallback or explicit check inmain(). Otherwise, rely on VyOS API validation.🤖 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 `@plugins/modules/vyos_static_routes.py` around lines 324 - 329, The distance field in the blackhole_config options within ARGUMENT_SPEC does not validate that the value is within the required range of 1-255 as per VyOS documentation. Modify the distance dict entry in blackhole_config to include range validation either by using an Ansible type callback (such as a custom validator) or by adding an explicit validation check in the main() function that verifies the distance value falls within 1-255 before passing it to the VyOS API.
218-218: 💤 Low valueOptional: Use list unpacking for path construction.
Ruff suggests
[*_BASE, _ROUTE_KEY[afi], dest]over concatenation. Applies throughout_route_cmdsandbuild_commands. Purely stylistic—current code is clear.🤖 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 `@plugins/modules/vyos_static_routes.py` at line 218, Replace list concatenation with list unpacking syntax throughout the code. In the `_route_cmds` and `build_commands` functions, change all occurrences of `_BASE + [...]` patterns to use the unpacking syntax `[*_BASE, ...]` instead. This stylistic change makes the code more idiomatic while maintaining identical functionality.Source: Linters/SAST tools
🤖 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 `@plugins/modules/vyos_static_routes.py`:
- Around line 226-229: The condition pattern in the _route_cmds function only
generates commands to set or update optional attributes but does not generate
delete commands when users omit attributes to restore VyOS defaults. In
plugins/modules/vyos_static_routes.py at lines 226-229, add an elif branch after
the existing if statement to check if dist is None and have_dist is not None,
then append a delete command using ("delete", bh_base + ["distance"]). In the
same file at lines 242-245, add a similar elif branch for next-hop distance
checking if dist is None and have_dist is not None, appending ("delete", nh_base
+ ["distance", str(have_dist)]). In the same file at lines 254-257, add an elif
branch for next-hop interface checking if iface is None and have_iface is not
None, appending ("delete", nh_base + ["interface"]).
- Around line 292-306: The replaced state detection at line 302 cannot identify
when next-hop attributes (distance, interface) are being cleared because
_route_cmds only generates commands for non-None values. When clearing an
attribute by setting it to None, _route_cmds produces no command, leaving
test_cmds empty even though the route should be recreated. Enhance the condition
that checks whether to delete and recreate the route to also compare next-hop
attributes directly between want_route and have_route (not just the commands
generated from them). Specifically, examine each next-hop in want_nhs and
have_nhs to detect when attributes like distance or interface differ between the
desired and current states, including cases where attributes are being removed
(transitioning from non-None to None).
In
`@tests/integration/targets/vyos_static_routes/tests/httpapi/_populate_config.yaml`:
- Line 19: Remove the `ignore_errors: true` directive from the populate fixture
configuration. This directive suppresses setup failures that should cause the
test to fail fast, which can hide actual module or API failures and allow
scenario assertions to pass incorrectly. Setup operations in the populate
fixture must not ignore errors so that idempotency testing remains reliable and
failures are caught immediately rather than being masked.
In `@tests/integration/targets/vyos_static_routes/tests/httpapi/replaced.yaml`:
- Around line 20-33: The test for the replaced operation does not actually
verify that the old routes were removed and only the new ones remain after the
replace action. Currently it only checks that the operation triggered a change
and subsequent runs are idempotent, which could pass even if the implementation
incorrectly merges routes instead of replacing them. Add a post-check assertion
after the idempotent assertion that gathers the actual static routes
configuration and validates that only the expected route set is present,
confirming the old routes have been removed. This could be done by adding a
gather or read-back task followed by an assertion comparing the gathered state
against the expected desired routes.
In `@tests/unit/modules/test_vyos_static_routes.py`:
- Around line 397-398: Remove the standalone unittest entrypoint block at the
end of the test_vyos_static_routes.py module. Delete the `if __name__ ==
"__main__":` block that calls `unittest.main()`. This ensures the module only
runs through pytest and pytest-xdist as intended by the collection test
guidelines, rather than allowing direct execution with the unittest runner.
---
Nitpick comments:
In `@plugins/modules/vyos_static_routes.py`:
- Around line 324-329: The distance field in the blackhole_config options within
ARGUMENT_SPEC does not validate that the value is within the required range of
1-255 as per VyOS documentation. Modify the distance dict entry in
blackhole_config to include range validation either by using an Ansible type
callback (such as a custom validator) or by adding an explicit validation check
in the main() function that verifies the distance value falls within 1-255
before passing it to the VyOS API.
- Line 218: Replace list concatenation with list unpacking syntax throughout the
code. In the `_route_cmds` and `build_commands` functions, change all
occurrences of `_BASE + [...]` patterns to use the unpacking syntax `[*_BASE,
...]` instead. This stylistic change makes the code more idiomatic while
maintaining identical functionality.
In `@tests/integration/targets/vyos_static_routes/tests/httpapi.yaml`:
- Around line 2-21: The HTTPAPI test runner code in
tests/integration/targets/vyos_static_routes/tests/httpapi.yaml (the
ansible.builtin.find task for collecting test cases, the
ansible.builtin.set_fact task for setting test_items, and the
ansible.builtin.include_tasks loop) duplicates the canonical version in
tests/integration/targets/vyos_static_routes/tasks/httpapi.yaml. Remove the
entire duplicated block and replace it with a single
ansible.builtin.include_tasks directive that references the canonical
tasks/httpapi.yaml file to maintain a single source of truth and prevent future
divergence.
In `@tests/integration/targets/vyos_static_routes/tests/httpapi/deleted.yaml`:
- Around line 22-26: The "Assert idempotent" task in the deleted.yaml test only
verifies that result.changed equals false, but does not verify that no commands
were generated. Add an additional assertion to the that list checking that
result.commands equals an empty list to ensure complete idempotency
verification, making this assertion consistent with the merged and replaced test
scenarios.
In `@tests/integration/targets/vyos_static_routes/tests/httpapi/gathered.yaml`:
- Around line 14-18: The gathered.yaml test assertions are too permissive and
only verify that at least one IPv4 entry exists in the result.gathered list.
Replace or extend these assertions to validate specific field values such as
destinations, next-hop addresses, and blackhole settings in the gathered output
to catch normalization regressions. Additionally, verify IPv6 entries in the
assertions if the _populate_config.yaml test data includes IPv6 static routes.
This ensures the test validates actual data correctness, not just the presence
of generic entries.
🪄 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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 70efbff2-72ea-45ec-b333-f054d52225dc
📒 Files selected for processing (17)
README.mddocs/vyos.rest.vyos_static_routes_module.rstplugins/modules/vyos_static_routes.pytests/integration/targets/vyos_static_routes/aliasestests/integration/targets/vyos_static_routes/defaults/main.yamltests/integration/targets/vyos_static_routes/tasks/httpapi.yamltests/integration/targets/vyos_static_routes/tasks/main.yamltests/integration/targets/vyos_static_routes/tests/httpapi.yamltests/integration/targets/vyos_static_routes/tests/httpapi/_populate_config.yamltests/integration/targets/vyos_static_routes/tests/httpapi/_remove_config.yamltests/integration/targets/vyos_static_routes/tests/httpapi/deleted.yamltests/integration/targets/vyos_static_routes/tests/httpapi/gathered.yamltests/integration/targets/vyos_static_routes/tests/httpapi/merged.yamltests/integration/targets/vyos_static_routes/tests/httpapi/replaced.yamltests/integration/targets/vyos_static_routes/vars/main.yamltests/unit/fixtures/static_routes_running.jsontests/unit/modules/test_vyos_static_routes.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
ansible/ansible(manual)
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: sanity / sanity-py3.11-2.19
- GitHub Check: unit-source / py3.13 / stable-2.20
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Format Python code using
blackwith line length of 100 characters
Files:
plugins/modules/vyos_static_routes.pytests/unit/modules/test_vyos_static_routes.py
**/test_*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use
pytestwithpytest-xdist(parallel execution with -n 2) for running collection tests
Files:
tests/unit/modules/test_vyos_static_routes.py
🪛 Ruff (0.15.15)
plugins/modules/vyos_static_routes.py
[warning] 218-218: Consider [*_BASE, _ROUTE_KEY[afi], dest] instead of concatenation
Replace with [*_BASE, _ROUTE_KEY[afi], dest]
(RUF005)
[warning] 223-223: Consider [*base, "blackhole"] instead of concatenation
Replace with [*base, "blackhole"]
(RUF005)
[warning] 229-229: Consider [*bh_base, "distance", str(dist)] instead of concatenation
Replace with [*bh_base, "distance", str(dist)]
(RUF005)
[warning] 236-236: Consider [*base, "next-hop", nh_addr] instead of concatenation
Replace with [*base, "next-hop", nh_addr]
(RUF005)
[warning] 245-245: Consider [*nh_base, "distance", str(dist)] instead of concatenation
Replace with [*nh_base, "distance", str(dist)]
(RUF005)
[warning] 250-250: Consider [*nh_base, "disable"] instead of concatenation
Replace with [*nh_base, "disable"]
(RUF005)
[warning] 252-252: Consider [*nh_base, "disable"] instead of concatenation
Replace with [*nh_base, "disable"]
(RUF005)
[warning] 257-257: Consider [*nh_base, "interface", iface] instead of concatenation
Replace with [*nh_base, "interface", iface]
(RUF005)
[warning] 269-269: Consider [*_BASE, route_key] instead of concatenation
Replace with [*_BASE, route_key]
(RUF005)
[warning] 275-275: Consider [*_BASE, route_key, route["dest"]] instead of concatenation
Replace with [*_BASE, route_key, route["dest"]]
(RUF005)
[warning] 287-287: Consider [*_BASE, route_key, dest] instead of concatenation
Replace with [*_BASE, route_key, dest]
(RUF005)
[warning] 303-303: Consider [*_BASE, route_key, dest] instead of concatenation
Replace with [*_BASE, route_key, dest]
(RUF005)
🔍 Remote MCP Context7
Based on my search of available documentation, I can now provide relevant context for reviewing this pull request:
Additional Context for PR Review
VyOS Static Routes Configuration Standards
The VyOS documentation confirms support for IPv4 and IPv6 static routes with next-hop configuration, including administrative distance parameters (allowed range 1-255). Specifically:
- IPv4 unicast routes are configured with:
set protocols static route <subnet> next-hop <address> - IPv6 unicast routes are configured with:
set protocols static route6 <subnet> next-hop <address> - Blackhole routes for IPv4 are configured with:
set protocols static route <subnet> blackhole - Blackhole routes for IPv6 are configured with:
set protocols static route6 <subnet> blackhole - Administrative distance for blackhole routes is configured with:
set protocols static route6 <subnet> blackhole distance <distance>
Validation note: The module's handling of IPv4/IPv6 AFI separation, next-hop configuration, and blackhole distance parameters aligns with VyOS CLI structure as documented.
Ansible Network Module State Handling Patterns
Ansible network resource modules should support the following state values: merged, replaced, overridden, deleted, and gathered. The PR implementation supports all five states:
merged- Ansible merges the on-device configuration with the provided configurationreplaced- Ansible replaces the on-device configuration subsection with the provided configuration subsectionoverridden- Ansible overrides the on-device configuration for the resource with the provided configurationdeleted- Ansible deletes the on-device configuration subsection and restores any default settings- The module also supports
gatheredstate for reading current configuration
Key review point: The module's state handling matches Ansible conventions. Verify in code review that replaced mode properly handles the replace semantics (replacing a specific subsection rather than the entire resource), and that overridden deletes extra routes as expected.
Test Coverage Assessment
The PR includes comprehensive test coverage:
- Integration tests covering all five state transitions (merged, replaced, deleted, gathered) with idempotency verification
- Unit tests validating core logic: config parsing, command generation, and state-specific behavior
- Fixtures for both IPv4 and IPv6 static routes with blackhole and next-hop configurations
Recommended focus areas for code review:
- Verify the
replacedstate correctly implements subsection-level replacement rather than full resource replacement - Validate idempotency of command generation in
_route_cmds()function - Check that the module correctly normalizes running configuration for comparison with desired state
- Ensure error handling for malformed configurations or API failures,
🔇 Additional comments (13)
tests/unit/fixtures/static_routes_running.json (1)
1-21: LGTM!tests/unit/modules/test_vyos_static_routes.py (1)
1-396: LGTM!tests/integration/targets/vyos_static_routes/aliases (1)
1-1: LGTM!tests/integration/targets/vyos_static_routes/defaults/main.yaml (1)
1-3: LGTM!tests/integration/targets/vyos_static_routes/vars/main.yaml (1)
1-1: LGTM!tests/integration/targets/vyos_static_routes/tasks/main.yaml (1)
2-5: LGTM!tests/integration/targets/vyos_static_routes/tasks/httpapi.yaml (1)
2-21: LGTM!tests/integration/targets/vyos_static_routes/tests/httpapi/_remove_config.yaml (1)
2-5: LGTM!tests/integration/targets/vyos_static_routes/tests/httpapi/merged.yaml (1)
7-37: LGTM!plugins/modules/vyos_static_routes.py (4)
1-143: LGTM!
146-189: LGTM!
192-212: LGTM!
353-386: LGTM!
| dist = (want_route["blackhole_config"] or {}).get("distance") | ||
| have_dist = (have_route.get("blackhole_config") or {}).get("distance") | ||
| if dist is not None and dist != have_dist: | ||
| cmds.append(("set", bh_base + ["distance", str(dist)])) |
There was a problem hiding this comment.
_route_cmds cannot clear optional attributes when user omits them.
The pattern if want_val is not None and want_val != have_val only handles setting/updating values, not clearing them. When user omits an optional attribute (distance, admin_distance, interface) to restore VyOS defaults, existing values persist because no delete command is generated.
plugins/modules/vyos_static_routes.py#L226-L229: Addelif dist is None and have_dist is not Nonebranch to delete blackhole distance.plugins/modules/vyos_static_routes.py#L242-L245: Addelif dist is None and have_dist is not Nonebranch to delete next-hop distance.plugins/modules/vyos_static_routes.py#L254-L257: Addelif iface is None and have_iface is not Nonebranch to delete next-hop interface.
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 229-229: Consider [*bh_base, "distance", str(dist)] instead of concatenation
Replace with [*bh_base, "distance", str(dist)]
(RUF005)
📍 Affects 1 file
plugins/modules/vyos_static_routes.py#L226-L229(this comment)plugins/modules/vyos_static_routes.py#L242-L245plugins/modules/vyos_static_routes.py#L254-L257
🤖 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 `@plugins/modules/vyos_static_routes.py` around lines 226 - 229, The condition
pattern in the _route_cmds function only generates commands to set or update
optional attributes but does not generate delete commands when users omit
attributes to restore VyOS defaults. In plugins/modules/vyos_static_routes.py at
lines 226-229, add an elif branch after the existing if statement to check if
dist is None and have_dist is not None, then append a delete command using
("delete", bh_base + ["distance"]). In the same file at lines 242-245, add a
similar elif branch for next-hop distance checking if dist is None and have_dist
is not None, appending ("delete", nh_base + ["distance", str(have_dist)]). In
the same file at lines 254-257, add an elif branch for next-hop interface
checking if iface is None and have_iface is not None, appending ("delete",
nh_base + ["interface"]).
| if state == "replaced" and dest in have_afi: | ||
| # check if anything differs before deleting | ||
| test_cmds = _route_cmds(afi, dest, want_route, have_route) | ||
| want_nhs = set(want_route.get("next_hops") or {}) | ||
| have_nhs = set(have_route.get("next_hops") or {}) | ||
| extra_nhs = have_nhs - want_nhs | ||
| want_bh = want_route.get("blackhole_config") | ||
| have_bh = have_route.get("blackhole_config") | ||
| have_bh_now = have_bh is not None | ||
| want_bh_now = want_bh is not None | ||
| if test_cmds or extra_nhs or have_bh_now != want_bh_now: | ||
| cmds.append(("delete", _BASE + [route_key, dest])) | ||
| have_route = {} | ||
| else: | ||
| continue # already matches — idempotent |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify replaced state behavior is tested with attribute clearing scenarios
rg -n "replaced" tests/integration/targets/vyos_static_routes/tests/httpapi/replaced.yaml -A 20 | head -50Repository: vyos/rest.vyos
Length of output: 1021
🏁 Script executed:
#!/bin/bash
# View the actual code at lines 292-306 with context
sed -n '280,320p' plugins/modules/vyos_static_routes.pyRepository: vyos/rest.vyos
Length of output: 1616
🏁 Script executed:
#!/bin/bash
# Find and examine the _route_cmds function
rg -n "def _route_cmds" plugins/modules/vyos_static_routes.py -A 30Repository: vyos/rest.vyos
Length of output: 1394
🏁 Script executed:
#!/bin/bash
# Check the full replaced.yaml test file to see if it covers attribute clearing
cat tests/integration/targets/vyos_static_routes/tests/httpapi/replaced.yamlRepository: vyos/rest.vyos
Length of output: 942
🏁 Script executed:
#!/bin/bash
# Check if _route_cmds has delete command generation (search full function)
rg -n "def _route_cmds" plugins/modules/vyos_static_routes.py -A 60 | head -80Repository: vyos/rest.vyos
Length of output: 2585
🏁 Script executed:
#!/bin/bash
# Check for delete commands in the module
rg -n '"delete"' plugins/modules/vyos_static_routes.py | head -20Repository: vyos/rest.vyos
Length of output: 420
🏁 Script executed:
#!/bin/bash
# Look for tests that clear/remove attributes in replaced.yaml or related files
find tests -name "*.yaml" -exec grep -l "replaced" {} \; | xargs grep -l "distance" 2>/dev/null || trueRepository: vyos/rest.vyos
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check module documentation or comments for delete attribute examples
rg -i "delete.*distance|distance.*delete" plugins/modules/ -A 2 -B 2Repository: vyos/rest.vyos
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if there are other test scenarios for replaced state
find tests -name "*.yaml" | xargs grep -l "state.*replaced" | xargs wc -l | sort -rn | head -5Repository: vyos/rest.vyos
Length of output: 375
🏁 Script executed:
#!/bin/bash
# Look at the full replaced.yaml to see all test cases
wc -l tests/integration/targets/vyos_static_routes/tests/httpapi/replaced.yamlRepository: vyos/rest.vyos
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check the full replaced.yaml file
cat tests/integration/targets/vyos_static_routes/tests/httpapi/replaced.yaml | head -150Repository: vyos/rest.vyos
Length of output: 942
replaced state cannot detect when next-hop attributes (distance, interface) are cleared.
The replaced detection at line 302 checks test_cmds to decide whether to delete and recreate the route. However, _route_cmds (lines 215–259) only generates commands when attributes have non-None values. Setting distance or interface to None generates no command, so test_cmds remains empty even when the intent is to clear those attributes. Consequently, the replaced condition evaluates to false, and the route is not deleted/recreated.
The integration test in replaced.yaml (35 lines) covers only adding/replacing next-hops, not clearing attributes—this scenario is untested.
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 303-303: Consider [*_BASE, route_key, dest] instead of concatenation
Replace with [*_BASE, route_key, dest]
(RUF005)
🤖 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 `@plugins/modules/vyos_static_routes.py` around lines 292 - 306, The replaced
state detection at line 302 cannot identify when next-hop attributes (distance,
interface) are being cleared because _route_cmds only generates commands for
non-None values. When clearing an attribute by setting it to None, _route_cmds
produces no command, leaving test_cmds empty even though the route should be
recreated. Enhance the condition that checks whether to delete and recreate the
route to also compare next-hop attributes directly between want_route and
have_route (not just the commands generated from them). Specifically, examine
each next-hop in want_nhs and have_nhs to detect when attributes like distance
or interface differ between the desired and current states, including cases
where attributes are being removed (transitioning from non-None to None).
| next_hops: | ||
| - forward_router_address: 2001:db8::1 | ||
| state: merged | ||
| ignore_errors: true |
There was a problem hiding this comment.
Do not suppress setup failures in populate fixture.
Line 19 (ignore_errors: true) can hide module/API failures and make scenario assertions pass for the wrong reason. Setup must fail fast for reliable idempotency testing.
Proposed fix
- name: Populate static_routes config for testing
vyos.rest.vyos_static_routes:
@@
state: merged
- ignore_errors: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ignore_errors: true | |
| - name: Populate static_routes config for testing | |
| vyos.rest.vyos_static_routes: | |
| state: merged |
🤖 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
`@tests/integration/targets/vyos_static_routes/tests/httpapi/_populate_config.yaml`
at line 19, Remove the `ignore_errors: true` directive from the populate fixture
configuration. This directive suppresses setup failures that should cause the
test to fail fast, which can hide actual module or API failures and allow
scenario assertions to pass incorrectly. Setup operations in the populate
fixture must not ignore errors so that idempotency testing remains reliable and
failures are caught immediately rather than being masked.
| - assert: | ||
| that: | ||
| - result.changed == true | ||
|
|
||
| - name: Replace static_routes configuration (IDEMPOTENT) | ||
| register: result | ||
| vyos.rest.vyos_static_routes: *id001 | ||
|
|
||
| - name: Assert idempotent | ||
| assert: | ||
| that: | ||
| - result.changed == false | ||
| - result.commands == [] | ||
|
|
There was a problem hiding this comment.
replaced semantics are not actually verified.
At Line 20-33, the scenario validates change/idempotency but never proves old routes were removed after replace. This can pass even if implementation behaves like merge. Add a post-check (e.g., gathered/read-back assertion) confirming only the declared route set remains.
🤖 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 `@tests/integration/targets/vyos_static_routes/tests/httpapi/replaced.yaml`
around lines 20 - 33, The test for the replaced operation does not actually
verify that the old routes were removed and only the new ones remain after the
replace action. Currently it only checks that the operation triggered a change
and subsequent runs are idempotent, which could pass even if the implementation
incorrectly merges routes instead of replacing them. Add a post-check assertion
after the idempotent assertion that gathers the actual static routes
configuration and validates that only the expected route set is present,
confirming the old routes have been removed. This could be done by adding a
gather or read-back task followed by an assertion comparing the gathered state
against the expected desired routes.
Source: MCP tools
| if __name__ == "__main__": | ||
| unittest.main() |
There was a problem hiding this comment.
Remove the standalone unittest entrypoint in this collection test module.
if __name__ == "__main__": unittest.main() is unnecessary here and encourages running outside the
pytest-xdist path used for collection tests.
Suggested patch
-if __name__ == "__main__":
- unittest.main()As per coding guidelines: **/test_*.py: Use pytest with pytest-xdist (parallel execution with -n 2) for running collection tests.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if __name__ == "__main__": | |
| unittest.main() |
🤖 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 `@tests/unit/modules/test_vyos_static_routes.py` around lines 397 - 398, Remove
the standalone unittest entrypoint block at the end of the
test_vyos_static_routes.py module. Delete the `if __name__ == "__main__":` block
that calls `unittest.main()`. This ensures the module only runs through pytest
and pytest-xdist as intended by the collection test guidelines, rather than
allowing direct execution with the unittest runner.
Source: Coding guidelines
1 participant
Change Summary
Types of changes
Related Task(s)
Related PR(s)
Component(s) name
Proposed changes
How to test
Test results
Tested against VyOS versions:
Checklist:
changelogs/fragmentsto describe the changes