Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

N8n r2 #761

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

N8n r2 #761

wants to merge 8 commits into from

Conversation

pegr69
Copy link

@pegr69 pegr69 commented Mar 31, 2024

Working version of N8N with some docs.

@Paraphraser
Copy link

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:

  • The user doesn't need to "just know" to also add the RDBMS container when s/he selects N8N.
  • It's a private RDBMS instance so, even if the user also selects Postgres in the menu and starts experimenting, there's less risk of interference. There's also no risk of the user making a mess of whatever they're working on, deleting the persistent store so they get a clean slate, while completely forgetting that N8N is also using the same Postgres instance.
  • Come the day when the user has more than one user-facing container (like N8N) using the same RDBMS conceptually, having distinct instances of the same RDBMS means there's no risk of conflicts.

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:

  • Because a bundled RDBMS is only accessible over the private nextcloud network, it doesn't need to expose any ports, and is a little bit more secure as a result.

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.

Paraphraser added a commit to Paraphraser/IOTstack that referenced this pull request Apr 2, 2024
Following on from discussion in SensorsIot#761, this adds quotes to port mappings
as recommended in docker-compose
[documentation](https://docs.docker.com/compose/compose-file/05-services/#short-syntax-3):

> HOST:CONTAINER should always be specified as a (quoted) string, to
avoid conflicts with
[yaml base-60 float](https://yaml.org/type/float.html)

Signed-off-by: Phill Kelley <[email protected]>
Paraphraser added a commit to Paraphraser/IOTstack that referenced this pull request Apr 2, 2024
Following on from discussion in SensorsIot#761, this adds quotes to port mappings
as recommended in docker-compose
[documentation](https://docs.docker.com/compose/compose-file/05-services/#short-syntax-3):

> HOST:CONTAINER should always be specified as a (quoted) string, to
avoid conflicts with
[yaml base-60 float](https://yaml.org/type/float.html)

Signed-off-by: Phill Kelley <[email protected]>
Paraphraser added a commit to Paraphraser/IOTstack that referenced this pull request Apr 2, 2024
Expands discussion on hardware serial and Bluetooth devices. Follows
on from SensorsIot#690 and includes more information about how to enable the
devices under Bookworm.

Research triggered by comments appended to SensorsIot#761.

Signed-off-by: Phill Kelley <[email protected]>
@Slyke
Copy link
Collaborator

Slyke commented Apr 30, 2024

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.

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.

3 participants