Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Pull request overview
This PR adds a new match: smart mode to vyos_config, intended to compute an “enforcing” diff (including delete commands) by comparing structured representations of the candidate and running configuration.
Changes:
- Add
smartto thematchparameter choices forvyos_config, and update examples/docs accordingly. - Implement
diff_match == "smart"in the VyOScliconfplugin using a newVyosConfhelper to generateset/deletecommand diffs. - Add unit tests for the new
VyosConfdiff logic and a newvyos_configtest case formatch=smart, plus a changelog fragment and a.gitignoreupdate.
Reviewed changes
Copilot reviewed 7 out of 10 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
plugins/cliconf/vyos.py |
Adds diff_match="smart" branch to generate delete/set diffs via VyosConf, and exposes smart in supported diff_match values. |
plugins/cliconf_utils/vyosconf.py |
Introduces VyosConf for parsing set/delete commands into a tree and computing structured diffs. |
plugins/modules/vyos_config.py |
Adds smart to the module argument choices and examples; module passes match through as diff_match. |
tests/unit/modules/network/vyos/test_vyos_config.py |
Adds a unit test case for match=smart in the vyos_config module tests. |
tests/unit/cliconf/test_utils_vyosconf.py |
Adds unit tests covering VyosConf parse/build/diff behavior. |
docs/vyos.vyos.vyos_config_module.rst |
Documents smart as a match choice and adds it to an example. |
changelogs/fragments/vyos_config_smart.yml |
Adds a release-note fragment for the new smart match feature. |
.gitignore |
Ignores *.bak files. |
Comments suppressed due to low confidence (1)
docs/vyos.vyos.vyos_config_module.rst:204
- The docs list
smartas a validmatchchoice, but the parameter description doesn’t explain whatsmartdoes or warn about its potential to remove existing configuration. Please expand the description to definesmartbehavior and any prerequisites/risks (e.g., whether it enforces full config vs. only specified subtrees).
<td>
<ul style="margin: 0; padding: 0"><b>Choices:</b>
<li><div style="color: blue"><b>line</b> ←</div></li>
<li>smart</li>
<li>none</li>
</ul>
</td>
<td>
<div>The <code>match</code> argument controls the method used to match against the current active configuration. By default, the desired config is matched against the active config and the deltas are loaded. If the <code>match</code> argument is set to <code>none</code> the active configuration is ignored and the configuration is always loaded.</div>
</td>
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| diff["config_diff"] = list(candidate_commands) | ||
| return diff | ||
| if diff_match == "smart": | ||
| running_conf = VyosConf(running.splitlines()) |
There was a problem hiding this comment.
diff_match == "smart" currently computes a full-tree diff between the entire running config and the (often partial) candidate. With a partial candidate (common for vyos_config), this will generate delete commands for any top-level config not present in the candidate, potentially wiping unrelated configuration. Consider scoping deletions to only the subtrees explicitly managed by the candidate (e.g., by adding a root-level “keep existing” sentinel or otherwise restricting the diff), and document the exact destructive semantics if full replacement is intended.
| running_conf = VyosConf(running.splitlines()) | |
| managed_roots = set() | |
| for line in candidate_commands: | |
| item = str(line).replace("'", "").strip() | |
| if item.startswith("set") or item.startswith("delete"): | |
| parts = item.split() | |
| if len(parts) > 1: | |
| managed_roots.add(parts[1]) | |
| scoped_running = list() | |
| for line in running.splitlines(): | |
| item = str(line).replace("'", "").strip() | |
| if item.startswith("set") or item.startswith("delete"): | |
| parts = item.split() | |
| if len(parts) > 1 and parts[1] in managed_roots: | |
| scoped_running.append(item) | |
| running_conf = VyosConf(scoped_running) |
| self.conn.get_diff = MagicMock( | ||
| return_value=self.cliconf_obj.get_diff(candidate, None, diff_match="none"), | ||
| ) | ||
| self.execute_module(changed=True, commands=lines, sort=False) | ||
|
|
There was a problem hiding this comment.
test_vyos_config_match_smart sets match="smart" but stubs conn.get_diff to call get_diff(..., diff_match="none"), so the new smart diff logic is never exercised. Update the stub to use diff_match="smart" and pass a realistic running config (e.g. self.running_config) so the test actually validates the behavior (including expected deletes/sets).
| self.conn.get_diff = MagicMock( | |
| return_value=self.cliconf_obj.get_diff(candidate, None, diff_match="none"), | |
| ) | |
| self.execute_module(changed=True, commands=lines, sort=False) | |
| diff = self.cliconf_obj.get_diff(candidate, self.running_config, diff_match="smart") | |
| config_diff = diff.get("config_diff", diff) if isinstance(diff, dict) else diff | |
| commands = [command for command in config_diff.splitlines() if command] | |
| self.assertTrue(any(command.startswith("delete ") for command in commands)) | |
| self.assertTrue(any(command.startswith("set ") for command in commands)) | |
| self.conn.get_diff = MagicMock(return_value=diff) | |
| self.execute_module(changed=True, commands=commands, sort=False) |
| line = re.match(r"^('(.*)'|\"(.*)\"|([^#\"']*))*", line).group(0).strip() | ||
| path = re.findall(r"('.*?'|\".*?\"|\S+)", line) | ||
| leaf = path[-1] | ||
| if leaf.startswith('"') and leaf.endswith('"'): | ||
| leaf = leaf[1:-1] | ||
| if leaf.startswith("'") and leaf.endswith("'"): | ||
| leaf = leaf[1:-1] | ||
| return [path[0], path[1:-1], leaf] |
There was a problem hiding this comment.
parse_line() strips quotes from the leaf token but leaves quotes intact on intermediate path tokens. If a quoted token appears in the path (e.g., a named object containing spaces), it will be stored with its surrounding quotes and later quote_key() can double-quote it, producing incorrect commands and diffs. Consider unquoting every parsed token (not just leaf) before storing it in the config tree.
Change Summary
The change modifies vyso_config module to add additional match feature, 'smart', that allows to overwrite the existing configuration by the supplied in the Ansible playboo, i..e this is to enforce the configuration.
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