nixos/quickwit: init #307808
nixos/quickwit: init #307808
Conversation
tcheronneau
force-pushed
the
nixos/quickwit
branch
from
9eedb88
to
a3cd717
Compare
tcheronneau
force-pushed
the
nixos/quickwit
branch
from
a3cd717
to
d10b48d
Compare
tcheronneau
force-pushed
the
nixos/quickwit
branch
from
d10b48d
to
a20034b
Compare
There was a problem hiding this comment.
@tcheronneau Great!
It needs some minor changes though, most importantly that the test actually works. The goal for that is that one can run:
NIX_PATH=nixpkgs=. nix-build --no-out-link -A quickwit.tests
To get that to work, your PR was missing a couple entries, such as in the global list of services, and the global list of tests.
I've pushed a commit for that, please take a look. (You need to squash it before final merge.)
For me the test doesn't pass yet though.
| users.groups.quickwit = {}; | ||
| system.activationScripts.makeQuickwitDir = '' | ||
| mkdir -p ${cfg.settings.data_dir} | ||
| chown -R quickwit:quickwit ${cfg.settings.data_dir} |
There was a problem hiding this comment.
This works.
I'm testing the full setup each time (with nixos-vm).
There was a problem hiding this comment.
@tcheronneau The fact that it works for you doesn't make it correct.
The path needs quoting because it is incorrect otherwise.
If you set (do not try this) data_dir = "/ my folder containing a space";, the above will chown / due to lack of quoting; if the command was rm -r, this would delete your whole computer.
|
@tcheronneau I pushed some more fixes, please take a look. |
| environmentFile = mkOption { | ||
| type = types.nullOr types.path; | ||
| default = null; | ||
| description = "Environment file to pass to the systemd service."; |
There was a problem hiding this comment.
Expands docs the same way other environmentFile options do
| group = "quickwit"; | ||
| }; | ||
| users.groups.quickwit = {}; | ||
| system.activationScripts.makeQuickwitDir = '' |
There was a problem hiding this comment.
I suspect that the current way to do that is with systemd.tmpfiles, see e.g.
nixpkgs/nixos/modules/services/monitoring/munin.nix
Lines 402 to 407 in 0638fe2
But do it a bit better than done there: Refer to users.users.quickwit.name instead of hardcoding the user name in multiple places.
There was a problem hiding this comment.
Hum this is weird to me though it's called tmpfiles.
https://search.nixos.org/options?channel=23.11&show=systemd.tmpfiles.rules&from=0&size=50&sort=relevance&type=packages&query=systemd.tmpfiles.rules it's says it's for volatile dir and data dir is everything but volatile.
There was a problem hiding this comment.
I did like that because I saw it in another package (I don't exactly remember which one though).
There was a problem hiding this comment.
Hum this is weird to me though it's called tmpfiles.
I find that weird, too.
Systemd docs say this about it:
StateDirectory=,CacheDirectory=,LogsDirectory=, andConfigurationDirectory=should be used to create directories under/var/lib/,/var/cache/,/var/log/, and/etc/.
tmpfiles.dshould be used for files whose lifetime is independent of any service or requires more complicated configuration.
So indeed it seems like StateDirectory= would be better than both tmpfiles and activationScripts.
|
Thanks for your help I'll review all your comments this evening. |
This works for me. |
@tcheronneau You mean on the current version, right? That makes sense, because the commits I pushed fixed that. But I believe your original version (on which I wrote the comment) without commit fbf98b9 because quickwit rejected that syntax. |
|
We're good now or is there something still to fix ? |
|
I think this one was largely superseded by #317152, @tcheronneau - I've moved your test here to #320342. |
Description of changes
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.