-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Minor documentation tweaks #2335
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
Conversation
Build size and comparison to main:
|
Kind ping |
Sorry for the delay! I'm away for a couple weeks but I'll try look over this when I can :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reads much better now :)
Not a docker expert so I can't review that too closely but all your reasoning looks sound to me (thanks for the great commit messages!)
The first section explains how to clone the repository, the second how to build Infinitime with the docker image, but the details on actually provisioning the image are at the end, despite this step taking place before the build itself. Move the sections into the order in which the steps should be followed.
`docker build` warns of deprecated syntax: 1 warning found (use docker --debug to expand): - LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format (line 71) Update Dockerfile, removing the deprecated syntax usage.
The docker build section points to another page with instructions on how to clone the repository, but this same page already contains these same instructions in the previous section.
The --user argument attempts to map the uid of the user inside the container to the user in the host. This works if docker is running as root, but is docker is running as the current user, then the uid in the container is mapped to a surrogate uid on the host, and this surrogate user does not have permissions to complete the build process. Clarify that the --user flag is only required when running docker as root. It is also likely not required by users using podman as a docker drop-in replacement, since podman always runs in rootless mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks :)
Reviewer note: will merge in a week or so if no other reviews as this is a doc change only
See commit messages for finer details.