fix(mgmt-server): use mgmt_server_router_id in frr.conf template - #626
fix(mgmt-server): use mgmt_server_router_id in frr.conf template#626GeertJohan wants to merge 1 commit into
Conversation
|
In our deployments we use |
|
@iljarotar Do your configurations perhaps define the variable twice, with and without underscore? Because it seems like the role asserts the underscore-version MUST be set? https://github.com/metal-stack/metal-roles/blob/master/partition/roles/mgmt-server/tasks/main.yaml#L8 |
e2e6775 to
bd7a0b7
Compare
|
It seems the linked assert always succeeds because
I've just pushed changes to fix the assert. Also implemented a backwards compatible move towards |
bd7a0b7 to
242085b
Compare
a8ef203 to
fde9a86
Compare
|
To me these changes look good. If we add this breaking change to the release notes, it should be fairly easy to migrate (especially since you have this migration path in there). @iljarotar can you please take another look? |
| - name: Deprecation warning for mgmt_server_routerid | ||
| run_once: true | ||
| print_message: | ||
| type: deprecate | ||
| msg: >- | ||
| The variable mgmt_server_routerid was renamed to mgmt_server_router_id. | ||
| action: Please define mgmt_server_router_id instead and remove the old variable. | ||
| when: mgmt_server_routerid is defined |
There was a problem hiding this comment.
Would it make sense to throw an error in this case?
|
Are we leaving the deprecated |
|
Sounds good @iljarotar , I'll make the change. |
The frr.conf template was the only place using the spelling mgmt_server_routerid, while the role's assert, README and defaults all declare mgmt_server_router_id. This went unnoticed because the mandatory variable assert never actually enforced anything for undefined variables: 'foo is not none' evaluates to true in Jinja when foo is undefined, so deployments defining only mgmt_server_routerid passed the check all along. The template now uses mgmt_server_router_id and the assert checks 'is defined and is not none' so it actually enforces the mandatory variables. This is a breaking change for deployments that only define mgmt_server_routerid: the fixed assert fails with a clear message until the variable is renamed. Generated-By: Claude Code Fable 5
11602ff to
343d1ae
Compare
|
Rebased and squashed. |
|
Please add a BREAKING_CHANGE notification in the PR description. This ensures that the breaking change is documented in the final release notes. Looks like this: ## Release Notes ### Breaking Change ```BREAKING_CHANGE |
|
/ok-to-build |
3 participants
It seems there is missing an underscore here but I'm not sure how it's gone unnoticed for 3 years?
Or perhaps
routeridwithout underscore is preferred, but then three other files err by having the underscore.