test/system: Ensure that the local temporary Docker registry is ready#1817
Conversation
... before proceeding to avoid potential races. Some changes by Debarshi Ray. containers#1811 containers#1817 Signed-off-by: Rolv Apneseth <rolv.apneseth@gmail.com>
debarshiray
force-pushed
the
wip/rishi/test-system-libs-helpers-ensure-docker-registry-is-ready
branch
from
d484a38 to
fd7a49d
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new helper function _wait_for_docker_registry to wait for the local Docker registry to become ready before attempting to log in. The review feedback suggests executing this helper function using BATS' run and assert_success for consistent error handling, and simplifying the double subshell redirection used to capture stderr from the openssl command.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| "${IMAGES[docker-reg]}" | ||
| assert_success | ||
|
|
||
| _wait_for_docker_registry |
There was a problem hiding this comment.
To ensure robust error handling and maintain consistency with the rest of the _setup_docker_registry function (where all commands are executed using BATS' run and verified with assert_success), we should execute _wait_for_docker_registry using run and assert its success. This guarantees that if the registry fails to reach a ready state, the test suite setup will fail immediately and cleanly with a clear error report.
run _wait_for_docker_registry
assert_success
There was a problem hiding this comment.
This is intentional.
First, using only run and assert_success is an anti-pattern. We shouldn't be doing that.
Second, until recently run --separate-stderr didn't work or had undefined behaviour inside setup_suite() because BATS_TEST_TMPDIR isn't defined. Using run in this function used in setup_suite() takes us closer to using some feature of run that doesn't work.
| error_details="$( (timeout 5s openssl s_client \ | ||
| -CAfile "$DOCKER_REG_CERTS_DIR/domain.crt" \ | ||
| -connect "$DOCKER_REG_URI" </dev/null >/dev/null) 2>&1)" || ret_val="$?" |
There was a problem hiding this comment.
The double subshell ( ... ) used to capture only stderr can be simplified by using standard redirection order 2>&1 >/dev/null. This avoids spawning an extra subshell process while achieving the exact same result.
error_details="$(timeout 5s openssl s_client \
-CAfile "$DOCKER_REG_CERTS_DIR/domain.crt" \
-connect "$DOCKER_REG_URI" </dev/null 2>&1 >/dev/null)" || ret_val="$?"
Rolv-Apneseth
left a comment
Rolv-Apneseth
left a comment
There was a problem hiding this comment.
Some very minor suggestions but LGTM
| -CAfile "$DOCKER_REG_CERTS_DIR/domain.crt" \ | ||
| -connect "$DOCKER_REG_URI" </dev/null >/dev/null) 2>&1)" || ret_val="$?" | ||
|
|
||
| if [ "$ret_val" -eq 0 ]; then |
There was a problem hiding this comment.
Maybe this is a little easier to read as a case statement? Something like
case "$ret_val" in
0)
break
;;
125)
error_message="timeout(1) failed"
break
;;
126)
error_message="Failed to invoke openssl(1) or timeout(1)"
break
;;
127)
error_message="openssl(1) not found"
break
;;
137)
error_message="SIGKILL received by openssl(1) or timeout(1)"
break
;;
esac
WDYT?
| local error_details | ||
| local error_message | ||
| local -i j | ||
| local -i num_of_retries=30 |
There was a problem hiding this comment.
Maybe this should be max_retries, and j could be just retries or num_retries?
2 participants
... before proceeding to avoid potential races.
Some changes by Debarshi Ray.
#1811