refactor(IDX): simplify build-image.sh and container-run.sh #8427
Conversation
nmattia
left a comment
nmattia
left a comment
There was a problem hiding this comment.
Thanks a lot! Really nice to see someone give these scripts some love :)
ci/container/build-image.sh
Outdated
| echo " otherwise will choose based on detected environment)" | ||
| echo "" | ||
| echo "Advanced:" | ||
| echo " Set the DOCKER_BUILD_ARGS environment variable to pass additional" |
There was a problem hiding this comment.
would it make sense to have DOCKER_CMD or simply DOCKER as an env var (instance of --container-cmd) to stay consistent?
There was a problem hiding this comment.
Maybe I'll make docker build args into an input as well - I agree it's confusing have the two formats.
| if ! sudo podman "${PODMAN_ARGS[@]}" image exists $IMAGE; then | ||
| if ! sudo podman "${PODMAN_ARGS[@]}" pull $IMAGE; then | ||
| # fallback to building the image | ||
| docker() { | ||
| # Preserve "${PODMAN_ARGS[@]}" in the exported function by passing | ||
| # them through a single variable, and unpacking them here. | ||
| PODMAN_ARGS=(${PODMAN_ARGS}) | ||
| sudo podman "${PODMAN_ARGS[@]}" "$@" --network=host | ||
| } | ||
| export -f docker | ||
| PODMAN_ARGS="${PODMAN_ARGS[@]}" "$REPO_ROOT"/ci/container/build-image.sh | ||
| unset -f docker | ||
| fi | ||
|
|
||
| if [ "${REBUILD_IMAGE:-false}" = true ]; then | ||
| "$REPO_ROOT"/ci/container/build-image.sh | ||
| elif $PODMAN_CMD "${ARGS[@]}" "${PODMAN_ARGS[@]}" image exists "$IMAGE"; then | ||
| $PODMAN_CMD "${ARGS[@]}" pull "$IMAGE" | ||
| else | ||
| "$REPO_ROOT"/ci/container/build-image.sh | ||
| fi |
There was a problem hiding this comment.
in my opinion the previous behavior was much less surprising: whenever you run container-run.sh, you'd get the latest image (whether you download it or build it) without having to think twice.
There was a problem hiding this comment.
Actually you're right, there was a small logic error. I fixed it now.
ci/container/container-run.sh
Outdated
| ARGS=() | ||
| DEVENV=false | ||
| fi | ||
| PODMAN_CMD="sudo podman" |
There was a problem hiding this comment.
out of curiosity, did you figure out why sudo is required? I guess --privileged requires it?
There was a problem hiding this comment.
Yeah it's required to access the hoststorage drive.
ci/container/container-run.sh
Outdated
|
|
||
| if findmnt /hoststorage >/dev/null; then | ||
| PODMAN_ARGS=(--root /hoststorage/podman-root) | ||
| # Detect if we're running in a Devenv environment |
There was a problem hiding this comment.
discussed offline, but FTR: I think it would be good to have a script that's independent of the devenv, and have a devenv-specific wrapper that calls said script. That would separate the concerns better (running the container vs developing in the devenv incl. image GC etc)
There was a problem hiding this comment.
Hey I agree, but would prefer to save that for another PR.
| other_args="--pids-limit=-1 -i $tty_arg --log-driver=none --rm --privileged --network=host --cgroupns=host" | ||
|
|
||
| # option to pass in another shell if desired | ||
| if [ $# -eq 0 ]; then |
There was a problem hiding this comment.
to make sure I understand: if no arg is set, then it drops you into an interactive shell with USHELL (defaulting to bash), and OTOH if some args are passed it interprets them as a command and executes said command, correct?
There was a problem hiding this comment.
No, the behavior is this:
- If no command is passed in CMD will be set to the env var $USHELL. If $USHELL is not set, it will default to
/usr/bin/bash - If a command is passed in it will use that command.
2 participants
This refactors
build-image.shandrun-container.shto simplify them a little. Here are the changes that were made:build-image.sh:container-run.sh:/usr/bin/bashand creates a more reproducible environment. It's still possible for the user to pass in their own command with a shell specified.