fix: Render bridges correctly for v2 on sysconfig with set-name #5674
Conversation
When listing interfaces in v2 format, we should expect to be able to reference other interfaces using the name in the configuration, not the name the interface will eventually take. This was broken when using `set-name`. To fix this, we now store the configuration id alongside the eventual name, and reference that instead of the name. Fixes canonicalGH-5574
|
@xiachen-rh , does this solve the issue that you raised in #5574 ? |
|
Sorry I just notice this, I will test it ASAP and let you know the result. |
|
@TheRealFalcon Hi James, the fix works for sysconfig renderer. However, we still have an issue for NM renderer, so I filed a new issue #5717 to track it. |
|
@xiachen-rh , thanks for checking and filing that issue. We can follow up on that separately. @holmanb , this is ready for review. |
|
@TheRealFalcon @holmanb waiting for this to get merged. Then #5740 . |
There was a problem hiding this comment.
@TheRealFalcon Thanks for this! I left a couple of nitpicks, take them or leave them. Either way it looks good to me.
| iface = copy.deepcopy(iface) | ||
|
|
||
| # Remove irrelevant keys | ||
| with suppress(KeyError): |
There was a problem hiding this comment.
I like using suppress, but in this case it doesn't appear to add value. Wouldn't something like this suffice?
if iface.get("config_id"):
iface.pop("config_id")There was a problem hiding this comment.
It doesn't remove value either than I think easier to ask forgiveness than permission is still slightly more idiomatic.
| if "nameservers" in dev_cfg: | ||
| search = dev_cfg.get("nameservers").get("search", []) | ||
| dns = dev_cfg.get("nameservers").get("addresses", []) | ||
| name_cmd = {"type": "nameserver"} | ||
| if len(search) > 0: | ||
| name_cmd.update({"search": search}) | ||
| name_cmd["search"] = search |
There was a problem hiding this comment.
We could similarly exploit the falsyness of search and dns (no need to check that len() > 0) and also remove the default values in .get("search", []) and .get("addresses", []), if you want.
Proposed Commit Message
Additional Context
Test Steps
Merge type