linuxserver / docker-unifi-network-application

GNU General Public License v3.0
723 stars 54 forks source link

[BUG] Missing required ENV vars doesn't alert you and fails in unexpected ways #89

Closed timwhite closed 5 months ago

timwhite commented 6 months ago

Is there an existing issue for this?

Current Behavior

It's not currently clear which ENV vars are required, and which are optional, and some missing required ENV vars will alert you, but others will not, leading to container startup failing. Some ENV vars could have sensible defaults and be optional. Most docker containers do this, and so this container not following that same behavior is confusing.

If MONGO_PORT is empty at startup, the message about not being able to connect to the database host isn't helpful, as it only shows the host, and not the port it's trying to connect to. This makes it appear to be using the default port, but instead no port is being used.

Expected Behavior

Missing required ENV vars would show an error message. https://github.com/linuxserver/docker-unifi-network-application/blob/main/root/etc/s6-overlay/s6-rc.d/init-unifi-network-application-config/run currently shows an error message if MONGO_HOST is empty, but not if MONGO_PORT is empty.

There are other required env vars that no error message is shown for, and it's not clear until you read further down in the readme that there are optional vars. It's not clear if PUID, PGID, and TZ are required or optional either.

Some sensible defaults would help.

Steps To Reproduce

  1. Start a container with MONGO_PORT undefined.
  2. Get the error message *** Defined MONGO_HOST ${MONGO_HOST} is not reachable, cannot proceed. *** which shows nothing about MONGO_PORT being empty

Environment

- OS: Debian Bookworm
- How docker service was installed: K3s

CPU architecture

x86-64

Docker creation

N/A (K3s manifests)

Container logs

*** Defined MONGO_HOST ${MONGO_HOST} is not reachable, cannot proceed. ***
github-actions[bot] commented 6 months ago

Thanks for opening your first issue here! Be sure to follow the relevant issue templates, or risk having this issue marked as invalid.

thespad commented 6 months ago

I would have thought that all the envs marked "optional" would imply that the others are not optional.

Screenshot_20240524-083320.png

timwhite commented 6 months ago

It's not common for TZ, PUID, PGID to be required env vars (at least in other docker containers outside the linuxserver system), having sensible defaults. Likewise, where there are sensible defaults (like port numbers), these are normally set.

Either way, we currently show an error message if MONGO_HOST isn't set, but not if the port isn't set. We blindly try to connect to a null port, and when it times out, give a less than helpful error message.

If they are not optional ENV vars, we should detect if they aren't set (or are empty) and show a helpful error message. Many images have defaults like port numbers set, and you only override them if needed. I'm sure I'm not the only one to waste hours trying to work out why the networking between my Mongo container and Unifi wasn't working, downgrading Mongo versions based on outdated comments, only to discover the error message was hiding part of the truth, that we don't have a default port set, and we don't show a warning when it's not set.

thespad commented 6 months ago

TZ is a generic Linux environment variable and applicable to the majority of containers that exist, whether they document it or not. It defaults to UTC, which is about as sensible a default as is possible. PUID and PGID default to 911 which is broadly speaking going to be a UID that doesn't clash with any common existing user or service, to avoid causing unexpected problems with host permissions. We have a general expectation that users will read our documentation for an image and understand what options need to be set for it to work.

We could set a default port if it's not provided by the user - you are correct that it often assumed - but again the port env is marked as non-optional and the examples we provide all include the default port, so even if you blindly copy/pasted you have it set correctly in most cases. I'll look at having it default to 27017 if nothing is provided by the user on the basis that it's the only connection variable we can reasonably attempt to make assumptions about.

As for error handling, we're not going to write traps for every env in every image because it would be phenomenal amount of work for very little gain, most don't have any at all. The only time we tend to do it is when missing a critical env will prevent startup entirely, or endlessly loop errors, and we need to make it clear to users that they've not set it.

LinuxServer-CI commented 5 months ago

This issue has been automatically marked as stale because it has not had recent activity. This might be due to missing feedback from OP. It will be closed if no further activity occurs. Thank you for your contributions.

drizuid commented 5 months ago

I feel like missing required things causing issues is not unexpected at all; we strongly encourage our users to read the readme and follow it. Optional items are noted as being optional. Closing this.