Skip to content

cloud run support#70

Open
apesternikov wants to merge 4 commits into
mainfrom
ap/cloud_run
Open

cloud run support#70
apesternikov wants to merge 4 commits into
mainfrom
ap/cloud_run

Conversation

@apesternikov

Copy link
Copy Markdown
Contributor

No description provided.

@codereviewbot-ai

codereviewbot-ai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Pull Request Review Summary

Great work on adding integration tests for the cloudrun_deploy .apply and .delete targets! The mock gcloud approach is clean and effective.

I have left a few actionable suggestions to improve the robustness of the test script:

  1. Check Exit Statuses: Since set -e is not active in the test script, we should explicitly check the exit status of ${apply_bin}, ${CRANE_BIN} validate, and ${delete_bin} to prevent silent test passes on failures.
  2. Idiomatic Bash Loop: Loop directly over the positional arguments "${@:7}" instead of using the unquoted scalar ${expected_services} to avoid potential word-splitting issues.
  3. Clean Logs: Redirect kill -9 stderr to /dev/null in the EXIT trap to avoid noisy "No such process" errors if the registry has already stopped.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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:

Suggested change
# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Similarly, we should check the exit status of ${delete_bin} to ensure that any failures during deletion are caught and fail the test.

Suggested change
# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Suggested change
# 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

(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.

Suggested change
trap "rm -rf $tmp_dir; kill -9 $registry_pid" EXIT
trap "rm -rf $tmp_dir; kill -9 $registry_pid 2>/dev/null" EXIT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant