Version 0.15.4 #2096
Version 0.15.4 #2096
Conversation
* Feature: add indexers site hyperlink * Fix: add an option taget on settings, change color to grey
* feat: add Proxmox Uptime View * fix: Proxmox Uptime * fix: Proxmox Uptime * fix: add env.example, make formattedUptime a constant * fix: Uptime * fix: Implement dayjs * fix: removed unused import * fix: Uptime
* fix: removing a category hides items from the wrapper below * refactor: improve variable names
|
I think the problem is the Dockerfile updates from #2011 |
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
There's a lot wrong with that PRs contributions. Including very concerning security risks that got through review. I would encourage addressing that promptly. |
Hi, why don't you submit a PR then? We would be happy to review and merge. |
|
@manuel-rw maybe it makes the most sense to just revert all changes made with that pull request for docker, don't have the PUID / PGID feature for now and just have it after we release Newmarr. Then we can release this version with confidence and don't have to deal with code that we don't really understand. |
There was a problem hiding this comment.
I just realized that this PR is a dev => master series of commits, rather than a typical PR 😓 (I did not notice the default branch for the project was dev, assumed a different git flow)
Hope the feedback is somewhat helpful regarding Docker at least 👍
If you do choose to go ahead and merge, at least remove the docker-cli package. But for the most part, reverting seems better, but I'd still drop the VOLUME directives.
Dockerfile
Outdated
| @@ -1,33 +1,106 @@ | |||
| FROM node:20.2.0-slim | |||
| FROM --platform=linux/amd64 node:20.2.0-slim as compiler | |||
There was a problem hiding this comment.
--platform here forces the builder node to be AMD64. If using buildx/buildkit to build on say ARM64 host, if it's got an AMD64 node via emulation it'll still use that. Probably redundant?
If you want to build with the native platform on the build host, you can instead use --platform=${BUILDPLATFORM}, which will be like building the image without --platform at all, unless you specify other non-native build targets.
For additional clarity, this is the requested platform for the image base that the directives that follow will run on. For projects like Rust, you may have a builder stage that can use the same common native platform but build the project for other architectures (where they then get copied to a platform specific image in a separate stage) which is faster than having one platform emulated for a build if no native build nodes are configured for other architectures.
Dockerfile
Outdated
| RUN yarn build | ||
|
|
||
|
|
||
| FROM node:20.2.0-alpine3.18 |
There was a problem hiding this comment.
Careful with Alpine as a base image. Ensure that you're actually getting worthwhile benefits by using it, it's often a source of troubleshooting gotchas that are not fun to debug (memory leaks, poor performance, DNS, glibc vs musl, are all known issues projects have had with Alpine).
It's typically chosen for the smaller size and attribution of that being more secure as a smaller attack surface. Just be sure you're not giving too much of a trade-off with that choice when you can get fairly competitive sized images by using alternatives when this matters.
Fedora for example can install a minimal image with dnf --installroot, although for NodeJS that is 124MB, Alpine is about half of that. For context:
node:slimdebian image you're using in the build stage is 217 MB.node:alpineis 154MB.
So Fedora is probably a win for you regardless size wise, while the --installroot approach would minimize deps (no package manager, minimal deps).
Similar to Alpine, fedora releases are a bit more frequently than Debian, but Fedora releases do get updates to packages over time too (vs Debian releases which besides age don't tend to upgrade packages much between releases beyond security fixes). Whereas your node images have the benefit of being pinned (you can pin the package with dnf too).
The Docker image support isn't that critical to the project. Since you're already familiar with Debian / apt, perhaps stay with that and optionally take advantage of the image suggestion below (Debian too) as the final stage.
That said you could try Google's debian based distroless NodeJS image too:
docker run --rm -it \ -v /var/run/docker.sock:/var/run/docker.sock \ wagoodman/dive:latest gcr.io/distroless/nodejs22-debian12:latest
It weighs in at 143MB and as you can see above is quite minimal for internal contents. This is for a final stage image only that you COPY your build into as there is no other programs to run, nor package manager available.
For security benefits that should serve you much better and keep things fairly simple.
Dockerfile
Outdated
| ARG PORT=7575 | ||
|
|
||
| # Keep free id >= 1000 for user, under node:x image by default node user uses 1000:1000 |
There was a problem hiding this comment.
Bulk of this I commented on from the original PR, and discourage the approach taken.
There is a feature with Docker that should arrive in future for the concern with better mapping container writes to volume mounts with different UID/GID. Usually the issue for users is just inconvenience if they want to interact with the files from the host, as otherwise they could keep it in a named data volume which avoids some issues (docker cp lets you easily transfer between host and container too).
I haven't looked at the issue that motivated these changes with PUID/PGID, but I assume it's not actually breaking functionality.
Dockerfile
Outdated
| RUN echo '#!/bin/bash\nnode /app/cli/cli.js "$@"' > /usr/bin/homarr | ||
| RUN chmod +x /usr/bin/homarr |
There was a problem hiding this comment.
Better to use an ENTRYPOINT ["/path/to/node", "/app/cli/cli.js"] instead, the "$@" is unnecessary as you get this functionality from ENTRYPOINT with the syntax shown, it will append CMD to it, or whatever a user provides as an override to CMD at runtime.
If you use the distroless image suggestion, you don't have to worry about the ENTRYPOINT, it'll point to node already, but they suggest to put the args into CMD, which may be a slight breaking change to the image if someone was providing custom args to cli.js.
Dockerfile
Outdated
| VOLUME [ "/app/data/configs" ] | ||
| VOLUME [ "/data" ] |
There was a problem hiding this comment.
Since there was talking about reverting such changes. I do want to point out that you should avoid VOLUME directives here.
They create anonymous data volumes per container instance. Those will accumulate with each container created unless the container is disposed of (eg: docker run --rm ... instead of just docker run ...).
It is worse for Docker Compose which will be more common in usage, as destroying containers does not remove these anonymous volumes they're tied to the project and service name, thus if you did for some reason want to rely on this implicit persistence it's more difficult to reset/discard it should the need arise (docker volumes ls is really unhelpful for identifying which anonymous volume belongs to what container).
Instead it's much better to just document these locations, either in proper docs or a compose.yaml example (don't use docker-compose.yml btw, official Docker docs are aligned with compose.yaml since 2023Q3 with Compose v2 release being deemed stable).
Users that want to persist data should provide explicit config to indicate that, otherwise so long as a container is not destroyed it will persist any filesystem state it has when stopped/restarted (sometimes a source of bugs), whereas VOLUME tends to just cause more problems than benefits.
If I could afford the time I would, but I tend to have a habit of underestimating time it takes me. I'm presently juggling too much as-is to take on more atm. Happy to provide review/feedback, but that's about all I can spare right now. |
Thank you for taking the time to write a detailed feedback on this PR. |
No description provided.