-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
run-docker.sh: functionality improvements #21413
base: master
Are you sure you want to change the base?
Conversation
6cf4ae5
to
c50cd8e
Compare
- adding automatic use of sudo when working with docker - improving image docker user setup to access repository from image docker - adding a special flag `--stop` which stops the running of image docker
c50cd8e
to
dec1f13
Compare
@@ -28,13 +28,23 @@ fi | |||
|
|||
USER=builder | |||
|
|||
if [ -n "${TERMUX_DOCKER_USE_SUDO-}" ]; then | |||
if [ $(id -u) -ne 0 ]; then |
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.
You don't need root to run docker
on linux, you can just add your own user to docker
group with sudo
once.
https://docs.docker.com/engine/install/linux-postinstall/
Also check commit bc20ba7 where TERMUX_DOCKER_USE_SUDO
was added.
Normally, run-docker.sh
should error out if current user has not been added to docker
group and is not running with root by checking with [[ " $(id -Gn) " != *" docker "* ]]
.
If you wanna add the root check, then use following. Will have to change shebang to bash
.
CURRENT_USER_ID="$(id -u)"
[[ " $(id -Gn) " == *" docker "* ]] && CURRENT_USER_IN_DOCKER_GROUP="true" || CURRENT_USER_IN_DOCKER_GROUP="false"
if [ -n "${TERMUX_DOCKER_USE_SUDO-}" ]; then
SUDO="sudo"
else
if [[ "$CURRENT_USER_ID" == "0" ]] || [[ "$CURRENT_USER_IN_DOCKER_GROUP" == "true" ]]; then
SUDO=""
else
echo "The current user '$CURRENT_USER_ID' is not in 'docker' group while running 'run-docker.sh'" 1>&2
echo "Either add current user to 'docker' group or run 'run-docker.sh' as 'root' user" 1>&2
echo "Check https://docs.docker.com/engine/install/linux-postinstall/#manage-docker-as-a-non-root-user for more info." 1>&2
exit 1
fi
fi
if [ "$STOP_CONTAINER" = "true" ]; then | ||
echo "Stopping container '$CONTAINER_NAME'..." | ||
$SUDO docker stop $CONTAINER_NAME >/dev/null 2>&1 | ||
$SUDO docker rm $CONTAINER_NAME >/dev/null 2>&1 |
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.
docker rm
will also remove the container entirely and it will then need to be redownloaded to start it again. And such functionality should ideally exist in a separate script instead of in a script named run-docker.sh
. The run-docker.sh
script could first be moved to a termux-docker.sh
script and committed. Then a new script run-docker.sh
can be created that just runs termux-docker.sh start
. The termux-docker.sh
script can then have the actual code for start
, stop
, remove
/prune
commands. This way should preserve git history of the old run-docker.sh
script.
The stop command doesn't need to run any other logic anyways and just needs container name, there is needless mixing of logic going on currently.
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.
docker rm will also remove the container entirely and it will then need to be redownloaded to start it again.
If by redownload you mean downloading the docker image from the repo, that won't happen. This command removes the container that was running, but the original image for the container that is loaded on the computer is not touched.
The stop command doesn't need to run any other logic anyways and just needs container name, there is needless mixing of logic going on currently.
If by mixing logic you mean running docker stop
with docker rm
, then this is done specifically so that you can "reboot" (i.e. start the container again with changes in the environment if there are any) the container.
$SUDO docker exec $DOCKER_TTY $CONTAINER_NAME sudo chown -R $REPO_UID $CONTAINER_HOME_DIR | ||
$SUDO docker exec $DOCKER_TTY $CONTAINER_NAME sudo chown -R $REPO_UID /data | ||
$SUDO docker exec $DOCKER_TTY $CONTAINER_NAME sudo usermod -u $REPO_UID builder | ||
$SUDO docker exec $DOCKER_TTY $CONTAINER_NAME sudo groupmod -g $REPO_GID builder |
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.
@sylirre you should probably review these changes.
Why in
run-docker.sh
was theTERMUX_DOCKER_USE_SUDO
variable used to enable the use ofsudo
?