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

✨ Environment variable: PUID and PGID (#240) #2011

Merged
merged 3 commits into from May 23, 2024
Merged

Conversation

nyok1912
Copy link
Contributor

This PR fixes #240

Category

Feature

Issue Number

Related issue: #240

New Vars

PUID and PGID

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi 👋. Thank you for making your first contribution to Homarr. Please ensure that you've completed all the points in the TODO checklist. We'll review your changes shortly.

Copy link
Owner

@ajnart ajnart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure this works without any repercussions ?
I'm pretty sure I or we have tried to implement this previously by using the base image of a famous set of Images (hotio base image)

We had made it so that the CI manually builds before creating the docker images so that it doesn't take like 15 minutes for the ARM VMs to build, and before that they couldn't even build the project because of errors in NextJS.

Can you document the PR more ? I'm not sure what a supervisor and the conf.d files are supposed to do

.env.example Outdated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure these changes need to be committed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change under this file is whitespace removed from end of file.
imagen

This fixes a mysterious issue during building:

You can reproduce this issue from clean scenary:

docker run --rm -it node:20.2.0-slim bash
And then, try to clone and build this project:

apt update && apt -y install git
git clone https://github.com/ajnart/homarr.git
cd homarr
yarn install
cp .env.example .env
yarn build

After that, throw next error caused by end whitespace:

Invalid environment variables: { NEXT_PUBLIC_DEFAULT_COLOR_SCHEME: [ 'Invalid input' ] }
- error Failed to load next.config.js, see more info .....

imagen

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's mindblowing 🤯 Totally didn't expect it could cause so many issues

Comment on lines 9 to 10
#stdout_events_enabled = true
#stderr_events_enabled = true
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why comment this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I forgot to remove this

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not exactly sure what this is

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding to supervisor: Ok, the main user in the container can be root for many reasons (testing, learning, new future services, etc.) but with supervisor you can start homarr with the user "homarr", now you can add more container services easily.
Besides now you can launch container with custom command and homarr works always, for example you can launch homarr shell like that:
docker run --rm -it homarr bash
If you do this currently docker container runs correctly and get container bash shell but homarr will never run, but with supervisor homarr will always run and you can overwrite command without any problem.
The supervisor is very helpful, if you don't like it i can work to remove it.

@ajnart ajnart added the 📝 Needs more info An issue that has too little information label Apr 18, 2024
@nyok1912
Copy link
Contributor Author

Are you sure this works without any repercussions ? I'm pretty sure I or we have tried to implement this previously by using the base image of a famous set of Images (hotio base image)

We had made it so that the CI manually builds before creating the docker images so that it doesn't take like 15 minutes for the ARM VMs to build, and before that they couldn't even build the project because of errors in NextJS.

Can you document the PR more ? I'm not sure what a supervisor and the conf.d files are supposed to do

Must be works without any repercussion because i don't touch any homarr code, only added new features same as keep synced PUID, PGID and DOCKER_PGID.

PUID - Must be same UID on host user, under container this UID will applied to homarr user.
PGID - Must be same GID on host user group, under container this GID will applied to homarr group.
DOCKER_PGID - Is needed for work with host docker without any problem, under container docker GID must be same as host docker GID to interact with host docker.sock bind mount.

Added docker entrypoint:
All sh scripts in the /docker-entrypoint.d folder will be run on every container startup, this allows you to configure the custom details needed on many systems; In this case, I wrote a user configuration script.
00-user-setup.sh – This script ensures that permissions between the host and container remain the same UID and GID when binded mount volumes. It also changes the GID of the docker to maintain the homarr user's permissions to interact with the docker as a binded mount volume. PUID and PGID can be modified any time, this allow easy migrations and changes in host for any reason.

Persistence data is all under /app/data/configs, this must be volumed between host, path binded mount volume based or docker named volumes system. This path always keeps PUID and PGID. I think database too must be save here.

And yes, the Docker build of this image is a bit heavy task because i included homarr build process inside docker build image.
Thereby anyone can do git clone of this repo and build image directly doing:
docker build -t custom_homarr .
Without do anything more, building and all nedded actions was written in multi step Dockerfile.

@Meierschlumpf
Copy link
Collaborator

Meierschlumpf commented Apr 18, 2024

Hi thanks for the pull request, seems a little confusing right now for me, but probably mostly knowledge issues on my side. We prefer to wait for one of our colleagues to come back from vacations so we can discuss how we move forward.

@joecarl
Copy link

joecarl commented Apr 30, 2024

One week ago I was about to open an issue about PUID but then I found that it is a well known issue... So I found this PR and I just wanted to thank the homarr team and @nyok1912 for this PR, this really saved me from a headache and I thought it deserverd the acknowledgement. Thanks!

Copy link
Collaborator

@manuel-rw manuel-rw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have to admit that I don't fully understand every line.
Have you made tests using existing data / setups to ensure that it doesn't break?
Would you also be willing to assist us on the upcoming version 1.0 (which requires a new dockerfile due to changed architecture).

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@nyok1912
Copy link
Contributor Author

nyok1912 commented May 2, 2024

I have to admit that I don't fully understand every line. Have you made tests using existing data / setups to ensure that it doesn't break? Would you also be willing to assist us on the upcoming version 1.0 (which requires a new dockerfile due to changed architecture).

Of course I can help in future versions, I love this project!

@manuel-rw manuel-rw merged commit cb2b28c into ajnart:dev May 23, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📝 Needs more info An issue that has too little information
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docker] Environment variable: PUID and PGID
5 participants