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.

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

migrate: support aliases #473

Merged
merged 1 commit into from
Jun 3, 2024
Merged

migrate: support aliases #473

merged 1 commit into from
Jun 3, 2024

Conversation

Kristof0127
Copy link

@Kristof0127 Kristof0127 commented May 24, 2024

Description

Netplan migrate haven't supported old syntax network interface aliases yet (eg. eth1:1 alias for eth1). An if statement is added to migrate.py that checks if interface name contains ':', this case the iface will be renamed and handled as not the alias one. The expected output is a simple interface that has multiple addresses. A test case is also provided in cli_legacy.py with the following name: test_static_ipv4_alias.

Checklist

  • Runs make check successfully.
  • Retains 100% code coverage (make check-coverage). (for migrate.py)
  • New/changed keys in YAML format are documented.
  • (Optional) Adds example YAML for new feature.
  • (Optional) Closes an open bug in Launchpad.

@slyon slyon added the community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments. label May 29, 2024
Copy link
Collaborator

@slyon slyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for your contribution to Netplan!

This looks mostly good to me! It is related to #89 and the discussion in LP#1743200.

Do you have experience with using netplan migrate? I haven't seen it widely used out in the wild. Furthermore, this command is hidden behind the ENABLE_TEST_COMMANDS env variable. I was actually pondering about dropping support for it altogether..

@Kristof0127 What do you think about my in-line comment? Do you want to improve upon this, or shall we merge it as-is? Both is fine for me!

'auto en1:1\niface en1:1 inet static\naddress 1.2.3.5/8', dry_run=True)[0]
self.assertEqual(_load_yaml(out), {'network': {
'version': 2,
'ethernets': {'en1': {'addresses': ["1.2.3.4/8", "1.2.3.5/8"]}}}}, out.decode())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought (non-blocking): I'm pondering if this should be translated to something like that instead:

version: 2
ethernets:
  en1:
    addresses:
      - "1.2.3.4/8"
      - "1.2.3.5/8":
          label: "en1:1"

But this could also be done as a separate PR, as currently this is a targeted fix for a specific problem. The solution could be enhanced to retain more information via the label.

@Kristof0127
Copy link
Author

@slyon thank you for your review. I don't have much experience with netplan migrate. However, in my current project, using migrate was the easiest way to ensure backward compatibility with old interface files, and I think it's quite a powerful tool.

In response to your in-line comment, I agree that including the label would be the comprehensive solution, but if I saw correctly using labels is only supported by the networkd backend. Unfortunately, I use NetworkManager as renderer, so using labels is not possible in my case. I think adding the label as well can be done in a later separate PR, if both backends can support this feature.

@slyon slyon merged commit ebf38ec into canonical:main Jun 3, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community This PR has been proposed by somebody outside of the Netplan team and roadmap commitments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants