galene: init at 0.2 #109414
galene: init at 0.2 #109414
Conversation
|
Result of 1 package blacklisted:
1 package built:
|
| } | ||
| ]; | ||
|
|
||
| systemd.tmpfiles.rules = [ ] |
There was a problem hiding this comment.
I think there is still an improvement to be made here. Let me try to find some time for this on the weekend and I'll get back to you.
There was a problem hiding this comment.
Hey, did you have the time to look at this ? Thanks !
There was a problem hiding this comment.
I apologize, not yet. I still have plans to...
| description = "Enable Galene Service."; | ||
| }; | ||
|
|
||
| stateDir = mkOption { |
There was a problem hiding this comment.
And do we actually need a stateDir option given all the others directory options? Maybe we can just hardcode /var/lib/galene because the other options exist.
There was a problem hiding this comment.
I feel it's good to let people decide where they want to put their state, and even if the other options exist, they default to reside in ${cfg.stateDir} which means you can change a single parameter (the stateDir) without having to change all of the other options.
There was a problem hiding this comment.
Sorry, what I specifically meant is... are any files stored directly in cfg.stateDir, or are all files stored under the other directories specified by -data, -groups, -recordings, and -static? After looking at the upstream project for a few short minutes it looked like cfg.stateDir doesn't actually hold any files... it just happened to have the other specified directories in it.
I didn't test this, though... so please let me know if I'm wrong.
There was a problem hiding this comment.
No, no files are stored directly in cfg.stateDir but the other directories.
There was a problem hiding this comment.
If the only directories stored in cfg.stateDir are already specified via other options then stateDir as an option is redundant... right? I guess it just means more typing because less defaults...
OK, take your pick. Either way works equally fine, just non technical irrelevant opinion on my part at this point I guess 😄
No problem. I'm gonna mark some of these comments as "resolved" but i'll wait until I resolve them all to push. |
There was a problem hiding this comment.
Great work on this! The module looks fantastic. After the minor points already mentioned are resolved I'm approving the module portion of this PR 👍
A NixOS test would be appreciated. Some people say it should be a hard requirement every module have a NixOS test, some people say it shouldn't. I'll mention that future PRs to this package or module will greatly benefit from a NixOS test, though. Committers can get nervous to hit "merge" if there is no test.
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
/marvin opt-in |
Motivation for this change
Status: draft
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)