fix: externalize production deploy script - #107
Conversation
protostatis
left a comment
There was a problem hiding this comment.
Sky's Code Review
This PR externalizes the ~550-line production cutover from the SSH action's inline script into a versioned deploy/release.sh, leaving a short bootstrap in the workflow to resolve the GitHub 'Exceeded max expression length 21000' rejection. The refactor is directionally sound: it moves secrets out of Docker command arguments into passthrough environment variables (GHCR_TOKEN via docker login --password-stdin, TELEGRAM_BOT_TOKEN via docker run -e KEY passthrough), fixes unquoted variable references, keeps the checkout-then-run ordering correct (git checkout before bash deploy/release.sh so the versioned script self-runs), and adds meaningful regression tests for bootstrap length, expression isolation, and shell parseability. No blocking security or correctness bugs found. Main suggestions: standardize on set -uo pipefail (the current set -e alone can mask pipeline failures in the heartbeat-watermark and disk-space checks), and note that REGISTRY/IMAGE_PREFIX are now silently required by release.sh but still sourced from repo/environment-level vars not visible in this diff. Also worth flagging: GITHUB_TOKEN and TELEGRAM_BOT_TOKEN now sit in the remote shell environ (visible via /proc//environ) for the duration of the SSH session, which is acceptable but a tradeoff to acknowledge, and docker system prune -af remains aggressively destructive.
Verdict: Comment
Comments
- Removing the
actions/checkout@v4step is correct here — the runner-side checkout was never used by the SSH step, which operates on the EC2 host's own git repository and performs its owngit fetch --tags origin+git checkout. - The checkout-then-run ordering (
git checkout "$RELEASE_TAG"followed bybash deploy/release.sh) is exactly right: the freshly checked-out, tag-versioned script self-locates viaBASH_SOURCEand runs against that tag. This achieves the stated goal of versioning the deploy logic. - The regression tests are a good addition, but the length assertion (
< 20_000) does not match the reported 1,289-byte bound and GitHub's documented 21,000 limit; consider asserting against a tighter constant (e.g.< 5_000) so future bootstrap growth toward the real ceiling is caught earlier. - Consider adding a test that asserts
release.shnever embeds${{ secrets./GITHUB_TOKENliterals (the${{guard already covers the expression syntax) to lock in the no-secret-in-script invariant going forward.
Reviewed by Sky — Unchained Sky engineering agent
| #!/usr/bin/env bash | ||
| # Run the fail-closed production cutover after the release tag is checked out. | ||
|
|
||
| set -e |
There was a problem hiding this comment.
Use set -euo pipefail instead of set -e. Without pipefail, a failing upstream command in a pipeline is masked by the last command's exit status. Concretely, the heartbeat-watermark extraction (printf ... | python3 -c ...) and disk-space check (df / | tail -1 | awk '{print $4}') will not abort if docker run or df fails but python3/awk still exits 0.
| docker system prune -af || true | ||
| docker builder prune -af || true | ||
|
|
||
| echo "Post-cleanup disk usage:" |
There was a problem hiding this comment.
AVAIL_KB=$(df / | tail -1 | awk '{print $4}') relies on the last-column field of df, which is fragile across filesystems (varies with SOURCE column width / mount names). Prefer df --output=avail / | tail -1 (GNU coreutils) or df -kP / | awk 'NR==2 {print $4}'. Combined with missing pipefail, a df failure here would silently produce an empty value that fails the [ "$AVAIL_KB" -lt ... ] comparison rather than aborting cleanly.
|
|
||
| # ========== DISK SPACE CHECK & CLEANUP ========== | ||
| echo "Pre-cleanup disk usage:" | ||
| df -h / |
There was a problem hiding this comment.
docker system prune -af removes all unused images and is destructive; if a concurrent process or a prior failed deploy holds the only known-good image tag, it is gone. This behavior is pre-existing (moved verbatim), but since the script is now versioned it may be worth gating the -a flag or preserving the :previous tags before pruning.
| fi | ||
| echo "Disk space OK: ${AVAIL_KB}KB available" | ||
|
|
||
| # ========== DOCKER LOGIN & PULL ========== |
There was a problem hiding this comment.
Good — piping the token via printf '%s' "$GHCR_TOKEN" | docker login --password-stdin keeps it out of argv. Just confirm that docker login itself is not logged with the secret echoed; consider 2>&1 redirection if login noise ends up in the SSH action logs.
| RELEASE_TAG: ${{ github.ref_name }} | ||
| RELEASE_IMAGE_TAG: ${{ needs.build-and-push.outputs.version }} | ||
| GHCR_ACTOR: ${{ github.actor }} | ||
| GHCR_TOKEN: ${{ secrets.GITHUB_TOKEN }} |
There was a problem hiding this comment.
GHCR_TOKEN and TELEGRAM_BOT_TOKEN are now forwarded via the SSH envs list, which places them in the remote shell environment (visible in /proc/<pid>/environ for the lifetime of the SSH session). This is the intended improvement over inline argv, but note it means any process on the EC2 host during the deploy can read the Telegram token. Acceptable, but worth a comment acknowledging the tradeoff vs. a dedicated secrets store.
| username: ${{ secrets.EC2_USERNAME }} | ||
| key: ${{ secrets.EC2_SSH_KEY }} | ||
| command_timeout: 45m | ||
| envs: RELEASE_TAG,RELEASE_IMAGE_TAG,REGISTRY,IMAGE_PREFIX,GHCR_ACTOR,GHCR_TOKEN,TELEGRAM_BOT_TOKEN |
There was a problem hiding this comment.
envs: RELEASE_TAG,RELEASE_IMAGE_TAG,REGISTRY,IMAGE_PREFIX,GHCR_ACTOR,GHCR_TOKEN,TELEGRAM_BOT_TOKEN forwards REGISTRY and IMAGE_PREFIX, but neither is defined in this job's env: block. They must resolve from workflow-level or repository/environment variables; if either is missing, release.sh will now fail at its required-vars guard. Confirm these are defined at a scope this diff doesn't show (the old inline code referenced ${{ env.REGISTRY }} so it likely already is, but the new hard requirement is worth verifying).
|
Review follow-up in d1bd517:
|
protostatis
left a comment
There was a problem hiding this comment.
Sky's Code Review
The PR externalizes a ~550-line deployment cutover from an inline GitHub Actions SSH script into a versioned deploy/release.sh, leaving a short 1,289-character bootstrap in the workflow to stay under GitHub's 21,000-char action-input expression limit. This is a sound remediation for the v1.37.0 expression-length rejection. The refactor also improves secret handling (GHCR_TOKEN/TELEGRAM_BOT_TOKEN now flow through the SSH action env instead of appearing inline in command args) and adds regression tests for bootstrap length, external-script use, and expression isolation. The logic is a faithful extraction of the prior inline script with set -euo pipefail and array-based env-arg handling. I found no production-blocking issues, but several correctness and safety observations merit attention before the v1.37.1 cutover.
Verdict: Comment
Comments
- The removal of
actions/checkout@v4combined with the bootstrap relying ongit fetch --tags originandgit checkout $RELEASE_TAGis correct only because the deployment runs on the remote EC2 worktree (cd /home/ec2-user/crypto_sentiment_crawler), not on the runner. Worth confirming the EC2 worktree has origin/upstream configured for tag fetch, otherwisegit fetch --tags originwill fail and release.sh won't run. - REGISTRY and IMAGE_PREFIX are referenced in release.sh but are absent from the workflow's
envs:list and from theenv:block. The old inline script used${{ env.REGISTRY }}and${{ env.IMAGE_PREFIX }}; ensure these are defined either at workflow/job level (and reachable) or added to the ssh-actionenvsso the required-variable guard doesn't fail-closed on a real deploy. - The
ls -t ... | tail -n +15 | xargs -r rmpruning now uses|| trueat the pipeline end andshellcheck disable=SC2012; thexargs -r rmunder old code was not guarded but also tolerated failures. This is a reasonable hardening, but noteset -o pipefailstill applies to thelspipeline, so the|| trueonly guards the finalrmstage — a missing directory inlscould still trip pipefail. Confirm this is intended.
Reviewed by Sky — Unchained Sky engineering agent
| echo "========== DEPLOYMENT START: ${{ github.ref_name }} ==========" | ||
|
|
||
| set -euo pipefail | ||
| echo "========== DEPLOYMENT BOOTSTRAP: $RELEASE_TAG ==========" |
There was a problem hiding this comment.
The bootstrap drops the explicit set -euo pipefail failure semantics on several critical checks. The old inline script wrapped the direct-egress probes in if ! ... ; then echo ERROR; exit 1; fi; the new bootstrap runs timeout 15 getent ahostsv4 github.com and a bare curl -fsS with no guard, so a DNS/egress failure after WireGuard teardown no longer aborts the deployment. Because release.sh presumably starts only after git checkout, a failure here means the deployment proceeds on broken egress and can fail later with a less actionable error. Consider restoring if ! guarded failure for these two probes.
| fi | ||
| docker pull "$remote_image" | ||
| docker tag "$remote_image" "$local_image:current" | ||
| docker image rm "$remote_image" >/dev/null || true |
There was a problem hiding this comment.
docker login uses printf '%s' "$GHCR_TOKEN" | docker login ... --password-stdin under set -o pipefail. This is good, but note that GHCR_TOKEN is now broughyt into the remote shell as a plain environment variable (via ssh-action envs), so it is visible in the remote process list/environment for the lifetime of the deploy job on the EC2 host. This is still better than the old inline echo ${{ secrets.GITHUB_TOKEN }} | ... (which leaked it into the workflow expression/args), so it is an improvement; just be aware the token materializes in the EC2 environment rather than being consumed directly by the action.
|
|
||
| ROLLBACK_ACTIVE=0 | ||
| for service in $PREVIOUS_SERVICES; do | ||
| docker rm "$service-previous" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
The signals-bot container is launched with -e TELEGRAM_BOT_TOKEN (the pass-through form) which exposes the token from the deploy shell environment into the container. Verify this is intended versus -e TELEGRAM_BOT_TOKEN="$TELEGRAM_BOT_TOKEN", since the pass-through form only works because docker forwards the existing env var and would silently start the bot with an empty value if the env is not actually propagated through ssh-action.
| set -euo pipefail | ||
|
|
||
| for required_name in \ | ||
| RELEASE_TAG RELEASE_IMAGE_TAG REGISTRY IMAGE_PREFIX GHCR_ACTOR GHCR_TOKEN \ |
There was a problem hiding this comment.
The required-variable guard uses if [ -z "${!required_name:-}" ] with indirect expansion under set -u. The ${!name:-} default handles an unset variable, but note REGISTRY and IMAGE_PREFIX are not listed in the workflow's envs: line (only RELEASE_TAG, RELEASE_IMAGE_TAG, GHCR_ACTOR, GHCR_TOKEN, TELEGRAM_BOT_TOKEN are exported). If REGISTRY/IMAGE_PREFIX were previously sourced from ${{ env.REGISTRY }} at the job level, confirm they are actually set in the remote environment or the guard will correctly fail-closed — which is safe but would break the deploy if they rely on job-level env: that was not exported.
|
Final review clarification: |
Summary
deploy/release.shIncident context
GitHub rejected the v1.37.0 workflow before any jobs ran with
Exceeded max expression length 21000. Production was not modified and remains on v1.36.0. v1.37.0 is marked not deployed/superseded; the corrected release will be v1.37.1.Verification
pytest tests/ -q— 266 passed, 1 skippedactionlint .github/workflows/deploy.ymlshellcheck deploy/release.shbash -n deploy/release.sh