fix: revert pgid puid docker changes

[Meierschlumpf]

This pull request reverts the changes of #2011
Details for maintainers can be found here

[github-actions]

Coverage Report

Status	Category	Percentage	Covered / Total
🔵	Lines	17.22%	5620 / 32633
🔵	Statements	17.22%	5620 / 32633
🔵	Functions	6.37%	33 / 518
🔵	Branches	37.22%	134 / 360

File Coverage

No changed files found.

Generated in workflow #6861

[polarathene]

Not sure if you want to apply these via separate PR, but here are some suggestions to consider.

At the very least I would avoid bringing back VOLUME.

[polarathene]

As per #2096 (comment)

Suggested change

	VOLUME [ "/app/data/configs" ]
	VOLUME [ "/data" ]

[polarathene]

This script has a shebang and was given the executable earlier with chmod +x, you can call it directly.

Suggested change

	ENTRYPOINT ["sh", "./scripts/run.sh"]
	ENTRYPOINT ["/app/scripts/run.sh"]

Regarding the script itself, I'm not quite sure if you need to run the two commands in the background and wait on them since you seem to be using it synchronously/blocking that seems a bit redundant?:

homarr/scripts/run.sh

Lines 14 to 17 in eba3010

	echo "Starting production server..."
	node /app/server.js & PID=$!
	wait $PID

[polarathene]

URL is a bit of an odd name btw, given that this isn't a file:// URI but a prefix that you remove to then use the absolute/relative path after :. Had a double take when I saw that at first due to :/ instead of :// 😅

[polarathene]

I understand that this is meant to be a convenience for homarr as a command if it were run in the container, instead of node /app/cli/cli.js.

I haven't searched git blame or your project history for relevance of what motivated this for the image, but I doubt anyone has been relying on it as you're just writing a fixed single line string here.

The \n would only create a new line if you used echo -e. So until someone actually wants the convenience of a homarr command, perhaps drop this? (since no one can actually run it, not a breaking change either)

Suggested change

	RUN echo '#!/bin/bash\nnode /app/cli/cli.js "$@"' > /usr/bin/homarr
	RUN chmod +x /usr/bin/homarr

[polarathene]

EXPOSE is typically more for documentation really. Users will explicitly publish ports for a service themselves.

I don't know why you set an ENV here, there is a ARG PORT early on the file that seems redundant. ARG is for build-time ENV only, while ENV will carry into the actual image.

You do set ENV PORT 7575 further down, but unless it's used for anything other than the entrypoint script, you don't need that either, since you already provide the 7575 fallback in your run.sh.

Suggested change

	# Expose the default application port
	EXPOSE $PORT
	ENV PORT=${PORT}
	# Expose the default application port
	EXPOSE 7575

Also if you're not that familiar with Docker and ports, you have your actual app/service listening on a port, and any other container on the same network can reach that port, similar to if running outside of a container. To make it reachable from the host system or external hosts connecting to the system, ports can be published which maps a port the user chooses to the internal container port, such as 80:7575. You only care about the 7575 internally, except if it's relevant to a URL a client would use (like a browser, which would connect via :80 in this case).

[polarathene]

If this has no relevance of having a default, why even set it in Dockerfile?

Just so we're clear ENV is configuring an environment variable with a value. If you don't actually need that in the image by default, because Homarr would make similar assumptions at runtime anyway when an ENV doesn't exist, then this doesn't add much value. Users would just refer to any common ENV docs for config.

[manuel-rw]

Thanks again for the feedback @polarathene . We are going to apply these suggestions for 1.0 but not for the next version of the old code. This is to ensure that existing setups don't break.

[polarathene]

The bulk of the feedback provided wouldn't have broken anything.

• VOLUME is anonymous volumes, they're not reliable for persistence. If anything the cause bugs that waste time troubleshooting.
• ENTRYPOINT feedback was to remove redundant noise, no change in functionality.
• ENV feedback was again removing noise that didn't appear to serve any practical purpose.
• homarr executable was broken since contributed, so dropping that wouldn't have broken anyones setup either.

Basically only my feedback with the DB URL would have been a breaking change, it wasn't something actionable in this PR.

Ah well.. you merged and released anyway 😓