[SAR] APPENG-5150: Add --purge-data flag to cleanup script#201
Conversation
Add an optional --purge-data flag to the Cloud Run cleanup script that deletes CloudSQL instances matching 'lightspeed' and lists Redis instances for manual deletion. Without the flag, data resources are preserved (existing behavior). This ensures data is not accidentally deleted during routine cleanup while providing an explicit purge option. - Add --purge-data CLI argument parsing - Add CloudSQL instance deletion loop when flag is set - Add Redis instance listing for manual cleanup - Update help text and 'not deleted' notice to reference --purge-data Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
luis5tb
left a comment
luis5tb
left a comment
There was a problem hiding this comment.
PR Review Summary
Critical (1)
1. Purge block runs after "Cleanup complete!" banner [cleanup.sh:275-290]
The --purge-data block executes after the "Cleanup complete!" message (line 264). If a CloudSQL deletion fails or hangs, the user already saw "complete." The purge should run as a new step section (e.g., "Step 5") before the summary block.
Important (3)
2. Confirmation prompt doesn't warn about data destruction [cleanup.sh:80-99]
The confirmation at line 93 lists services, secrets, Pub/Sub, and service accounts — but never mentions CloudSQL or data destruction when --purge-data is set. Running ./cleanup.sh --force --purge-data deletes databases with zero data-specific warning. The confirmation should include a line like " - CloudSQL instances matching 'lightspeed' (IRREVERSIBLE DATA LOSS)" when PURGE_DATA=true, or add a second confirmation specifically for the purge.
3. Misleading comment: # Flush Redis data [cleanup.sh:285]
The code doesn't flush anything; it lists instances with a "delete manually" note. The comment should say # List Redis instances for manual cleanup.
4. Missing indentation inside if block [cleanup.sh:293-296]
The echo lines inside if [ "$PURGE_DATA" != true ] are not indented, breaking the script's consistent 4-space indentation style:
if [ "$PURGE_DATA" != true ]; then
echo " - Cloud SQL instances (use --purge-data to delete)" # <-- should be indented
echo " - Cloud Memorystore Redis instances (use --purge-data to delete)" # <-- same
fiSuggestions (3)
5. Options header says "Redis data" but Redis isn't deleted [cleanup.sh:12]
--purge-data Also delete CloudSQL instances and Redis data implies Redis gets deleted, but the code only prints a manual-cleanup notice. Suggest: --purge-data Also delete CloudSQL instances (lists Redis for manual cleanup).
6. Regex filter could be narrower [cleanup.sh:280]
name~lightspeed could match unintended instances. Consider anchoring (name~^lightspeed) or using the existing DB_INSTANCE_NAME variable for an exact match.
7. Log warnings on CloudSQL delete failure [cleanup.sh:281]
|| true prevents script abort (good), but silently hides errors. Consider adding a log_warn on failure for observability.
Strengths
- Backwards-compatible: existing behavior unchanged without the flag
- Argument parsing follows the established pattern cleanly
|| trueon CloudSQL delete prevents the script from aborting mid-loop2>/dev/nullon list commands handles disabled APIs gracefully- Good use of
--quietto suppress interactive gcloud prompts
🤖 Generated with Claude Code
|
suggested fixes at yuvalk#4 |
2 participants
Summary
Addresses APPENG-5150.
Add an optional
--purge-dataflag to the Cloud Run cleanup script that enables deletion of CloudSQL instances and lists Redis instances for manual cleanup. Without the flag, data resources are preserved (existing behavior unchanged).Changes
--purge-dataCLI argument todeploy/cloudrun/cleanup.shname~lightspeed) when flag is setSAR Reference
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com