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
Conversation
I will update the PR following this comment. |
There was a problem hiding this 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.
This needs another rebase. |
Simplifies greatly the interpretation of commit differences.
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? |
Thanks!
There's no specific order, but this PR is quite big, which is why it's harder to review. |
Networking-Tests are looking good.
|
There was a problem hiding this 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.
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
I apologize if my commit messages were not clear enough.
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
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.
Thank you as well for reviewing this. |
Could we move on with this PR? |
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 :-) |
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.
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)