feat(T7206): Fix vyos default config path#89
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 refactors VyOS configuration file handling by introducing a shared utility module to standardize config file path resolution and saving operations. It addresses an issue with wrong default config paths in cloud-init for VyOS systems.
- Extracts common VyOS configuration file logic into a dedicated
vyos_util.pymodule - Centralizes config file path resolution with fallback to multiple default locations
- Standardizes configuration saving across VyOS-related modules
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
cloudinit/config/vyos_util.py |
New utility module providing centralized VyOS config file path resolution and saving functions |
cloudinit/config/cc_vyos_userdata.py |
Refactored to use the new utility module for config file operations |
cloudinit/config/cc_vyos.py |
Updated to use centralized config file handling from the utility module |
zdc
left a comment
There was a problem hiding this comment.
Thank you for the PR!
The solution you propose seems overly complicated for the issue it addresses. While it could work with some adjustments, the Cloud‑Init structure limits us here and forces us to keep modules self‑sustainable - so we should avoid introducing vyos_util.py.
I would recommend simplifying it as follows:
- Update the path to the default config file in both modules: change
/opt/vyatta/etc/config.boot.default→/usr/share/vyos/config.boot.default. - Add a new variable in both modules:
CFG_FILE_SAVEPATH = '/opt/vyatta/etc/config/config.boot' - Use that variable in the save operation:
with open(CFG_FILE_SAVEPATH, 'w') as f:
That resolves the problem with just three lines of code changes per module.
Please also reformat the commit description according to the requirements (header + description of the problem solved or changes made).
|
Hi, code updated. |
dmbaturin
left a comment
There was a problem hiding this comment.
Now it's much simpler, should be good to go.
zdc
left a comment
There was a problem hiding this comment.
Thank you for the update! It looks good to go.
Proposed Commit Message
Additional Context
Test Steps
Checklist:
Hi,
I wanted to test this changes, but i don't have idea how to duild this package myself.
Could you provide me instructions how to build it as .deb package?