Fix Review App Deletion Workflows #639
Conversation
|
""" WalkthroughA new GitHub Action, "Delete Control Plane App," has been introduced to automate the deletion of Control Plane applications and their resources. This action uses a Bash script that validates input, prevents deletion of critical environments, checks for the application's existence, and deletes it if found. Workflow files have been updated to use this new action and script, with adjustments to environment variable handling and removal of unused parameters. The nightly workflow was streamlined by removing explicit stale PR fetching and per-PR deletion logic in favor of a single cleanup command. Deployment workflow improvements include an early exit if deployment is disabled and removal of redundant conditional checks. Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant DeleteAction
participant DeleteScript
participant ControlPlane
Workflow->>DeleteAction: Trigger Delete Control Plane App
DeleteAction->>DeleteScript: Run delete-app.sh with APP_NAME, CPLN_ORG
DeleteScript->>DeleteScript: Validate inputs and safety checks
alt App name is safe
DeleteScript->>ControlPlane: cpflow exists APP_NAME CPLN_ORG
alt App exists
DeleteScript->>ControlPlane: cpflow delete APP_NAME CPLN_ORG --force
ControlPlane-->>DeleteScript: Deletion result
else App does not exist
DeleteScript-->>Workflow: Print warning, exit
end
else App name contains "production" or "staging"
DeleteScript-->>Workflow: Print error, exit
end
DeleteScript-->>Workflow: Print confirmation or error
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi 👋 Here are the commands available for this PR:
Use |
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.github/actions/delete-control-plane-app/delete-app.sh (1)
8-8: Consider enabling stricter error handling
Usingset -eis good, but adding-uand-o pipefailhelps catch unset variables and failures in pipelines.
Apply this diff:-set -e +set -euo pipefail
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/actions/delete-control-plane-app/action.yml(1 hunks).github/actions/delete-control-plane-app/delete-app.sh(1 hunks).github/workflows/delete-review-app.yml(2 hunks).github/workflows/nightly-remove-stale-review-apps.yml(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/delete-review-app.yml
148-148: input "org" is not defined in action "Delete Control Plane App" defined at "./.github/actions/delete-control-plane-app". available inputs are "app_name", "cpln_org"
(action)
🔇 Additional comments (7)
.github/actions/delete-control-plane-app/delete-app.sh (4)
11-12: Environment variable validation is clear and robust
Using parameter expansion with an error message ensures the script fails fast if required variables are missing.
15-20: Safety check prevents deleting critical environments
The case-insensitive grep for “production” or “staging” is an effective guardrail. Error messages are sent to stderr for visibility.
23-27: Graceful handling when the application does not exist
Checking existence first and exiting with status 0 avoids unnecessary errors in CI runs.
30-34: Deletion logic handles failures appropriately
The conditional deletion with error reporting on failure is well-structured..github/workflows/nightly-remove-stale-review-apps.yml (1)
51-51: Explicitly setting the organization variable
AssigningCPLN_ORG=${{ vars.CPLN_ORG_STAGING }}here aligns with how the delete script expectsCPLN_ORG..github/actions/delete-control-plane-app/action.yml (2)
4-11: Composite action inputs are well-defined
Acceptingapp_nameandcpln_orgcovers the necessary parameters for deletion.
12-21: Runs block correctly invokes the deletion script
Usinggithub.action_pathensures the script is executed from the correct location.
|
✅ Deployment complete for PR #639, commit 3100c84 |
|
✅ Deployment complete for PR #639, commit 3b8ab76 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/actions/delete-control-plane-app/delete-app.sh (2)
8-9: Enhance script robustness withpipefailand undefined variable checks
It's best practice to addset -o pipefailandset -ualongsideset -eto catch failures in pipelines and unset variables.Proposed diff:
-set -e +set -euo pipefail
38-39: Consider adding explicit exit code at the end
While a trailingechoreturns 0 by default, an explicitexit 0can improve clarity.Proposed diff:
echo "✅ Successfully deleted application: $APP_NAME" +exit 0
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
-
.github/actions/delete-control-plane-app/delete-app.sh(1 hunks) -
.github/workflows/delete-review-app.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/delete-review-app.yml
🧰 Additional context used
🪛 Shellcheck (0.10.0)
.github/actions/delete-control-plane-app/delete-app.sh
[error] 13-13: ${$x} is invalid. For expansion, use ${x}. For indirection, use arrays, ${!x} or (for sh) eval.
(SC2298)
[error] 14-14: ${$x} is invalid. For expansion, use ${x}. For indirection, use arrays, ${!x} or (for sh) eval.
(SC2298)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: deploy
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (3)
.github/actions/delete-control-plane-app/delete-app.sh (3)
16-22: Safety check for critical environments is solid
The grep-based guard prevents deletion of any app containing “production” or “staging” (case-insensitive), which is a good safeguard.
25-29: Handle non-existent apps gracefully
Exiting with status 0 when the app doesn’t exist is appropriate to avoid false failures in workflows.
31-36: Robust error handling on deletion
Checking the exit status ofcpflow deleteand exiting with an error code on failure ensures the Action fails fast if deletion does not succeed.
|
✅ Deployment complete for PR #639, commit 3d45555 |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/delete-control-plane-app/delete-app.sh(1 hunks).github/workflows/deploy-to-control-plane-review-app.yml(2 hunks).github/workflows/nightly-remove-stale-review-apps.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/nightly-remove-stale-review-apps.yml
- .github/actions/delete-control-plane-app/delete-app.sh
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: deploy
- GitHub Check: test (22.x, 3.3.4)
🔇 Additional comments (1)
.github/workflows/deploy-to-control-plane-review-app.yml (1)
205-205: Removal of console link from initial comment is intentional
Dropping theCONSOLE_LINKhere aligns with the new streamlined startup message. Subsequent status updates still include the console URL once it’s available.
|
|
||
| - name: Setup Control Plane App if Not Existing | ||
| if: env.DO_DEPLOY == 'true' && env.APP_EXISTS == 'false' | ||
| if: env.APP_EXISTS == 'false' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Include DO_DEPLOY guard in the app setup condition
Right now the setup step only checks APP_EXISTS, so it can still run when deployments are disabled. It’s safer to combine both flags:
if: env.APP_EXISTS == 'false' && env.DO_DEPLOY != 'false'This ensures that a new app is only provisioned when we explicitly want to deploy.
| if [[ "${{ env.DO_DEPLOY }}" == "false" ]]; then | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Early-exit logic does not skip subsequent steps as intended
The exit 0 inside the Validate Deployment Request step only terminates that shell script—it does not prevent the rest of the job from executing. As a result, even when DO_DEPLOY is "false", all downstream steps will still run.
To properly gate the workflow, consider emitting a step output (e.g., echo "do_deploy=${DO_DEPLOY}" >> $GITHUB_OUTPUT in the validate script) and then adding:
if: steps.validate.outputs.do_deploy == 'true'to every subsequent step. This ensures that all deployment steps are truly skipped when DO_DEPLOY is disabled.
|
✅ Deployment complete for PR #639, commit 8487edc |
|
✅ Deployment complete for PR #639, commit 7de811b |
|
✅ Review app for PR #639 was successfully deleted |
|
✅ Deployment complete for PR #639, commit 2ad243c |
|
/delete-review-app |
|
✅ Review app for PR #639 was successfully deleted |
|
🚀 Deploying to Control Plane... ⏳ Waiting for deployment to be ready... |
|
✅ Review app for PR #639 was successfully deleted |
|
✅ Deployment complete for PR #639, commit 7440108 |
|
✅ Review app for PR #639 was successfully deleted |
This change is
Summary by CodeRabbit