fix: stash EC2 worktree changes before deploy checkout#57
Conversation
protostatis
left a comment
There was a problem hiding this comment.
Sky's Code Review
This PR adds EC2 worktree stash logic before release tag checkout to preserve local operational edits (like environment configuration changes). The approach is pragmatic and fixes the documented v1.28.0 failure where a local edit blocked checkout. Key concerns: (1) no error handling around the stash/checkout sequence — if stash succeeds but checkout fails, the dirty state is lost with no rollback; (2) stashes accumulate indefinitely with no pruning; (3) --include-untracked combined with stash creates a one-way discard rather than stash's intended save-and-restore pattern.
Verdict: Comment
Comments
- The stash approach is a pragmatic fix for the documented failure case, but it treats stash as a one-way discard mechanism. If the intent is to make stash an audit trail, document that expectation and add periodic cleanup. If the intent is purely to prevent checkout conflicts, consider whether a
.git/info/excluderule or a deploy-wrapper script that handles dirty trees explicitly would be cleaner. - No evidence of
git diff --checkor manual verification in the PR. Consider confirming the deploy workflow runs end-to-end in a staging environment with a dirty worktree to validate the stash/checkout sequence. - The
STASH_NAMEuses${{ github.ref_name }}which will resolve to the tag name (e.g.,v1.28.0) on tag-triggered deploys — this is correct and useful for identifying which deploy created the stash.
Reviewed by Sky — Unchained Sky engineering agent
| # Pull latest code. Preserve any EC2-local edits so deployment can | ||
| # move to the release tag without discarding operational changes. | ||
| git fetch --tags origin | ||
| if [ -n "$(git status --porcelain)" ]; then |
There was a problem hiding this comment.
Consider adding set -euo pipefail at the top of the step's run block if not already present. If git stash push succeeds but git checkout fails for any reason, the stashed changes are stranded — no attempt to pop/restore them. A trap or explicit error handling would prevent permanent loss of EC2-local edits.
| git fetch --tags origin | ||
| if [ -n "$(git status --porcelain)" ]; then | ||
| STASH_NAME="pre-deploy-${{ github.ref_name }}-$(date -u +%Y%m%dT%H%M%SZ)" | ||
| git stash push --include-untracked -m "$STASH_NAME" |
There was a problem hiding this comment.
git fetch --tags origin is correct since we're checking out a tag, but note that git fetch origin without --tags already fetches tags if they're associated with fetched refs. Consider whether --tags adds meaningful coverage here, or if it fetches tags unnecessarily on every deploy.
| STASH_NAME="pre-deploy-${{ github.ref_name }}-$(date -u +%Y%m%dT%H%M%SZ)" | ||
| git stash push --include-untracked -m "$STASH_NAME" | ||
| echo "Stashed EC2 local worktree changes: $STASH_NAME" | ||
| fi |
There was a problem hiding this comment.
Stashes accumulate over time with no pruning. Old entries like pre-deploy-v1.28.0-..., pre-deploy-v1.29.0-... will pile up indefinitely. Consider adding a cleanup step after successful deploy (e.g., git stash drop for the stash we just created, or a periodic git stash clear) since the pattern here treats stash as a discard buffer rather than a save-and-restore mechanism.
Summary
Why
The v1.28.0 deploy failed before containers were touched because the EC2 checkout had a local edit to ERROR: WG_PRIVATE_KEY is not set. This lets deploy proceed without discarding that local change.
Verification