-
Notifications
You must be signed in to change notification settings - Fork 69
[WIP] added commit-confirm options to vyos_config #229
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
2bb2f53
to
089418f
Compare
089418f
to
2e220db
Compare
@lucaelin, could you rebase/resolve conflicts? |
Manually resolved conflicts, testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds support for VyOS’s commit-confirm
feature to the vyos_config
module, allowing automatic or manual confirmation with a configurable timeout before reverting to the previous configuration.
- Introduce
confirm
andconfirm_timeout
parameters tovyos_config
- Propagate
confirm
toload_config
and updatecommit
in CLIConf to usecommit-confirm
- Add example usage for manual and automatic confirm workflows
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
plugins/modules/vyos_config.py | Added confirm /confirm_timeout parameters and integrated logic |
plugins/module_utils/network/vyos/vyos.py | Extended load_config signature to pass confirm through to CLI |
plugins/cliconf/vyos.py | Updated edit_config and commit to support commit-confirm calls |
Comments suppressed due to low confidence (2)
plugins/modules/vyos_config.py:102
- Parameter docs say
confirm_timeout
is in minutes, but VyOS'scommit-confirm
expects seconds. Either convert minutes to seconds before invoking CLI or clarify the unit in the documentation.
- - Minutes to wait for confirmation before reverting the configuration. Does
plugins/modules/vyos_config.py:337
- New
confirm
andconfirm_timeout
flows aren’t covered by existing tests. Consider adding unit or integration tests to validate manual and automatic confirmation behavior, including timeout and revert logic.
confirm = None
- name: revert after ten minutes, if connection is lost | ||
vyos.vyos.vyos_config: | ||
src: vyos_template.j2 | ||
confirm: yes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example uses confirm: yes
, but valid choices are automatic
, manual
, or none
. Update the example to a supported value (e.g., confirm: manual
).
confirm: yes | |
confirm: manual |
Copilot uses AI. Check for mistakes.
command = 'commit-confirm {0} comment {1}'.format(confirm, comment) | ||
else: | ||
command = 'commit-confirm {0}'.format(confirm) | ||
else: | ||
command = "commit" | ||
self.send_command(command) | ||
if comment: | ||
command = 'commit {0}'.format(comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CLI call omits the comment
keyword and quotes, producing commit foo
instead of commit comment "foo"
. It should be commit comment "<message>"
to handle spaces and match VyOS syntax.
Copilot uses AI. Check for mistakes.
All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
I have read the CLA Document and I hereby sign the CLA |
SUMMARY
I've added an option to the vyos_config module to use
commit-confirm
with a given timeout instead of running a straightcommit
. This functionality is included in vyos and now exposed to the vyos_config module. If no confirmation is received by vyos, it will reboot into the previously existing config, as an attempt to restore functionality.The module can also automatically confirm the configuration if the connection remains working after applying the new config, or not confirm the configuration for you, leaving it up to the user to allow further testing and manual confirmation.
ISSUE TYPE
COMPONENT NAME
commit-confirm
ADDITIONAL INFORMATION
If you accidentally remove the interface configuration your playbook is using to connect to the router, you would previously get a timeout from the module, but the broken config would still remain on the device. This change allows you to specify a timeout, after which the router reboots into the old configuration.
I think tests should be implemented for this feature, but I am not sure how. Can you point me in the right directions here please?