Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix nm bridges #5740

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fix nm bridges #5740

wants to merge 2 commits into from
+207 −25

Conversation

TheRealFalcon
Copy link
Member

In draft state until #5674 merges.

Proposed Commit Message

fix: Render v2 bridges correctly on network-manager with set-name

Similar to the recent sysconfig fix, ensure bridges render correctly
for configs that contain `set-name`.

Fixes GH-5717

Additional Context

Test Steps

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@TheRealFalcon
fix: Render bridges correctly for v2 on sysconfig with set-name
6cf4d30 
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
@TheRealFalcon
fix: Render v2 bridges correctly on network-manager with set-name
75e95ff 
Similar to the recent sysconfig fix, ensure bridges render correctly
for configs that contain `set-name`.

Fixes canonicalGH-5717
@TheRealFalcon
Copy link
Member Author

FYI @xiachen-rh . I am less familiar with network manager keyfile definition, so please let me know if this works for you.

@xiachen-rh
Copy link
Contributor

FYI @xiachen-rh . I am less familiar with network manager keyfile definition, so please let me know if this works for you.

OK, I will test it and let you know the result.

@xiachen-rh
Copy link
Contributor

@TheRealFalcon It works when using NM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TheRealFalcon @xiachen-rh