nixos/hledger-web: add stateDir, use own user, fix ExecStart #116107
Conversation
ofborg
bot
added
6.topic: nixos
erictapen
force-pushed
the
hledger-web-statedir
branch
from
0b31d77 to
9423de7
Compare
| type = types.path; | ||
| example = "/home/hledger/.hledger.journal"; | ||
| type = types.str; | ||
| default = ".hledger.journal"; |
There was a problem hiding this comment.
Somehow this breaking change slipped by me before. You might consider adding a check or assertion that the value here is a file name and not a path+file name. Release notes wouldn't hurt either...
You could probably do better, but at least checking that the name doesn't start with a forward slash could help 🤷♂️
There was a problem hiding this comment.
I guess that would make sense to have. I implemented an assertion:
error:
Failed assertions:
- services.hledger-web.journalFile is no longer defined as a path, but
as a file name located in services.hledger-web.stateDir.
Given that hledger wasn't mentioned once in the NixOS release notes, I'd figure the module isn't used enough to justify an entry?
erictapen
force-pushed
the
hledger-web-statedir
branch
from
9423de7 to
f1649a0
Compare
| ''; | ||
| }; | ||
|
|
||
| journalFile = mkOption { | ||
| type = types.path; | ||
| example = "/home/hledger/.hledger.journal"; | ||
| type = types.str; |
There was a problem hiding this comment.
Reading the documentation of hledger-web again, I've seen that you can define multiple journal files using -f hledger-web documentation. We can consider making this a listof strings types.nix
There was a problem hiding this comment.
Sounds reasonable, I added another commit that makes the change. Removed the assertion with it, as users will see an error with their configuration anyway.
@marijanp I you think it's good I can squash the commit into the first one.
| "--capabilities=${capabilityString}" | ||
| (optionalString (cfg.baseUrl != null) "--base-url=${cfg.baseUrl}") | ||
| (optionalString (cfg.serveApi) "--serve-api") | ||
| ] ++ (map (f: "--file=${stateDir}/${f}") cfg.journalFiles) |
There was a problem hiding this comment.
I would have expected that you only need to set -f once, in case somebody is wondering too. But this is correct. -f for each file.
This allows for shared hledger installations, where the web interface is available via network and multiple user share a SSH access to the hledger user. Also added `--serve` to the CLI options, as hledger-web tries to open a webbrowser otherwise: hledger-web: xdg-open: rawSystem: runInteractiveProcess: exec: does not exist (No such file or directory) Co-authored-by: Aaron Andersen <[email protected]>
erictapen
force-pushed
the
hledger-web-statedir
branch
from
e4cbee8 to
55c7cb3
Compare
|
Squashed and rebased, thanks for the review! |
Motivation for this change
This allows for shared hledger installations, where the web interface is available via network and multiple user share a SSH access to the hledger user.
Also added
--serveto the CLI options, as hledger-web tries to open a webbrowser otherwise:hledger-web: xdg-open: rawSystem: runInteractiveProcess: exec: does not exist (No such file or directory)Things done
hledgeroverhledger-web, is this should be useful for users of Hledger in general, not only hledger-web.cfg.stateDircfg.capabilitiesas booleans, as I messed with shell escaping for too long and this seems more adequate to me.sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)