cloud run support#70
Conversation
Pull Request Review SummaryGreat work on adding integration tests for the I have left a few actionable suggestions to improve the robustness of the test script:
Once these minor improvements are addressed, this PR is ready to merge! 🤖 Reviewed by codereviewbot.ai |
| # Prepend mock gcloud to PATH | ||
| export PATH="${tmp_dir}:${PATH}" | ||
|
|
||
| # 1. Execute the apply target executable |
There was a problem hiding this comment.
Since set -e is not set in this script, if ${apply_bin} or ${CRANE_BIN} validate fails, the script will continue executing and might incorrectly report a successful test run.
We should explicitly check their exit statuses:
| # 1. Execute the apply target executable | |
| # 1. Execute the apply target executable | |
| if ! ${apply_bin}; then | |
| echo "Error: Apply target failed" | |
| exit 1 | |
| fi | |
| # Verify that the image is successfully pushed using crane | |
| if ! ${CRANE_BIN} validate -v --fast --remote ${expected_image_ref}; then | |
| echo "Error: Crane validation failed" | |
| exit 1 | |
| fi |
| exit 1 | ||
| fi | ||
|
|
||
| # 2. Execute the delete target executable |
There was a problem hiding this comment.
Similarly, we should check the exit status of ${delete_bin} to ensure that any failures during deletion are caught and fail the test.
| # 2. Execute the delete target executable | |
| # 2. Execute the delete target executable | |
| if ! ${delete_bin}; then | |
| echo "Error: Delete target failed" | |
| exit 1 | |
| fi |
| # 2. Execute the delete target executable | ||
| ${delete_bin} | ||
|
|
||
| # Verify gcloud delete arguments |
There was a problem hiding this comment.
Instead of using the unquoted scalar variable ${expected_services} which undergoes word splitting, we can loop directly over the positional arguments starting from index 7 ("${@:7}"). This is safer and more idiomatic in bash.
| # Verify gcloud delete arguments | |
| # Verify gcloud delete arguments | |
| for service in "${@:7}"; do |
|
|
||
| # Create a temporary directory for the mock gcloud and manifest dump | ||
| tmp_dir=$(mktemp -d) | ||
| trap "rm -rf $tmp_dir; kill -9 $registry_pid" EXIT |
There was a problem hiding this comment.
(Nit) If the registry process has already exited by the time the trap runs, kill -9 will print a noisy "No such process" error to stderr. We can redirect stderr to /dev/null to keep the test output clean.
| trap "rm -rf $tmp_dir; kill -9 $registry_pid" EXIT | |
| trap "rm -rf $tmp_dir; kill -9 $registry_pid 2>/dev/null" EXIT |
No description provided.