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 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.

@datafoo datafoo force-pushed the datafoo:fix-issue-91761 branch from 1d48c87 to 660ced5 Jul 2, 2020
@Ma27 Ma27 self-requested a review Jul 6, 2020
Copy link
Member

Ma27 left a comment

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
@datafoo datafoo force-pushed the datafoo:fix-issue-91761 branch from 660ced5 to 8896bce Jul 9, 2020
@flokli
Copy link
Contributor

flokli commented Jul 11, 2020

This needs another rebase.

@datafoo datafoo force-pushed the datafoo:fix-issue-91761 branch from 8896bce to 70407f0 Jul 13, 2020
@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 Jul 13, 2020
@flokli flokli added this to To Do in systemd via automation Jul 13, 2020
@Ma27
Ma27 approved these changes Jul 13, 2020
@mweinelt
Copy link
Member

mweinelt commented Jul 13, 2020

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 left a comment

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
0 of 10 tasks complete
@datafoo
Copy link
Contributor Author

datafoo commented Aug 4, 2020

Could we move on with this PR?

@NinjaTrappeur NinjaTrappeur removed their request for review Aug 4, 2020
@flokli flokli requested a review from aanderse Aug 5, 2020
@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
14 checks passed
14 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="70407f0"; rev="70407f09da97a4a4a79d1927c7074ca317f37d7f"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="70407f0"; rev="70407f09da97a4a4a79d1927c7074ca317f37d7f"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="70407f0"; rev="70407f09da97a4a4a79d1927c7074ca317f37d7f"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="70407f0"; rev="70407f09da97a4a4a79d1927c7074ca317f37d7f"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="70407f0"; rev="70407f09da97a4a4a79d1927c7074ca317f37d7f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="70407f0"; rev="70407f09da97a4a4a79d1927c7074ca317f37d7f"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="70407f0"; rev="70407f09da97a4a4a79d1927c7074ca317f37d7f"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
systemd automation moved this from In Progress to Done Aug 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
systemd
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.