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

nixos/networkd: allow strings for addresses #342307

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Sep 16, 2024

Description of changes

⚠️ Note: this is a draft on purpose and I didn't fix up all the uses of address yet. First I'd like to discuss if

  • this makes sense to you.
  • we want to keep address or deprecate it.

The difference between address and addresses is that the former adds an Address key into the [Network] section whereas the latter creates its own [Address] section. However, these are effectively equivalent as far as systemd-networkd is concerned (from systemd.network(5)):

This is a short-hand for an [Address] section only containing an
Address key (see below).

I think it's pretty awkward that you have to remember which one to use.

This patch alters the type of addresses to allow both [Address]-config encoded as Nix attribute-set and strings that will be rendered to

[Address]
Address=...

As a result, the address option is obsolete, so I decided to deprecate it.


cc @NixOS/systemd @mweinelt

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

The difference between `address` and `addresses` is that the former adds
an `Address` key into the `[Network]` section whereas the latter creates
its own `[Address]` section. However, these are effectively equivalent
as far as systemd-networkd is concerned (from `systemd.network(5)`):

    This is a short-hand for an [Address] section only containing an
    Address key (see below).

I think it's pretty awkward that you have to remember which one to use.

This patch alters the type of `addresses` to allow both
`[Address]`-config encoded as Nix attribute-set and strings that will be
rendered to

    [Address]
    Address=...

As a result, the `address` option is obsolete, so I decided to deprecate
it.
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Sep 16, 2024
@@ -2470,6 +2470,7 @@ let
address = mkOption {
default = [ ];
type = types.listOf types.str;
apply = warn "`systemd.network.networks.\"${name}\".address` is deprecated, use `systemd.network.networks.\"${name}\".addresses` instead.";
Copy link
Member Author

Choose a reason for hiding this comment

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

Went this route since mkRenamedOptionModule seems broken for submodules: #96006

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Sep 16, 2024
@Ma27 Ma27 marked this pull request as ready for review October 12, 2024 14:47
@Ma27 Ma27 requested a review from a team October 12, 2024 14:48
@Ma27
Copy link
Member Author

Ma27 commented Oct 12, 2024

This still requires fixups of the existing usages, but I'd still like to get some feedback on that.

@mweinelt
Copy link
Member

mweinelt commented Oct 12, 2024

Yes, I think [Address] sections and the plural option name make more sense and are at the same time more powerful, because they allow further customization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants