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: update options #91960

Merged
merged 5 commits into from Aug 6, 2020
Merged

nixos/networkd: update options #91960

merged 5 commits into from Aug 6, 2020

Conversation

datafoo
Copy link
Contributor

@datafoo datafoo commented Jul 1, 2020

Motivation for this change

To fix missing option DNSDefaultRoute= (see #91761) and other missing options.

Things done

I have not tested all options that I touched.

  • Tested using NixOps
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@datafoo
Copy link
Contributor Author

datafoo commented Jul 2, 2020

I will update the PR following this comment.

Copy link
Member

@Ma27 Ma27 left a comment

Choose a reason for hiding this comment

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

I'm wondering if we should add a note to the assertion-functions in systemd-lib.nix which state that deprecated options are removed from the upstream man-pages and aren't supported by that module and for changed names, it should be referred to the upstream release-notes.

nixos/modules/system/boot/networkd.nix Outdated Show resolved Hide resolved
nixos/modules/system/boot/networkd.nix Show resolved Hide resolved
@flokli
Copy link
Contributor

flokli commented Jul 11, 2020

This needs another rebase.

@datafoo
Copy link
Contributor Author

datafoo commented Jul 13, 2020

This needs another rebase.

Done

This is the second time I rebased my PR on master after more recent PRs than mine were merged. What guides the order in which PRs are merged?

@flokli
Copy link
Contributor

flokli commented Jul 13, 2020

Thanks!

This is the second time I rebased my PR on master after more recent PRs than mine were merged. What guides the order in which PRs are merged?

There's no specific order, but this PR is quite big, which is why it's harder to review.

@flokli flokli requested a review from Ma27 July 13, 2020 19:29
@flokli flokli added this to To Do in systemd via automation Jul 13, 2020
@mweinelt
Copy link
Member

Networking-Tests are looking good.

/nix/store/4y8k9isgglvhycvq59kg50hx62rw08kn-vm-test-run-Bond-Networking-Networkd
/nix/store/0kmcb3mwlm63hjkbd50h075d61ncqf0i-vm-test-run-Bridge-Networking-Networkd
/nix/store/dcz18v4qz82sx9ynr3wyqimzrmaigpgg-vm-test-run-OneInterfaceDHCP-Networking-Networkd
/nix/store/1g9p0kgpd8nzbv6kqkkcq08nh8hmvmxi-vm-test-run-SimpleDHCP-Networking-Networkd
/nix/store/3014l21l3qrg01s3x33ys08fgjqrs9lw-vm-test-run-Link-Networking-Networkd
/nix/store/jllmqcvalzn1imsb6x0r57dfcdw2v7cz-vm-test-run-Loopback-Networking-Networkd
/nix/store/431izfl3mb82crr6kaw143ky0r04c9yl-vm-test-run-MACVLAN-Networking-Networkd
/nix/store/p7vj2zfcl228a9p5ri27ycrdm1yix6v9-vm-test-run-Privacy-Networking-Networkd
/nix/store/fs1rk6x92h83gfm1f8pm4ps8158g17ph-vm-test-run-routes-Networking-Networkd
/nix/store/wvh4cha8ssx3ypplwqp75wf1phiy46yx-vm-test-run-Sit-Networking-Networkd
/nix/store/blbh606jdzsbgdadbavj8nl0ic00c4d0-vm-test-run-Static-Networking-Networkd
/nix/store/mgv2b09h658glqn8lgz2s3akxdwzf7d7-vm-test-run-Virtual-Networking-Networkd
/nix/store/8kfj6r2kh6ab2w5g6ix304fr21zlw2vz-vm-test-run-vlan-Networking-Networkd

Copy link
Contributor

@flokli flokli left a comment

Choose a reason for hiding this comment

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

I tried reviewing this, but it's really hard to do.

e9d13d3 added some commented-out checks due to Nix' <= 2.2 integer size, and 70407f0 commented them back in. Could this be squashed together, so we only add the checks in a single commit?

Also, e9d13d3 does a bit too much at once. For example, it removes OriginalName. Is this intended?

Could you revise the commits, and make it these changes easier to review? Maybe something like this also deserves a separate commit.

Thanks again for your patience! I'll try to react quicker on further updates.

systemd automation moved this from To Do to In Progress Jul 14, 2020
@datafoo
Copy link
Contributor Author

datafoo commented Jul 15, 2020

I tried reviewing this, but it's really hard to do.

e9d13d3 added some commented-out checks due to Nix' <= 2.2 integer size, and 70407f0 commented them back in. Could this be squashed together, so we only add the checks in a single commit?

As much as I could try, each of these 2 commits do one logical thing and one thing only, as described by the commit messages

  • e9d13d3: update options for systemd 245. That commit synchronizes the code with the man page of networkd and, for homogeneity with the rest of the code, keeps avoiding the use of assertRange with 64bits integers.
  • 70407f0: use assertRange with 64bits integers. This commit takes into account this comment. Perhaps it would better fit as a separate PR after this one would have been merged. Since the PR is about cleaning up the networkd module, I included this commit here.

I apologize if my commit messages were not clear enough.

Also, e9d13d3 does a bit too much at once. For example, it removes OriginalName. Is this intended?

Again, that commit synchronizes the code with the man page of networkd. The best way to review that I would suggest is to read the man page. For the specifics of OriginalName, refer to man systemd.link. I also try to double check by contacting the author here, without success. However, you kind of confirmed it yourself here. To facilitate our work, let's discuss specific code issues/questions with the review feature of Github.

Could you revise the commits, and make it these changes easier to review? Maybe something like this also deserves a separate commit.

I do not think it is beneficial because the commit does one clear logical thing. If you disagree, please describe what you mean by "these changes" because, as you may have noted, this is not the only thing I removed in this commit e9d13d3.

Thanks again for your patience! I'll try to react quicker on further updates.

Thank you as well for reviewing this.

@flokli flokli mentioned this pull request Jul 31, 2020
10 tasks
@datafoo
Copy link
Contributor Author

datafoo commented Aug 4, 2020

Could we move on with this PR?

@picnoir picnoir removed their request for review August 4, 2020 19:54
@flokli flokli requested a review from aanderse August 5, 2020 16:11
@flokli
Copy link
Contributor

flokli commented Aug 6, 2020

I gave this another close look, and while it is pretty hard to review due to the amount of changes, this seems to be fine, and should make things more readable and understandable in the longer run.

Thanks for the work you put in this, and your patience with me :-)

@flokli flokli merged commit c1f77f4 into NixOS:master Aug 6, 2020
systemd automation moved this from In Progress to Done Aug 6, 2020
@datafoo datafoo deleted the fix-issue-91761 branch January 10, 2022 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
systemd
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants