nixos/smartd: allow extra cli options for daemon #32327
Conversation
| @@ -108,6 +108,18 @@ in | |||
| ''; | |||
| }; | |||
|
|
|||
| extraOptions = mkOption { | |||
| default = ""; | |||
| type = types.str; | |||
There was a problem hiding this comment.
How about types.listOf types.str?
There was a problem hiding this comment.
I looked at several examples of an extraOptions... option, and there seems to be a roughly 50/50 split between a single string and lists of strings. Is there a strong reason for me to change this? Do core maintainers prefer one over the other?
There was a problem hiding this comment.
I like the list type it because it allows easily merging multiple definitions. But I cannot speak for others.
There was a problem hiding this comment.
I'm with @bjornfor on this for the same reason.
|
@GrahamcOfBorg eval |
| @@ -222,7 +234,7 @@ in | |||
|
|
|||
| path = [ pkgs.nettools ]; # for hostname and dnsdomanname calls in smartd | |||
|
|
|||
| serviceConfig.ExecStart = "${pkgs.smartmontools}/sbin/smartd --no-fork --configfile=${smartdConf}"; | |||
| serviceConfig.ExecStart = "${pkgs.smartmontools}/sbin/smartd ${cfg.extraOptions} --no-fork --configfile=${smartdConf}"; | |||
There was a problem hiding this comment.
Assuming the attribute is converted to a list of strings, we will need lib.concatStringsSep " " cfg.extraOptions here.
kevin-hanselman
force-pushed
the
smartd_expose_cli_options
branch
from
7950252
to
c6e377f
Compare
kevin-hanselman
force-pushed
the
smartd_expose_cli_options
branch
from
c6e377f
to
a92f098
Compare
kevin-hanselman
requested review from
edolstra,
edwtjo,
FRidh,
nbp,
peti,
ttuegel and
zimbatm
as code owners
|
Forgot to rebase 😞. Will fix. Sorry to all those who got pinged for review. |
This enables further customization of smartd.
kevin-hanselman
force-pushed
the
smartd_expose_cli_options
branch
from
a92f098
to
0dfcd45
Compare
|
I think the integration check failure may have been a race condition of sorts when I pushed twice in quick succession. Is there any way to re-run the checks? |
|
@GrahamcOfBorg eval |
|
Looks good to me. Applied to master (65fb15a). Thanks! |
Motivation for this change
This enables further customization of smartd.
Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)