Correct password option docs and add related tests #310484
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/3958 |
wegank
added
the
2.status: merge conflict
|
@Ma27 any thoughts on this PR or know who should review it? I just noticed today you recently made some recent changes that lead to a merge conflict (which I have yet to fix), so just pinging to see if you want to take a look or have an idea who to ping to get this reviewed. |
|
Last times I touched this part, it was kinda hard to find anybody who felt responsible. |
fidgetingbits
force-pushed
the
clarify-password-option-overrides
branch
from
00147d2 to
47580a1
Compare
fidgetingbits
removed
the
2.status: merge conflict
|
Merge conflicts should be fixed now |
fidgetingbits
force-pushed
the
clarify-password-option-overrides
branch
from
47580a1 to
b572dd2
Compare
|
Just a note that to run the tests I added you can run the following from nixpkgs root folder: Looks like the latter is failing now due to a new assertion about user types, so I will have to investigate. |
|
Looks like there are more aggressive asserts that helps with some of the systemd-sysusers stuff, so I removed a couple of assertions from that test that no longer will run. Also changed the user type and added groups to ensure other new assertions pass so the test actually runs. I've left the changes as separate commit now for sake of reference, but can squash them before merge. |
|
Also asked in the nixos-systemd Matrix channel for more reviewers for the systemd-sysusers part. |
|
I'd really like to see this in 24.11 when it's good to go: this isn't a behavioral fix, but clarifies the current behavior in both warnings and the manual (and locks it down with a test). |
|
I've purposefully not rebased commits for now to make diffing of feedback a bit easier. I added an additional test to so we can track where |
|
I still need to add the clearer wording for the greg case with mutableUsers difference, but otherwise all other feedback should be addressed now. |
ofborg
bot
added
2.status: merge conflict
|
I took a stab at rewording the whole thing based on EmergentMind's feedback and because I found myself getting confused reading it even though I'm working on it. That allowed me to de-duplicate some text from other options, which makes One part I didn't include which I considered putting at the end of Since technically that order still matches what is shown earlier (just with password, hashedPassword, hashedPasswordFile omitted), I wasn't sure if I should explicitly include it or not. Happy to add it if others thing it's worth. |
|
Good for another review pass I think. In order to fix the assert warning being wrong, and make it harder to mess up the override order text in various places, I tried to deduplicate it all and use some variable expansion to help generate it in various places. |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4905 |
|
It's on my radar and I still have the tab open. |
fidgetingbits
force-pushed
the
clarify-password-option-overrides
branch
from
9b1696b to
4d1ff6e
Compare
Testing showed that the existing documentation regarding password override ordering was incorrect. This commit corrects the errors and refactors the way the text is constructed to make updating future ordering changes significantly easier.
This commit adds two new tests to show that the ordering of password overrides documentation in nixos/modules/config/user-groups.nix is correct. The override behavior differs depending on whether a system has systemd-sysusers enabled, so there are two tests.
fidgetingbits
force-pushed
the
clarify-password-option-overrides
branch
from
4d1ff6e to
b84fb1e
Compare
Otherwise the evaluation warnings have a two or even three lines of whitespace between paragraphs.
…fy SetCredential check It was kinda weird to assert that the clear-text password was in the unit when the hashed password was the effective one. This change makes it explicit that both are in there and the latter takes precedence.
|
Successfully created backport PR for |
|
Thanks a lot for all your help working through this! |
Description of changes
This attempts to clarify the true behavior of password option overrides, as the documentation seems to be wrong (and appears to have been for at least 10 years). These docs also clarify systemd-sysusers quirks that I coincidentally noticed someone happened to file recently here as well.
The relevant option docs have been updated, but more importantly I created two new tests to try to show the true behavior of the overrides in their current form. This way if anything ever changes on this front it will be caught and docs can be updated accordingly.
If you want to run the tests I added, you will have to change the maintainer for now as my being added to maintainers is pending this PR being merged.
I've not used Nix for very long, so would appreciate someone who has dealt with this code in the past to validate what I've found. @nikstur and @NeQuissimus, perhaps you could both take a look as I've repurposed some of your tests that touch the same areas, so I suspect you'll both be familiar.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.