feat: Feature parity with Rust launcher #2
Conversation
internal/commands/launch.go
Outdated
| cfgNum := len(cfg.TaskRunners) | ||
|
|
||
| if cfgNum == 1 { | ||
| log.Print("Loaded config file loaded with a single runner config") |
There was a problem hiding this comment.
Logger writes to stderr by default, which is not what we would want here. Also could we enable these logs only if certain env var is set, similar to RUST_LOG in the rust launcher
There was a problem hiding this comment.
Also could we enable these logs only if certain env var is set, similar to RUST_LOG in the rust launcher
Why do we want to allow hiding the launcher logs by default? n8n main, worker, webhook, etc. all show at least info logs by default. As a user I'd like to always have a few informative lines to know the runner is working as expected.
There was a problem hiding this comment.
I would consider most of these logs to be debug logs, like for example the loading of the config here. Sure we can keep one or two to know the runner is working, (tho it can also be interpreted from the log from n8n that runner has connected), but we don't need all of these. This would be in quite contrast compared to what we log in the n8n app itself, which is very minimal
There was a problem hiding this comment.
Got it, let's do this division of info/debug logs as a follow up 👍🏻
|
Thanks for the feedback! 🙏🏻 Addressed, let me know what you think |
cmd/launcher/main.go
Outdated
| ) | ||
|
|
||
| func main() { | ||
| if len(os.Args) < 2 { | ||
| log.Fatal("Missing argument. Expected `launch` or `kill` subcommand") | ||
| logs.Logger.Fatal("Missing argument. Expected `launch` or `kill` subcommand") |
There was a problem hiding this comment.
Since this only does "launch" now, we don't need the launch command when starting this. It can do that by default. Instead we could take the runner type as the first arg:
./bin/main javascript| @@ -7,6 +7,8 @@ import ( | |||
| "os" | |||
| ) | |||
|
|
|||
| const configPath = "/etc/n8n-task-runners.json" | |||
There was a problem hiding this comment.
I think it would still be nice to test this locally without having to write to /etc/. We can do that later on tho
https://linear.app/n8n/issue/PAY-2243