n8n: init 0.96.0 #98234
n8n: init 0.96.0 #98234
Conversation
|
Added a test again based on bazaar service test, just checking that the port is available and working |
freezeboy
force-pushed
the
add-n8n
branch
2 times, most recently
from
da06607
to
82f4a52
Compare
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
|
Diff LGTM, only there should be 2 commits. |
|
1 for the package and 1 for the module and test ? |
|
Yes. |
|
cc me when you force push. |
|
cc @doronbehar |
There was a problem hiding this comment.
@freezeboy I find it difficult to review this module with the whitespace here. Can you please have consistent indentation throughout the module, following the standards for nixpkgs?
|
@aanderse Sorry, I force pushed this PR and I am not sure which indent problem you have :/ |
You are using a mixture of tabs and spaces. If you view your changes in the github ui don't you see very inconsistent spacing like this? |
|
Oh my bad ... It's ugly, didn't notice the tabs here |
freezeboy
force-pushed
the
add-n8n
branch
2 times, most recently
from
7516158
to
0591ee0
Compare
nixos/modules/services/misc/n8n.nix
Outdated
| Group = cfg.group; | ||
| StateDirectory = "n8n"; | ||
| SyslogIdentifier = "n8n"; | ||
| Environment = "N8N_CONFIG_FILES=${configFile}"; |
There was a problem hiding this comment.
Personally I prefer systemd.services.n8n.environment.N8N_CONFIG_FILES because it allows the user more control, but this is fine as well if you don't like my suggestion.
| }; | ||
|
|
||
| config = mkIf cfg.enable { | ||
| systemd.services.n8n = { |
There was a problem hiding this comment.
I noticed the upstream repo doesn't have a systemd unit file. They might benefit from you submitting a PR to add one.
nixos/modules/services/misc/n8n.nix
Outdated
| description = "Open ports in the firewall for the n8n web interface."; | ||
| }; | ||
|
|
||
| package = mkOption { |
There was a problem hiding this comment.
It looks like the n8n package has no build options and only a single version in nixpkgs: what is the value this option provides?
There was a problem hiding this comment.
just to make it easy to use a custom version ?
There was a problem hiding this comment.
This is what overrides are for.
|
@aanderse Thank you for the the review, I tried to fix the things you mention Atm, no idea to fix the test and the hardening parts |
nixos/modules/services/misc/n8n.nix
Outdated
| description = "Group under which n8n runs."; | ||
| }; | ||
|
|
||
| config = { |
There was a problem hiding this comment.
No, added to the list
There was a problem hiding this comment.
So basically, I rename my "config" key to a single empty option, link to the documentation, and use a lib to build the json file?
Is there a general policy for the default?
There was a problem hiding this comment.
So basically, I rename my "config" key to a single empty option, link to the documentation, and use a lib to build the json file?
Yes. If you don't think this provides enough value to the sysadmin you could utilize a freeform module option to provide type checking and validation to your settings option.
Please don't hesitate to ask for any help on this PR 😄
There was a problem hiding this comment.
Ok I see in the config part, instead of the options
nixos/modules/services/misc/n8n.nix
Outdated
| Group = cfg.group; | ||
| StateDirectory = "n8n"; | ||
| SyslogIdentifier = "n8n"; | ||
| Environment = "N8N_CONFIG_FILES=${configFile}"; |
|
I guess the hardening blocks the tests, I can't write the config to /var/lib/n8n anymore |
freezeboy
force-pushed
the
add-n8n
branch
2 times, most recently
from
9dabaaf
to
d056c73
Compare
|
Test passes, thank you for all your reviews, the module is way simpler and safer now. |
|
@aanderse I did some cleanup, I think this time it is ok |
There was a problem hiding this comment.
Fantastic job on the module @freezeboy! I approve of the module portion of this PR. Unfortunately I don't usually review packages, and am not overly qualified to review Right, @doronbehar has already reviewed the package changes... so I think we're good to merge. Right? 😄nodePackage changes so we will have to get someone else to review that.
|
@freezeboy I'm a bit reluctant into adding yet another package with complete node dependencies Nix expressions. There is a call for node packages maintainers and in general most people (I think) would like to get less of such expressions, mostly because they take a lot of disk space (even after removed of course, due to version control), see e.g: #96961 (review) . There was one PR in the past that aggregated a few |
|
@doronbehar In fact I tried yesterday to put it it node-packages.json file, but I encountered two problems: btc-rpc-explorer was having a problem with a timeout with github.com, and the generate script was taking ages ... so I gave up |
|
Sigh... Maybe in the future this will work better, with flakes or some mach-nix kind of implementation for Node packages. In the meantime it's not worth waiting IMO. The test passes and the diff LGTM. |
|
Result of 1 package blacklisted:
1 package built:
|
Motivation for this change
New package for n8n.io app
Service heavily inspired from
services/misc/bazaar.nixNever wrote a test, and don't know which one would be great for such a service
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)