Skip to content

test/system: use a random port for the local Docker registry #1822

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.

Sign up for GitHub

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

Open
Rolv-Apneseth wants to merge 1 commit into
base: main
Choose a base branch
from
+18 −8
Open

test/system: use a random port for the local Docker registry #1822

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions test/system/libs/helpers.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,12 @@ readonly ROOTLESS_PODMAN_RUNROOT_DIR="$BATS_SUITE_TMPDIR/runroot"
readonly DOCKER_REG_ROOT="$BATS_SUITE_TMPDIR/docker-registry-root"
readonly DOCKER_REG_CERTS_DIR="$BATS_SUITE_TMPDIR/certs"
readonly DOCKER_REG_AUTH_DIR="$BATS_SUITE_TMPDIR/auth"
readonly DOCKER_REG_URI="localhost:50000"
readonly DOCKER_REG_NAME="docker-registry"
# Store the registry URI in a file so that loading this script from different
# test processes will still use the same URI for the same test suite run. The
# contents of the file are set when setting up the local Docker registry.
readonly DOCKER_REG_URI_FILE="$BATS_SUITE_TMPDIR/docker-reg-uri"
DOCKER_REG_URI="$(cat "$DOCKER_REG_URI_FILE" 2>/dev/null || echo "")"

@debarshiray debarshiray Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One quick question: is the || echo "" really needed?

@Rolv-Apneseth Rolv-Apneseth Jul 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought so yeah, we want the error to be ignored (when the file hasn't been created yet). Maybe there's a better way?


# Podman and Toolbx commands to run
readonly TOOLBX="${TOOLBX:-$(command -v toolbox)}"
Expand Down Expand Up @@ -166,12 +170,6 @@ function _setup_docker_registry() {
-out "${DOCKER_REG_CERTS_DIR}"/domain.crt
assert_success

# Add certificate to Podman's trusted certificates (rootless)
run mkdir -p "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"
assert_success
run cp "${DOCKER_REG_CERTS_DIR}"/domain.crt "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"/domain.crt
assert_success

# Create a registry user
# username: user; password: user
run mkdir -p "${DOCKER_REG_AUTH_DIR}"
Expand All @@ -197,13 +195,25 @@ function _setup_docker_registry() {
--env REGISTRY_HTTP_TLS_KEY=/certs/domain.key \
--name "${DOCKER_REG_NAME}" \
--privileged \
--publish 50000:5000 \
--publish 5000 \
--rm \
--volume "${DOCKER_REG_AUTH_DIR}":/auth \
--volume "${DOCKER_REG_CERTS_DIR}":/certs \
"${IMAGES[docker-reg]}"
assert_success

# Determine the randomly assigned host port and persist the DOCKER_REG_URI to a file
local docker_reg_port
docker_reg_port="$(podman --root "${DOCKER_REG_ROOT}" port "${DOCKER_REG_NAME}" 5000)"
DOCKER_REG_URI="localhost:${docker_reg_port##*:}"
echo "$DOCKER_REG_URI" > "$DOCKER_REG_URI_FILE"

# Add certificate to Podman's trusted certificates (rootless)
run mkdir -p "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"
assert_success
run cp "${DOCKER_REG_CERTS_DIR}"/domain.crt "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"/domain.crt
assert_success
Comment on lines +212 to +215

@gemini-code-assist gemini-code-assist Bot Jul 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

According to the repository's general rules, using run followed immediately by assert_success is an anti-pattern in BATS. Additionally, run should not be used inside setup_suite() or functions called by it (such as _setup_docker_registry()), because BATS variables like BATS_TEST_TMPDIR are not defined in that context, which can lead to undefined behavior. These commands should be executed directly instead.

  mkdir -p "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"
  cp "${DOCKER_REG_CERTS_DIR}"/domain.crt "$HOME"/.config/containers/certs.d/"${DOCKER_REG_URI}"/domain.crt
References
  1. In BATS (Bash Automated Testing System), avoid using 'run' followed immediately by 'assert_success' as it is considered an anti-pattern. Additionally, avoid using 'run' inside 'setup_suite()' or functions called by it, because BATS variables like 'BATS_TEST_TMPDIR' are not defined in that context, which can lead to undefined behavior.

@Rolv-Apneseth Rolv-Apneseth Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's pre-existing code that I just moved, but that makes sense, I could make that change

@debarshiray debarshiray Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, @Rolv-Apneseth is right. These uses of run already existed in the code. I took the liberty to remove some of these in #1825

I couldn't remove all of them because I was running out of time testing all the error paths. Feel free to submit a pull request removing the rest. :)


_wait_for_docker_registry

run podman login \
Expand Down
Loading