Fix review app delete project checkout#307
Conversation
WalkthroughThis PR introduces configurable working directory support to the deletion workflow and action. The control plane app action gains an optional ChangesWorking Directory Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a missing
Confidence Score: 4/5Safe to merge; the workflow, action, and spec changes are focused and correct. The three-file change is narrowly scoped: it adds a checkout step, threads working_directory: app through the action chain, and verifies the wiring with a YAML-parsing spec. The only gap is that the spec doesn't assert the if: steps.config.outputs.ready == 'true' guard on the new checkout step, so that condition could silently regress. No correctness bugs were found in the workflow or action logic itself. spec/github_workflows_spec.rb — missing assertion for the Important Files Changed
Sequence DiagramsequenceDiagram
participant GH as GitHub Event
participant CV as Validate Config
participant CO as Checkout (app/)
participant SE as Setup Environment
participant DA as Delete App (cpflow)
participant FN as Finalize Status
GH->>CV: PR closed / issue_comment / workflow_dispatch
CV-->>CO: "ready == 'true'"
CO->>CO: checkout consumer repo → app/
CO-->>SE: "working_directory=app"
SE->>SE: detect Ruby version in app/
SE->>SE: install cpln + cpflow
SE-->>DA: "working_directory=app"
DA->>DA: cd app/ → cpflow delete -a APP_NAME
DA-->>FN: success/failure
FN->>GH: update PR comment
Reviews (1): Last reviewed commit: "Fix review app delete project checkout" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it "runs cpflow delete from a downstream app checkout" do | ||
| expect(step_named("Checkout repository")).to include( | ||
| "uses" => "actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd", | ||
| "with" => include( | ||
| "path" => "app", | ||
| "persist-credentials" => false | ||
| ) | ||
| ) | ||
|
|
||
| expect(step_named("Setup environment").fetch("with")).to include( | ||
| "working_directory" => "app" | ||
| ) | ||
| expect(step_named("Delete review app").fetch("with")).to include( | ||
| "working_directory" => "app" | ||
| ) | ||
| end |
There was a problem hiding this comment.
if guard on the checkout step
The "Checkout repository" step carries if: steps.config.outputs.ready == 'true' in the workflow, which keeps it from running when config validation fails. The current spec uses include(...) so the if key is silently ignored. If that guard were accidentally dropped, the test would still pass while the step would check out into app/ even on invalid configs — causing a misleading empty working directory for subsequent steps.
Consider asserting "if" => "steps.config.outputs.ready == 'true'" alongside the other fields so the condition is pinned by the spec.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| def step_named(name) | ||
| steps.find { |step| step["name"] == name } | ||
| end |
There was a problem hiding this comment.
If a step is renamed or removed, step_named returns nil and the downstream .include(...) or .fetch("with") failure says expected nil to include ... — it doesn't tell you which step went missing. A small guard makes regressions easier to diagnose:
| end | |
| steps.find { |step| step["name"] == name } || | |
| raise("No workflow step named #{name.inspect}. Available: #{steps.filter_map { |s| s['name'] }.inspect}") |
| it "runs cpflow delete from a downstream app checkout" do | ||
| expect(step_named("Checkout repository")).to include( | ||
| "uses" => "actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd", | ||
| "with" => include( | ||
| "path" => "app", | ||
| "persist-credentials" => false | ||
| ) | ||
| ) | ||
|
|
||
| expect(step_named("Setup environment").fetch("with")).to include( | ||
| "working_directory" => "app" | ||
| ) | ||
| expect(step_named("Delete review app").fetch("with")).to include( | ||
| "working_directory" => "app" | ||
| ) | ||
| end |
There was a problem hiding this comment.
The test validates the checkout step's uses and with values but not its if: guard. If someone removed the if: steps.config.outputs.ready == 'true' condition, this test would still pass — but the checkout would now run even when config validation failed. Consider adding:
expect(step_named("Checkout repository")).to include(
"if" => "steps.config.outputs.ready == 'true'"
)
justin808
force-pushed
the
jg-codex/delete-review-app-project-checkout
branch
from
de1c2e6 to
c374ce2
Compare
Code Review — PR #307: Fix review app delete project checkoutOverviewThis PR fixes a real bug: The composite action change is backwards-compatible (default is Issues1. Changelog not updated The 2. When a step name does not match (e.g. due to a rename), 3. The spec verifies the checkout is wired correctly ( 4. Ruby version detection now reads the downstream app, not cpflow (intentional, worth a comment) Switching SummaryThe fix itself is correct and the new test file is a good addition. The items above are all minor (changelog entry, test robustness, one clarifying comment). Happy to approve once the changelog is updated. |
| let(:steps) { workflow.fetch("jobs").fetch("delete-review-app").fetch("steps") } | ||
|
|
||
| def step_named(name) | ||
| steps.find { |step| step["name"] == name } |
There was a problem hiding this comment.
When a step name is not found, find returns nil and the downstream expect(nil).to include(...) / nil.fetch("with") failure messages are opaque ("expected nil to include…"). Consider raising a step-not-found error immediately so the name mismatch is surfaced clearly:
| steps.find { |step| step["name"] == name } | |
| steps.find { |step| step["name"] == name } || | |
| raise("Step #{name.inspect} not found in workflow. Available: #{steps.filter_map { |s| s["name"] }.inspect}") |
| "persist-credentials" => false | ||
| ) | ||
| ) | ||
|
|
There was a problem hiding this comment.
The spec does not assert the if: guard on the checkout step. If the condition is accidentally removed or changed, the step would run even when config validation fails. Consider adding:
expect(step_named("Checkout repository")).to include(
"if" => "steps.config.outputs.ready == 'true'"
)
| uses: ./.cpflow/.github/actions/cpflow-setup-environment | ||
| with: | ||
| token: ${{ secrets.CPLN_TOKEN_STAGING }} | ||
| org: ${{ vars.CPLN_ORG_STAGING }} |
There was a problem hiding this comment.
The working_directory was previously .cpflow (the cpflow repo checkout). Changing it to app (the downstream caller repo) means Ruby version detection now reads the downstream project's .ruby-version / .tool-versions / Gemfile — which is the correct behaviour since cpflow needs to run in the app's Ruby environment. A short comment here would make this non-obvious choice self-documenting for future maintainers.
| org: ${{ vars.CPLN_ORG_STAGING }} | |
| working_directory: app # app's Ruby version files, not cpflow's |
* origin/main: Fix deploy-image showing container names instead of workload names (#255) (#294) Re-enable RuboCop NewCops and fix violations (#300) [codex] Fix stale app cleanup for image-less GVCs (#310) Handle non-zero exit from cpln workload exec gracefully (#301) Document Thruster HTTP/2 proxy configuration for Rails apps (#292) Friendly error in cpflow-detect-release-phase when controlplane.yml missing (#299) Document downstream cpflow testing (#308) Fix review app delete project checkout (#307) Fallback Ruby setup when no version file exists Move generated GitHub Actions logic upstream Animate review app command feedback (#304) Use Node 24 GitHub action versions (#303) Relax thor dependency to ~> 1.3 for Rails 8 solid_queue compatibility (#264) (#291)
1 participant
Summary
Validation
This fixes the failure observed when closing shakacode/react-webpack-rails-tutorial#734: cpflow delete could not find project_folder/.controlplane/controlplane.yml.
Note
Low Risk
Low risk: workflow wiring change to ensure
cpflowdelete runs in the correct repo directory, plus test coverage for the new checkout/working-directory behavior.Overview
Fixes review-app deletion by checking out the downstream/caller repo into
app/before running delete, and then runningcpflowsetup + delete from thatappworking directory so.controlplane/controlplane.ymlis present.Updates the
cpflow-delete-control-plane-appcomposite action to accept aworking_directoryinput (default.) and applies it viaworking-directory, and adds/updates RSpec assertions to lock in the workflow/action wiring.Reviewed by Cursor Bugbot for commit c374ce2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Tests
Chores