netdata service: init #20183
netdata service: init #20183
Conversation
|
@womfoo, 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. |
| }; | ||
|
|
||
| configText = mkOption { | ||
| type = types.nullOr types.str; |
There was a problem hiding this comment.
This could be made type = types.line; with "" as default. Removing nullOr save the need for if isNull cfg.configText then "" else " later, and types.lines will provide better value merging in case the option is defined multiple times.
womfoo
force-pushed
the
init/netdata-service
branch
from
92f2254
to
5bae511
Compare
|
Thanks @ericsagnes! It just got back to me that netdata works with empty configs. Rebased changes with your feedback. |
womfoo
force-pushed
the
init/netdata-service
branch
from
5bae511
to
7ef98a8
Compare
|
Rebased changes to address conflict in ids.nix |
| @@ -278,6 +278,7 @@ | |||
| postgrey = 258; | |||
| hound = 259; | |||
| leaps = 260; | |||
| netdata = 261; | |||
There was a problem hiding this comment.
Looks to me like the data generated by this service is highly machine specific and that ownership will be eventually consistent, given the preStart script. Please consider dynamic uid allocation.
womfoo
force-pushed
the
init/netdata-service
branch
from
7ef98a8
to
4579535
Compare
|
@joachifm thanks for reviewing this. I've updated the commit to use dynamic uids. |
| wantedBy = [ "multi-user.target" ]; | ||
| preStart = concatStringsSep "\n" (map (dir: '' | ||
| test -d ${dir} || { | ||
| echo "creating directory for netdata in ${dir}" |
There was a problem hiding this comment.
Alternatively, you could do mkdir -vp which will print a message iff the directory is created.
| "/var/log/netdata" | ||
| "/var/lib/netdata" ]); | ||
| serviceConfig = { | ||
| Type = "simple"; |
There was a problem hiding this comment.
Type = "simple" is the default.
| in { | ||
| options = { | ||
| services.netdata = { | ||
| enable = mkEnableOption "netdata"; |
There was a problem hiding this comment.
I believe mkEnableOption has fallen out of favour so probably shouldn't be used for new code.
womfoo
force-pushed
the
init/netdata-service
branch
3 times, most recently
from
728720e
to
6ac08e5
Compare
|
Rebased and tested OK |
| preStart = concatStringsSep "\n" (map (dir: '' | ||
| test -d ${dir} || { | ||
| echo "creating directory for netdata in ${dir}" | ||
| mkdir -vp ${dir} |
There was a problem hiding this comment.
I believe you now should be able to remove the echo and enclosing conditional; mkdir -vp should achieve the same effect.
womfoo
force-pushed
the
init/netdata-service
branch
from
6ac08e5
to
2fcba40
Compare
| after = [ "network.target" ]; | ||
| wantedBy = [ "multi-user.target" ]; | ||
| preStart = concatStringsSep "\n" (map (dir: '' | ||
| test -d ${dir} || mkdir -vp ${dir} |
There was a problem hiding this comment.
You could even remove the test because mkdir -p doesn't fail if the directory already exists.
womfoo
force-pushed
the
init/netdata-service
branch
from
2fcba40
to
2715222
Compare
Motivation for this change
make netdata easier to enable
Things done
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)