irker: init at 2017-02-12 #23963
irker: init at 2017-02-12 #23963
Conversation
|
@dtzWill, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @joachifm and @offlinehacker to be potential reviewers. |
|
|
||
| users.extraUsers = singleton { | ||
| name = "irkerd"; | ||
| uid = config.ids.uids.irkerd; |
There was a problem hiding this comment.
Because I didn't realize you could assign them dynamically, I was looking at other services as examples. I'll look into dynamic, thanks!
There was a problem hiding this comment.
If uid is null it should assign a uid on system activation.
|
Please use separate commits. |
|
Aye aye o7. I squashed them for submitting hehe, my mistake. Will fix with dynamic uid issue, thanks for the review! |
|
Comments should be addressed now, thanks. Tested and seems to be working with dynamic user bit (after deactivating service to purge the user). |
| config = mkIf cfg.enable { | ||
| systemd.services.irkerd = { | ||
| description = "Internet Relay Chat (IRC) notification daemon"; | ||
| requires = [ "network.target" ]; |
There was a problem hiding this comment.
I think it makes sense to do after instead, potentially you also might want to consider using network-online.target depending on what the desired semantics are.
There was a problem hiding this comment.
I had the same thought (at least regarding using after), but went with requires since that's what their provided systemd service file does. I'm not sure what the exact semantic differences are between the two, and since it seems after is the common situation I'd default to using that unless there's a good reason for the other? Anyway I'll update accordingly shortly.
There was a problem hiding this comment.
Pulling in network.target is likely always redundant for upper-layer service units. Specifying after is almost certainly correct, either way (dependency generally doesn't imply ordering).
|
I've pushed a few fixups. Also, wrt. to |
|
Changes look great, thank you very much. Appreciate the explanations and effort, I'll keep them in mind in the future! I also went ahead and actually removed the systemd dependency and related patching-- this was only used to help install the service file which we ignore anyway. |
|
LGTM. Thank you |
Motivation for this change
IRC notification daemon for on-site installation with services like GitLab.
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)