T8989: prefix_lists module#12
Conversation
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdds the Changesvyos_prefix_lists module
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
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
🤖 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 `@changelogs/fragments/t8989_prefix_lists.yml`:
- Around line 1-3: The release note for the vyos_prefix_lists module is
currently classified under the trivial category, but since this is a new
user-facing feature module with tests and documentation, it should be moved to a
feature category. Remove the entry from the trivial section and add it under
minor_changes (or major_changes if this is a breaking change) to properly
reflect that this is a feature addition rather than a trivial infrastructure
change.
In `@docs/vyos.rest.vyos_prefix_lists_module.rst`:
- Around line 37-257: Replace all duplicate `id="parameter-"` anchor IDs with
unique, parameter-specific IDs throughout the documentation table. For each
parameter row, use the parameter name to generate a unique ID following the
pattern `id="parameter-{parameter_name}"`. For example, replace the empty ID in
the config parameter with `id="parameter-config"`, the afi parameter with
`id="parameter-afi"`, the prefix_lists parameter with
`id="parameter-prefix_lists"`, and similarly for all other parameters including
description, entries, action, ge, le, prefix, sequence, and name. This ensures
each anchor is unique and the permalink functionality works correctly for
cross-references.
In `@plugins/modules/vyos_prefix_lists.py`:
- Around line 279-310: The state == "overridden" path only deletes prefix-lists
that are absent from want_afi (lines 279-280) but then applies generic update
logic (lines 282-310) that preserves existing rules and descriptions instead of
replacing them. When state == "overridden", surviving prefix-lists (those in
both have_afi and want_afi) should be fully replaced, not merged. Add a
condition in the loop starting at line 282 that checks if state == "overridden"
and pl_name exists in have_afi, then apply the same deletion-and-rebuild logic
currently used only for state == "replaced" (lines 289-301) to ensure extra
rules and stale descriptions are removed rather than preserved.
- Around line 315-320: The ARGUMENT_SPEC dictionary defines config as a generic
list of dictionaries without specifying the required nested schema, while the
module docstring at lines 22-75 documents the required structure with fields
like afi, name, and sequence. This mismatch causes KeyError exceptions in lines
267, 213, 216, and 269 when the code accesses these keys directly using bracket
notation. Move the documented suboptions schema from the module docstring into
the ARGUMENT_SPEC config parameter definition, converting the documented
structure (with required fields, types, and descriptions) into proper Ansible
argument specification using the suboptions keyword. This will enforce
validation at argument parsing time and provide clear error messages instead of
unhandled exceptions.
In `@tests/unit/modules/test_vyos_prefix_lists.py`:
- Around line 12-20: The sys.path.insert statement on line 12 traverses four
levels up from the test file location, which goes above the repository root and
prevents the import of tests.unit.modules.base on line 20 from working. Reduce
the number of parent directory traversals from four to three in the
sys.path.insert call so that it correctly points to the repository root, making
the relative import paths valid. This will allow pytest to properly collect and
import the test module and its dependencies.
🪄 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: 224a62b5-2aa7-4b3a-8343-15f6e55a23f9
📒 Files selected for processing (9)
README.mdchangelogs/fragments/t8989_prefix_lists.ymldocs/vyos.rest.vyos_prefix_lists_module.rstplugins/modules/vyos_prefix_lists.pytests/integration/targets/vyos_prefix_lists/tests/httpapi/deleted.yamltests/integration/targets/vyos_prefix_lists/tests/httpapi/gathered.yamltests/integration/targets/vyos_prefix_lists/tests/httpapi/merged.yamltests/integration/targets/vyos_prefix_lists/tests/httpapi/replaced.yamltests/unit/modules/test_vyos_prefix_lists.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). (3)
- GitHub Check: sanity / sanity-py3.11-2.19
- 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:
tests/unit/modules/test_vyos_prefix_lists.pyplugins/modules/vyos_prefix_lists.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_prefix_lists.py
🪛 GitHub Actions: Code Coverage / 0_Code Coverage Python 3.10.txt
tests/unit/modules/test_vyos_prefix_lists.py
[error] 20-20: Pytest failed to import test module due to ModuleNotFoundError: No module named 'tests'. Import at tests/unit/modules/test_vyos_prefix_lists.py:20: 'from tests.unit.modules.base import VyOSModuleTestCase, load_fixture'.
🪛 GitHub Actions: Code Coverage / Code Coverage Python 3.10
tests/unit/modules/test_vyos_prefix_lists.py
[error] 20-20: Pytest ImportError while importing test module. ModuleNotFoundError: No module named 'tests'.
🪛 Ruff (0.15.15)
plugins/modules/vyos_prefix_lists.py
[warning] 226-226: Consider [*base, "rule", str(seq)] instead of concatenation
Replace with [*base, "rule", str(seq)]
(RUF005)
[warning] 235-235: Consider [*rbase, api_key, want_rule[field]] instead of concatenation
Replace with [*rbase, api_key, want_rule[field]]
(RUF005)
[warning] 237-237: Consider [*rbase, "ge", str(want_rule["ge"])] instead of concatenation
Replace with [*rbase, "ge", str(want_rule["ge"])]
(RUF005)
[warning] 239-239: Consider [*rbase, "le", str(want_rule["le"])] instead of concatenation
Replace with [*rbase, "le", str(want_rule["le"])]
(RUF005)
[warning] 248-248: Consider [*rbase, api_key, want_rule[field]] instead of concatenation
Replace with [*rbase, api_key, want_rule[field]]
(RUF005)
[warning] 251-251: Consider [*rbase, field, str(want_rule[field])] instead of concatenation
Replace with [*rbase, field, str(want_rule[field])]
(RUF005)
[warning] 306-306: Consider [*base, "description", want_pl["description"]] instead of concatenation
Replace with [*base, "description", want_pl["description"]]
(RUF005)
🔍 Remote MCP Context7
Based on the search results, I've found important contextual information for your PR review. Let me search for more specific details about REST API modules in the vyos collection.## Summary of Relevant Context for PR Review
Based on the additional research, here are key contextual facts relevant to reviewing this PR:
VyOS REST API Module Architecture
VyOS previously used shell session emulation for programmatic access, but has since moved toward HTTP API support. This PR appears to be introducing modules for the newer vyos.rest collection that leverages REST APIs with httpapi connection type, which is a newer approach compared to the existing network_cli modules.
VyOS Prefix Lists Functionality
The prefix lists being implemented have specific VyOS configuration capabilities:
- Prefix lists provide the most powerful prefix-based filtering mechanism, with prefix length range specification in addition to access-list functionality
- Prefix filtering can be done using both
prefix-list(IPv4) andprefix-list6(IPv6) - Key configurable fields include: action (permit/deny), prefix, sequence number, description, and optional
ge(greater than/equal to) andle(less than/equal to) netmask length constraints
Module Testing and Idempotency
The PR includes comprehensive unit tests that validate:
- IPv4 and IPv6 prefix list parsing from VyOS running config
- Normalization of configurations (omitting None values)
- Idempotent behavior for merged, replaced, and deleted states
- Proper command generation for state transitions
Ansible Collection Context
The VyOS collection has been tested against VyOS 1.3.8, 1.4.1, and the current rolling release for 1.5, with compatibility maintained where possible for older versions. The VyOS collection primarily supports network_cli connections, though this PR appears to introduce modules using httpapi instead.
Key Review Considerations
- REST vs CLI: This PR introduces REST API modules (
vyos.restnamespace based on the file paths), which differ from the existingvyos.vyoscollection modules that use CLI/SSH connections - Idempotency: The module implements proper normalization and command generation to ensure idempotent behavior across merged, replaced, and deleted states
- Documentation: Documentation is comprehensive, including parameters, operational examples, and return values following Ansible standards
- Test Coverage: Unit tests cover parsing, normalization, and command generation scenarios
[::Web Search::],
🔇 Additional comments (1)
README.md (1)
83-86: LGTM!
| --- | ||
| trivial: | ||
| - vyos_prefix_lists - Added new module and tests and documentation. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check changelog guidelines and structure
find . -name "*.md" -o -name "*.rst" | grep -i changelog | head -20Repository: vyos/rest.vyos
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look at changelog examples to understand category usage
find ./changelogs -type f | head -20Repository: vyos/rest.vyos
Length of output: 229
🏁 Script executed:
#!/bin/bash
# Check if vyos_prefix_lists module exists
find . -name "*prefix_list*" -type fRepository: vyos/rest.vyos
Length of output: 284
🏁 Script executed:
#!/bin/bash
# Check the actual changelog fragment content
cat changelogs/fragments/t8989_prefix_lists.ymlRepository: vyos/rest.vyos
Length of output: 141
🏁 Script executed:
#!/bin/bash
# Check changelog configuration for category definitions
cat changelogs/config.yamlRepository: vyos/rest.vyos
Length of output: 878
🏁 Script executed:
#!/bin/bash
# Look at other changelog fragments to understand category patterns
cat changelogs/fragments/*.yml | head -50Repository: vyos/rest.vyos
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Check if there's documentation on changelog categories
find . -type f \( -name "CHANGELOG*" -o -name "CHANGES*" -o -name "CONTRIBUTING*" \) | head -10Repository: vyos/rest.vyos
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the CHANGELOG to see examples of what goes into trivial vs feature categories
head -100 CHANGELOG.rstRepository: vyos/rest.vyos
Length of output: 131
🏁 Script executed:
#!/bin/bash
# Check if there's a README or contribution guide that explains changelog categories
cat README.md 2>/dev/null | head -50 || cat CONTRIBUTING.md 2>/dev/null | head -50 || echo "No README/CONTRIBUTING found"Repository: vyos/rest.vyos
Length of output: 1163
🏁 Script executed:
#!/bin/bash
# Look at git history to see what types of entries go into trivial
cd changelogs && ls -laRepository: vyos/rest.vyos
Length of output: 353
Move this release note out of trivial.
A new module with tests and documentation is a feature addition and should be classified as minor_changes or major_changes, not trivial. The trivial category is appropriate only for infrastructure changes (like the tox configuration entry in the same fragment directory), not user-facing features.
🤖 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 `@changelogs/fragments/t8989_prefix_lists.yml` around lines 1 - 3, The release
note for the vyos_prefix_lists module is currently classified under the trivial
category, but since this is a new user-facing feature module with tests and
documentation, it should be moved to a feature category. Remove the entry from
the trivial section and add it under minor_changes (or major_changes if this is
a breaking change) to properly reflect that this is a feature addition rather
than a trivial infrastructure change.
| <tr> | ||
| <td colspan="4"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>config</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">list</span> | ||
| / <span style="color: purple">elements=dictionary</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>List of prefix-list configurations grouped by address family.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="3"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>afi</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">string</span> | ||
| / <span style="color: red">required</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| <ul style="margin: 0; padding: 0"><b>Choices:</b> | ||
| <li>ipv4</li> | ||
| <li>ipv6</li> | ||
| </ul> | ||
| </td> | ||
| <td> | ||
| <div>Address family identifier.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="3"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>prefix_lists</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">list</span> | ||
| / <span style="color: purple">elements=dictionary</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>Named prefix lists.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="2"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>description</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">string</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>Prefix list description.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="2"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>entries</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">list</span> | ||
| / <span style="color: purple">elements=dictionary</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>Prefix list rules.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="1"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>action</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">string</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| <ul style="margin: 0; padding: 0"><b>Choices:</b> | ||
| <li>permit</li> | ||
| <li>deny</li> | ||
| </ul> | ||
| </td> | ||
| <td> | ||
| <div>Permit or deny.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="1"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>description</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">string</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>Rule description.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="1"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>ge</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">integer</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>Minimum prefix length.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="1"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>le</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">integer</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>Maximum prefix length.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="1"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>prefix</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">string</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>Network prefix to match.</div> | ||
| </td> | ||
| </tr> | ||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="1"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>sequence</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">integer</span> | ||
| / <span style="color: red">required</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>Rule sequence number.</div> | ||
| </td> | ||
| </tr> | ||
|
|
||
| <tr> | ||
| <td class="elbow-placeholder"></td> | ||
| <td class="elbow-placeholder"></td> | ||
| <td colspan="2"> | ||
| <div class="ansibleOptionAnchor" id="parameter-"></div> | ||
| <b>name</b> | ||
| <a class="ansibleOptionLink" href="#parameter-" title="Permalink to this option"></a> | ||
| <div style="font-size: small"> | ||
| <span style="color: purple">string</span> | ||
| / <span style="color: red">required</span> | ||
| </div> | ||
| </td> | ||
| <td> | ||
| </td> | ||
| <td> | ||
| <div>Prefix list name.</div> | ||
| </td> | ||
| </tr> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check file size and examine the anchor IDs in both sections
wc -l docs/vyos.rest.vyos_prefix_lists_module.rst
echo "---"
# View the entire parameter and return-values sections
sed -n '37,257p' docs/vyos.rest.vyos_prefix_lists_module.rst | grep -E 'id="(parameter|return)-"' | head -20
echo "---"
# View return-values section
sed -n '353,442p' docs/vyos.rest.vyos_prefix_lists_module.rst | grep -E 'id="(parameter|return)-"' | head -20Repository: vyos/rest.vyos
Length of output: 1464
Use unique anchor IDs in parameter and return-values tables.
The id="parameter-" anchors in lines 37-257 and id="return-" anchors in lines 353-442 are duplicated across all rows. This breaks permalink functionality—all option links in a section target the same anchor instead of their respective parameters. Emit option-specific IDs to fix documentation cross-references.
🤖 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 `@docs/vyos.rest.vyos_prefix_lists_module.rst` around lines 37 - 257, Replace
all duplicate `id="parameter-"` anchor IDs with unique, parameter-specific IDs
throughout the documentation table. For each parameter row, use the parameter
name to generate a unique ID following the pattern
`id="parameter-{parameter_name}"`. For example, replace the empty ID in the
config parameter with `id="parameter-config"`, the afi parameter with
`id="parameter-afi"`, the prefix_lists parameter with
`id="parameter-prefix_lists"`, and similarly for all other parameters including
description, entries, action, ge, le, prefix, sequence, and name. This ensures
each anchor is unique and the permalink functionality works correctly for
cross-references.
| if state == "overridden": | ||
| for pl_name in set(have_afi) - set(want_afi): | ||
| cmds.append(("delete", ["policy", api_key, pl_name])) | ||
|
|
||
| for pl_name, want_pl in want_afi.items(): | ||
| have_pl = have_afi.get(pl_name, {}) | ||
| base = ["policy", api_key, pl_name] | ||
| want_rules = want_pl.get("rules", {}) | ||
| have_rules = have_pl.get("rules", {}) | ||
|
|
||
| if state == "replaced" and pl_name in have_afi: | ||
| # Only delete and rebuild if something actually differs | ||
| want_desc = want_pl.get("description") | ||
| have_desc = have_pl.get("description") | ||
| extra_seqs = set(have_rules.keys()) - set(want_rules.keys()) | ||
| test_cmds = [] | ||
| for seq, want_rule in want_rules.items(): | ||
| test_cmds += _rule_cmds(base, seq, want_rule, have_rules, state) | ||
| if test_cmds or extra_seqs or want_desc != have_desc: | ||
| cmds.append(("delete", ["policy", api_key, pl_name])) | ||
| have_pl = {} | ||
| have_rules = {} | ||
| else: | ||
| continue # already matches — idempotent | ||
|
|
||
| # Description | ||
| if want_pl.get("description") and want_pl["description"] != have_pl.get("description"): | ||
| cmds.append(("set", base + ["description", want_pl["description"]])) | ||
|
|
||
| # Rules | ||
| for seq, want_rule in want_rules.items(): | ||
| cmds += _rule_cmds(base, seq, want_rule, have_rules, state) |
There was a problem hiding this comment.
state: overridden still merges surviving prefix-lists.
Line 279 only deletes list names absent from want_afi. For names that remain, Lines 289-310 never remove extra rules or stale descriptions, so overridden behaves like merged for those lists instead of replacing them.
Suggested fix
- if state == "replaced" and pl_name in have_afi:
+ if state in {"replaced", "overridden"} and pl_name in have_afi:
# Only delete and rebuild if something actually differs
want_desc = want_pl.get("description")
have_desc = have_pl.get("description")
extra_seqs = set(have_rules.keys()) - set(want_rules.keys())🧰 Tools
🪛 Ruff (0.15.15)
[warning] 306-306: Consider [*base, "description", want_pl["description"]] instead of concatenation
Replace with [*base, "description", want_pl["description"]]
(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_prefix_lists.py` around lines 279 - 310, The state ==
"overridden" path only deletes prefix-lists that are absent from want_afi (lines
279-280) but then applies generic update logic (lines 282-310) that preserves
existing rules and descriptions instead of replacing them. When state ==
"overridden", surviving prefix-lists (those in both have_afi and want_afi)
should be fully replaced, not merged. Add a condition in the loop starting at
line 282 that checks if state == "overridden" and pl_name exists in have_afi,
then apply the same deletion-and-rebuild logic currently used only for state ==
"replaced" (lines 289-301) to ensure extra rules and stale descriptions are
removed rather than preserved.
| ARGUMENT_SPEC = dict( | ||
| config=dict(type="list", elements="dict"), | ||
| state=dict( | ||
| default="merged", | ||
| choices=["merged", "replaced", "overridden", "deleted", "gathered"], | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, locate the file and check its size
find . -name "vyos_prefix_lists.py" -type f
wc -l plugins/modules/vyos_prefix_lists.py 2>/dev/null || echo "File not found at that path"Repository: vyos/rest.vyos
Length of output: 138
🏁 Script executed:
#!/bin/bash
# Read the relevant sections of the file
# Lines 315-320 (ARGUMENT_SPEC)
echo "=== Lines 315-320 (ARGUMENT_SPEC) ==="
sed -n '315,320p' plugins/modules/vyos_prefix_lists.py
# Lines 212, 215, 266 (dictionary accesses)
echo -e "\n=== Line 212 ==="
sed -n '210,215p' plugins/modules/vyos_prefix_lists.py
echo -e "\n=== Line 215 ==="
sed -n '213,218p' plugins/modules/vyos_prefix_lists.py
echo -e "\n=== Line 266 ==="
sed -n '264,270p' plugins/modules/vyos_prefix_lists.pyRepository: vyos/rest.vyos
Length of output: 1117
🏁 Script executed:
#!/bin/bash
# Search for validation logic and imports
echo "=== VyOSModule import ==="
grep -n "VyOSModule\|from\|import" plugins/modules/vyos_prefix_lists.py | head -20
echo -e "\n=== Full ARGUMENT_SPEC definition ==="
sed -n '315,350p' plugins/modules/vyos_prefix_lists.pyRepository: vyos/rest.vyos
Length of output: 1347
🏁 Script executed:
#!/bin/bash
# Check for any validation on config items
echo "=== Lines around 212, 215, 266 with context ==="
sed -n '200,220p' plugins/modules/vyos_prefix_lists.py
echo "---"
sed -n '260,275p' plugins/modules/vyos_prefix_lists.py
# Search for "config" usage and any validation
echo -e "\n=== Search for config validation or normalization ==="
grep -n "config\|validate\|fail_json" plugins/modules/vyos_prefix_lists.py | head -30Repository: vyos/rest.vyos
Length of output: 2887
🏁 Script executed:
#!/bin/bash
# Check the module documentation more thoroughly
echo "=== Documentation section (lines 20-100) ==="
sed -n '20,100p' plugins/modules/vyos_prefix_lists.py
# Look for VyOSModule implementation
echo -e "\n=== Search for VyOSModule file ==="
find . -path "*module_utils*vyos*" -name "*.py" | head -5Repository: vyos/rest.vyos
Length of output: 2686
🏁 Script executed:
#!/bin/bash
# Look at the documented config structure in YAML documentation
sed -n '22,75p' plugins/modules/vyos_prefix_lists.pyRepository: vyos/rest.vyos
Length of output: 1747
Define config schema in ARGUMENT_SPEC to enforce validation before code execution.
The documented schema (lines 22–75) specifies required fields (afi, name, sequence), but ARGUMENT_SPEC at line 316 only declares config=dict(type="list", elements="dict") without nested structure. Lines 267, 213, 216, and 269 access these keys directly with [], causing KeyError if playbook data is malformed. Move the documented suboptions from the module docstring into ARGUMENT_SPEC to enforce validation through Ansible's argument validator, ensuring clean error messages instead of unhandled exceptions.
🤖 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_prefix_lists.py` around lines 315 - 320, The
ARGUMENT_SPEC dictionary defines config as a generic list of dictionaries
without specifying the required nested schema, while the module docstring at
lines 22-75 documents the required structure with fields like afi, name, and
sequence. This mismatch causes KeyError exceptions in lines 267, 213, 216, and
269 when the code accesses these keys directly using bracket notation. Move the
documented suboptions schema from the module docstring into the ARGUMENT_SPEC
config parameter definition, converting the documented structure (with required
fields, types, and descriptions) into proper Ansible argument specification
using the suboptions keyword. This will enforce validation at argument parsing
time and provide clear error messages instead of unhandled exceptions.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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)
tests/unit/modules/test_vyos_prefix_lists.py (1)
217-217:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFragile assertion: substring search can match unintended paths.
The check
"deny" in c[1]searches for substring"deny"anywhere in the command path list. A more robust approach would verify the specific path element, e.g.,c[1] == ["policy", "prefix-list", "PL1", "rule", "10", "action", "deny"]or checkc[1][-1] == "deny"if the last element is always the action value.🔍 Proposed fix
- set_idx = next(i for i, c in enumerate(ops) if c[0] == "set" and "deny" in c[1]) + set_idx = next(i for i, c in enumerate(ops) if c[0] == "set" and c[1][-1] == "deny")🤖 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_prefix_lists.py` at line 217, The substring search `"deny" in c[1]` in the assertion at line 217 is fragile because it matches "deny" anywhere within the command path list element, not just as the action value. Replace this fragile check with a more robust approach that verifies the specific path element—either by checking the last element of the path with `c[1][-1] == "deny"` if the action is always the final element, or by comparing against the complete expected path. This ensures the assertion matches the intended operation rather than accidentally matching any path containing the substring "deny".
🧹 Nitpick comments (1)
tests/unit/modules/test_vyos_prefix_lists.py (1)
129-219: ⚡ Quick winConsider adding tests for
overriddenandgatheredstates.The test suite covers
merged,deleted, andreplacedstates but omitsoverriddenandgathered. Adding tests for these states would improve coverage and validate edge cases like deleting unmentioned prefix-lists (overridden) and read-only config retrieval (gathered).🤖 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_prefix_lists.py` around lines 129 - 219, Add test methods to the TestVyOSPrefixListsBuildCommands class to cover the `overridden` and `gathered` states. For the `overridden` state, create a test method like test_overridden that verifies unmentioned prefix-lists are deleted while specified ones replace existing configurations. For the `gathered` state, create a test method like test_gathered that validates read-only config retrieval returns the current state without applying any changes. Reuse the helper methods _have_empty() and _have_with_ipv4_pl() as needed to provide test data and call build_commands with the appropriate state parameter to validate the expected behavior.
🤖 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 `@tests/unit/modules/test_vyos_prefix_lists.py`:
- Line 217: The substring search `"deny" in c[1]` in the assertion at line 217
is fragile because it matches "deny" anywhere within the command path list
element, not just as the action value. Replace this fragile check with a more
robust approach that verifies the specific path element—either by checking the
last element of the path with `c[1][-1] == "deny"` if the action is always the
final element, or by comparing against the complete expected path. This ensures
the assertion matches the intended operation rather than accidentally matching
any path containing the substring "deny".
---
Nitpick comments:
In `@tests/unit/modules/test_vyos_prefix_lists.py`:
- Around line 129-219: Add test methods to the TestVyOSPrefixListsBuildCommands
class to cover the `overridden` and `gathered` states. For the `overridden`
state, create a test method like test_overridden that verifies unmentioned
prefix-lists are deleted while specified ones replace existing configurations.
For the `gathered` state, create a test method like test_gathered that validates
read-only config retrieval returns the current state without applying any
changes. Reuse the helper methods _have_empty() and _have_with_ipv4_pl() as
needed to provide test data and call build_commands with the appropriate state
parameter to validate the expected behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e6d1f4f6-594c-4b0f-bcbd-58bbd3245fe3
📒 Files selected for processing (2)
tests/unit/fixtures/prefix_lists_running.jsontests/unit/modules/test_vyos_prefix_lists.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). (2)
- 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:
tests/unit/modules/test_vyos_prefix_lists.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_prefix_lists.py
🔍 Remote MCP Context7
Perfect. Now I have sufficient concrete information to provide a comprehensive summary. Let me compile the findings.
Additional Relevant Context for PR Review
Based on targeted research, here are the concrete facts relevant to reviewing this pull request:
Ansible Network Resource Module Standards
State Management Requirements:
According to Ansible's network resource module development guidelines, modules maintaining the vyos.rest collection should support the following state values:
- merged: Combines provided configuration with existing device settings
- replaced: Replaces specific configuration subsections (with caution to avoid disrupting device access)
- overridden: Replaces entire resource configuration
- deleted: Removes configuration subsections and restores defaults
- gathered: Retrieves current resource details from the device (read-only, no changes)
- rendered and parsed: Device-native rendering and parsing (optional, offline-capable)
The module implementation includes all required states (merged, replaced, deleted, gathered), which aligns with Ansible standards.
HTTP API Connection Pattern:
The PR correctly uses connection: httpapi as the execution mode for REST-based modules, following Ansible's httpapi plugin patterns.
VyOS REST API and Prefix Lists Configuration
REST API Endpoint Availability:
VyOS provides a REST API accessible via HTTPS with the following authentication mechanism:
- API key-based authentication (configured via
set service https api keys id <name> key <apikey>) - Endpoints require form-based data submission with JSON payloads
VyOS Prefix List Configuration Structure:
The VyOS policy prefix-list configuration hierarchy supports:
- IPv4 prefix-lists (
policy prefix-list <name>) - IPv6 prefix-lists (
policy prefix-list6 <name>) - Rule attributes per rule:
action(permit/deny),prefix(IPv4/IPv6 CIDR),description(optional),ge(greater-than-equal prefix length, 0-32 for IPv4, 0-128 for IPv6),le(less-than-equal prefix length)
The module's handling of ge/le normalization (filtering out None values, as shown in unit tests) aligns with VyOS's optional range constraints.
Code Review Observations
Key Strengths:
- Comprehensive unit test coverage across configuration parsing, normalization, and command generation
- Idempotent behavior validated for state transitions (merged, replaced, deleted)
- Proper handling of IPv4 and IPv6 configurations separately
- Documentation follows Ansible standards with parameters, examples, and return values
Areas to Verify During Review:
- Verify the module correctly handles the VyOS REST API response structure when retrieving running prefix-list configuration
- Confirm the
build_commandslogic properly generatesset policy prefix-list[6] <name> rule <seq> ...syntax for all state transitions - Validate that the
commitandsaveoperations are handled (or documented as device responsibility) after configuration changes - Test compatibility with multiple VyOS versions (given VyOS has version-specific API differences between branches)
🔇 Additional comments (4)
tests/unit/modules/test_vyos_prefix_lists.py (3)
7-34: LGTM!
36-81: LGTM!
84-127: LGTM!tests/unit/fixtures/prefix_lists_running.json (1)
1-42: LGTM!
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