Skip to content

Conversation

i-am-logger
Copy link

having an issue #1628 will appriciate the help.

@i-am-logger i-am-logger force-pushed the logger/n8n branch 5 times, most recently from 13c929f to f5fbdba Compare December 6, 2024 19:38
@i-am-logger
Copy link
Author

working, would appriciate a review

Copy link
Member

@sandydoo sandydoo left a comment

Choose a reason for hiding this comment

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

Thanks, @i-am-logger. I've requested a bunch of minor changes, but otherwise looks good overall.

};

processes.n8n = {
exec = "${pkgs.n8n}/bin/n8n";
Copy link
Member

Choose a reason for hiding this comment

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

It's nice to have a package option in the module to let people override the package used.

@@ -0,0 +1,7 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

To test that it launches, you can add a .test.sh script in this folder and curl a health endpoint of some kind. There are lots of examples of this in the repo.

@@ -0,0 +1,2 @@
{ ... }:
Copy link
Member

Choose a reason for hiding this comment

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

The n8n can be removed in favor of the test.

@sandydoo
Copy link
Member

@i-am-logger, I think quite a few of the requested changes are also applicable to your previous PR. I'll be happy to review it once updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants