Skip to content

n8n: init 0.96.0#98234

Merged
doronbehar merged 2 commits intoNixOS:masterfrom
freezeboy:add-n8n
Dec 5, 2020
Merged

n8n: init 0.96.0#98234
doronbehar merged 2 commits intoNixOS:masterfrom
freezeboy:add-n8n

Conversation

@freezeboy
Copy link
Contributor

@freezeboy freezeboy commented Sep 18, 2020 •
edited
Loading

Motivation for this change

New package for n8n.io app

Service heavily inspired from services/misc/bazaar.nix
Never wrote a test, and don't know which one would be great for such a service

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Sep 18, 2020
@freezeboy
Copy link
Contributor Author

freezeboy commented Sep 19, 2020

Added a test again based on bazaar service test, just checking that the port is available and working

@freezeboy freezeboy force-pushed the add-n8n branch 2 times, most recently from da06607 to 82f4a52 Compare September 19, 2020 18:27
@freezeboy freezeboy changed the title n8n: init 0.83.0 n8n: init 0.87.1 Oct 15, 2020
@nixos-discourse
Copy link

nixos-discourse commented Nov 15, 2020

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review/3032/368

@doronbehar
Copy link
Contributor

doronbehar commented Nov 28, 2020

Diff LGTM, only there should be 2 commits.

@freezeboy
Copy link
Contributor Author

freezeboy commented Nov 28, 2020

1 for the package and 1 for the module and test ?

@doronbehar
Copy link
Contributor

doronbehar commented Nov 28, 2020

Yes.

@doronbehar
Copy link
Contributor

doronbehar commented Nov 28, 2020

cc me when you force push.

@freezeboy
Copy link
Contributor Author

freezeboy commented Nov 28, 2020

cc @doronbehar

aanderse
aanderse requested changes Nov 28, 2020
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@freezeboy
Copy link
Contributor Author

freezeboy commented Nov 28, 2020

@aanderse Sorry, I force pushed this PR and I am not sure which indent problem you have :/

@aanderse
Copy link
Member

aanderse commented Nov 28, 2020

@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?
Screenshot_20201128_121811

@freezeboy
Copy link
Contributor Author

freezeboy commented Nov 28, 2020

Oh my bad ... It's ugly, didn't notice the tabs here

@freezeboy freezeboy force-pushed the add-n8n branch 2 times, most recently from 7516158 to 0591ee0 Compare November 28, 2020 17:24
aanderse
aanderse requested changes Nov 28, 2020
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have reviewed the module portion of this PR. I hope you find the review helpful. Please don't hesitate to ask if you have any questions.

nixos/modules/services/misc/n8n.nix Outdated
Copy link
Member

@aanderse aanderse Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@freezeboy freezeboy Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem

Copy link
Member

@aanderse aanderse Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed anymore.

nixos/modules/services/misc/n8n.nix Outdated
Copy link
Member

@aanderse aanderse Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

@aanderse aanderse Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the n8n package has no build options and only a single version in nixpkgs: what is the value this option provides?

Copy link
Contributor Author

@freezeboy freezeboy Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just to make it easy to use a custom version ?

Copy link
Member

@aanderse aanderse Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what overrides are for.

@freezeboy
Copy link
Contributor Author

freezeboy commented Nov 28, 2020

@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

aanderse
aanderse reviewed Nov 28, 2020
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently I lost a comment from my review.

nixos/modules/services/misc/n8n.nix Outdated
Copy link
Member

@aanderse aanderse Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you looked at RFC42 yet?

Copy link
Contributor Author

@freezeboy freezeboy Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, added to the list

Copy link
Contributor Author

@freezeboy freezeboy Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@aanderse aanderse Nov 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄

Copy link
Contributor Author

@freezeboy freezeboy Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see in the config part, instead of the options

nixos/modules/services/misc/n8n.nix Outdated
Copy link
Member

@aanderse aanderse Nov 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't needed anymore.

@freezeboy freezeboy changed the title n8n: init 0.87.1 n8n: init 0.95.1 Nov 28, 2020
@freezeboy
Copy link
Contributor Author

freezeboy commented Nov 28, 2020

I guess the hardening blocks the tests, I can't write the config to /var/lib/n8n anymore

@freezeboy freezeboy force-pushed the add-n8n branch 2 times, most recently from 9dabaaf to d056c73 Compare November 28, 2020 22:02
@freezeboy
Copy link
Contributor Author

freezeboy commented Nov 28, 2020

Test passes, thank you for all your reviews, the module is way simpler and safer now.

@freezeboy
Copy link
Contributor Author

freezeboy commented Nov 29, 2020

@aanderse I did some cleanup, I think this time it is ok

aanderse
aanderse approved these changes Nov 29, 2020
View reviewed changes
Copy link
Member

@aanderse aanderse left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 nodePackage changes so we will have to get someone else to review that. Right, @doronbehar has already reviewed the package changes... so I think we're good to merge. Right? 😄

@freezeboy freezeboy changed the title n8n: init 0.95.1 n8n: init 0.96.0 Dec 4, 2020
@doronbehar
Copy link
Contributor

doronbehar commented Dec 5, 2020

@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 init prs with node packages that people wanted to add them to https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/node-packages/node-packages.json - #100606 . Hence, if you are motivated, it'd be much appreciated if you could do something similar. I hope I'd be able to help you with such a PR quick enough so that it won't be blocked due to merge conflicts.

@freezeboy
Copy link
Contributor Author

freezeboy commented Dec 5, 2020

@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

@doronbehar
Copy link
Contributor

doronbehar commented Dec 5, 2020

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.

@doronbehar
Copy link
Contributor

doronbehar commented Dec 5, 2020

Result of nixpkgs-review pr 98234 1

1 package blacklisted:
  • tests.nixos-functions.nixos-test
1 package built:
  • n8n

@doronbehar doronbehar merged commit 956e642 into NixOS:master Dec 5, 2020
@freezeboy freezeboy deleted the add-n8n branch December 5, 2020 12:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@doronbehar doronbehar doronbehar left review comments

@aanderse aanderse aanderse approved these changes

Assignees

No one assigned

Labels

6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@freezeboy @nixos-discourse @doronbehar @aanderse