Remove legacy cutover rollback support - #420
Conversation
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughRemoves legacy per-instance checkout scripts ( ChangesLegacy cutover/rollback removal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/server/deploy.sh`:
- Line 340: The deploy status message in the rollback guidance should not
advertise `predeploy_rollback.sh` when the deploy is running with `--no-backup`.
Update the logging around the `log` call in `scripts/server/deploy.sh` to gate
the rollback command on `DO_BACKUP`, or replace it with a clear warning that
rollback data is unavailable when no paired dump exists. Make sure any related
docs and tests that assume a rollback pair is always present are updated to
match the `predeploy_rollback.sh` / `DO_BACKUP` behavior.
In `@scripts/server/instance.sh`:
- Around line 324-325: The instance detection in instance.sh is too permissive:
if .env exists but app/current is missing, the script can mark the instance as
existing and later reconfigure it without taking the bootstrap/migration path.
Update the IS_EXISTING logic and the reconfigure flow to explicitly reject or
reroute instances that lack a release symlink, using the existing
INSTANCE_DIR/app, INSTANCE_DIR/current, and NEEDS_APP_BOOTSTRAP checks to force
the full deploy path when app/current is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 68d3b410-8663-4007-b9ed-1137b52b11fb
📒 Files selected for processing (9)
apps/workflow/tests/test_xero_instance_templates.pydocs/updating.mdscripts/legacy_rollback.shscripts/predeploy_backup.shscripts/server/README.mdscripts/server/cutover_legacy_instance.shscripts/server/deploy.shscripts/server/instance.shscripts/server/release-utils.sh
💤 Files with no reviewable changes (3)
- scripts/legacy_rollback.sh
- scripts/server/cutover_legacy_instance.sh
- scripts/server/release-utils.sh
| else | ||
| log " sudo $SCRIPT_DIR/../predeploy_rollback.sh $instance $(short_release_sha "$previous_sha")" | ||
| fi | ||
| log " sudo $SCRIPT_DIR/../predeploy_rollback.sh $instance $(short_release_sha "$previous_sha")" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't advertise predeploy_rollback.sh after a --no-backup deploy.
predeploy_rollback.sh restores the newest predeploy_*_<hash>.sql.gz for the requested short SHA. If this deploy ran with --no-backup, there is no fresh paired dump, so the printed command will either fail or roll the instance back to an older snapshot for the same release hash. Please gate this message on DO_BACKUP (or emit an explicit warning that rollback data is unavailable) and update the docs/tests that now assume the rollback pair always exists.
Suggested guard
- log " sudo $SCRIPT_DIR/../predeploy_rollback.sh $instance $(short_release_sha "$previous_sha")"
+ if [[ $DO_BACKUP -eq 1 ]]; then
+ log " sudo $SCRIPT_DIR/../predeploy_rollback.sh $instance $(short_release_sha "$previous_sha")"
+ else
+ log " --no-backup was used, so no fresh pre-deploy dump exists for rollback."
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log " sudo $SCRIPT_DIR/../predeploy_rollback.sh $instance $(short_release_sha "$previous_sha")" | |
| if [[ $DO_BACKUP -eq 1 ]]; then | |
| log " sudo $SCRIPT_DIR/../predeploy_rollback.sh $instance $(short_release_sha "$previous_sha")" | |
| else | |
| log " --no-backup was used, so no fresh pre-deploy dump exists for rollback." | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/server/deploy.sh` at line 340, The deploy status message in the
rollback guidance should not advertise `predeploy_rollback.sh` when the deploy
is running with `--no-backup`. Update the logging around the `log` call in
`scripts/server/deploy.sh` to gate the rollback command on `DO_BACKUP`, or
replace it with a clear warning that rollback data is unavailable when no paired
dump exists. Make sure any related docs and tests that assume a rollback pair is
always present are updated to match the `predeploy_rollback.sh` / `DO_BACKUP`
behavior.
1 participant
Summary
Validation
pytest apps/workflow/tests/test_xero_instance_templates.pybash scripts/test_release_utils.shshellcheck scripts/server/deploy.sh scripts/predeploy_backup.sh scripts/predeploy_rollback.sh scripts/server/release-utils.sh scripts/server/instance.sh scripts/test_release_utils.shrg -n "cutover_legacy_instance|legacy_rollback|legacy-rollbacks|is_legacy_checkout|compact_legacy_instance_checkout|allow-dirty|dirty legacy working tree" scripts docs apps/workflow/tests -g '!docs/plans/**'KAN-275
Summary by CodeRabbit
Bug Fixes
Documentation