SensorsIot / IOTstack

Docker stack for getting started on IOT on the Raspberry PI
GNU General Public License v3.0
1.42k stars 303 forks source link

N8n r2 #761

Open pegr69 opened 3 months ago

pegr69 commented 3 months ago

Working version of N8N with some docs.

Paraphraser commented 3 months ago

Hi Peter - I have a couple of comments. All this is just my 2¢. I'm just a garden-variety user of and occasional contributor to this repo so please feel free to ignore everything I say.

Apologies-in-advance for this turning out to be a fair bit longer than I had in mind when I started.

port-pairing

I note that you've dropped the quotes around the port mapping. Please see ports in the docker-compose documentation. Rather than try to figure out whether any particular port-pairing is at risk of misinterpretation, IOTstack tries to follow that advice.

Truth to tell, I just did a quick scan of the existing templates where I found a few more quote-less examples (dashmachine and heimdall and, in old-menu, those plus homer, portainer agent and qbittorrent). I'll put in PRs to fix those.

postgres

I've never tried to use N8N but it looks to me like the original service definition assumed MariaDB while you're proposing a switch to Postgres. Yes?

There's no implied value-judgement in that question. I'm just trying to make sure I haven't misunderstood your intention. I'm completely agnostic when it comes to RDBMS (although I'm quite partial to SQLite).

Whether we're talking MariaDB (previously) or Postgres (your PR), it looks to me like there's an implied assumption that the user selecting N8N in the menu will also be expected to select the corresponding RDBMS package in the menu. Yes?

Assuming "yes" and "yes", could I perhaps invite you to study the existing NextCloud service definition, and also #759 which I've just submitted to add a WordPress container.

In each case, the RDBMS is bundled with the headline service definition. There's no reason why a private instance of Postgres can't be bundled with N8N in the same fashion. Key benefits:

If you think this sounds like a good idea, can you please read the bit of the WordPress doco (added as part of #759) with the heading "About MariaDB" so you understand that nextcloud is the correct network name for the back-end private network, irrespective of the headline container.

And that brings me to another benefit:

build.py

With all due respect to Slyke who slaved his guts out migrating the old-menu build.sh and directoryfix.sh functionality to build.py (and adding password-generation along the way), the approach still suffers from the fundamental weakness that it depends on the menu, particularly when it comes to "fixups" like copying files into place, or setting ownership or permissions.

In my view, fixups are best handled by using a local Dockerfile (Mosquitto is a straightforward example of a Dockerfile adding self-repair to an existing image). There's a bit more on this in #331.

While the NextCloud service definition still relies on build.py, I'm proposing an alternative scheme in #759 which relies on writing passwords (and related variables) into ~/IOTstack/.env. This builds on similar work done for devices: clauses. See the Zigbee2MQTT service definition for one example.

Basically, between using Dockerfiles to add self-repair and having variable substitutions handled by docker-compose (including prompting for omissions where defaults are not appropriate), I believe we can at least reduce and perhaps totally avoid the need for build.py/build.sh or similar edifices. That will simplify both adding new containers and maintaining existing containers.

It also means service definitions will "just work" whether they're added via the menu or concatenated onto compose files by hand. By "just work" I mean that, best case, the container will always at least start; and worst case, it will at least point the user to what needs to be done to get the container to start. For me, the opposite of "just work" is a container silently failing and going into a restart loop giving the user little-to-no hints as to why.

ttyAMA0

Although the variable is commented-out in the N8N service definition (and you didn't actually change it in your PR), I found myself wondering about this line:

# - USBDEVICES=/dev/ttyAMA0

There's background to what I'm about to say at #690. Once you've read it, you'll appreciate (if you didn't know it already) that what ttyAMA0 means depends on both history and platform. I mention "platform" because IOTstack is deployed on non-Pi hardware (eg Intel running Debian natively or as a guest in Proxmox-VE) where ttyAMA0 has never had any meaning.

Since #690 was written, Raspberry Pi OS has advanced to Bookworm and it's completely different to what was being foreshadowed at the time. By default, there is no /dev/serial0, no /dev/ttyS0 and no /dev/ttyAMA0. Getting the first two needs enable_uart=1 in config.txt; getting the second needs dtoverlay=pi3-disable-bt as well. The notion of /dev/serial1 never seems to come up.

Anyway, as a non-user of N8N speaking to a user of N8N:

  1. What do you think ttyAMA0 means in the N8N context? Serial port? Bluetooth?
  2. Should it have a different name so its purpose is clear, or should the USBDEVICES variable be omitted entirely?
  3. Should any of this be documented?

For Q3 I'm thinking of something like Node-RED Serial Port Access which explains that, to be accessible inside the container, the device will have to be referenced in some manner via devices: or device_cgroup_rules clauses. That would also be true of N8N, irrespective of your answers to Q1 and Q2.

Slyke commented 2 months ago

Hey @pegr69 Is there a Dockerhub hosted instance of this where we can see what's in the built images? It seems like n8n is more targeted towards Enterprise and Businesses, with the DevOps, CICD CRM etc tools it offers instead of selfhosted IoT type setups. It doesn't appear to support plugins, but they have extensive integrations with a bunch of systems. I need to go through the licensing agreement too, most of the services on IoTStack use MIT, GNU etc.