Document cpflow downstream setup#747
Conversation
🚀 Quick Review App CommandsWelcome! Here are the commands you can use in this PR:
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a "GitHub Review App Setup" section and clarifies cpflow secrets/variables and advanced testing; repins all cpflow wrapper workflows to a newer immutable upstream commit SHA, and annotates promotion workflow permissions and environment secret usage. ChangesControl Plane Documentation and Workflow Reference Update
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.
Reviewed by Cursor Bugbot for commit d5e1fd6. Configure here.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @@ -15,4 +15,4 @@ permissions: | |||
| jobs: | |||
| show-help: | |||
| if: vars.REVIEW_APP_PREFIX != '' | |||
There was a problem hiding this comment.
This guard contradicts the new documentation. The README and cpflow-help.md now say REVIEW_APP_PREFIX is optional and inferred from controlplane.yml, but this condition means the help workflow will be silently skipped when REVIEW_APP_PREFIX is unset — which is the new recommended default.
If the upstream reusable workflow now supports inferring the prefix, this guard should be removed (or updated to a condition that always evaluates to true for authorized callers). As-is, teams following the new "leave REVIEW_APP_PREFIX unset" guidance will never see the help command fire.
| if: vars.REVIEW_APP_PREFIX != '' | |
| if: true |
| # upstream workflow. The upstream workflow only reads the named secrets it | ||
| # references, but GitHub does not enforce that boundary. Strict consumers can | ||
| # set CPFLOW_GITHUB_ACTIONS_REF to an immutable commit SHA. | ||
| secrets: inherit |
There was a problem hiding this comment.
The comment mentions CPFLOW_GITHUB_ACTIONS_REF as an env var for strict consumers, but the actual input used everywhere else in these workflows is control_plane_flow_ref. This looks like a variable name that doesn't exist — it may confuse readers trying to act on this guidance.
Consider referencing the correct mechanism (the control_plane_flow_ref workflow input or the @<sha> ref in uses:) instead.
Code Review: Document cpflow downstream setupOverviewThis PR documents the minimal GitHub/Control Plane setup for review apps, staging, and production, and bumps all generated cpflow workflow wrappers from commit IssuesBug: Minor: Misleading variable name in Security
Documentation QualityThe new sections in One minor note: the README update changes the review app naming convention from SummaryThe |
Greptile SummaryThis PR documents the minimal GitHub and Control Plane setup required for generated cpflow review apps, staging deploys, and production promotion, and bumps all workflow SHA pins from
Confidence Score: 4/5Safe to merge with one follow-up: the REVIEW_APP_PREFIX guard in cpflow-review-app-help.yml should be removed or relaxed now that the variable is documented as optional. The documentation and SHA-pin changes are accurate and consistent. The only functional mismatch is in cpflow-review-app-help.yml, where a pre-existing guard silently suppresses the onboarding help comment for any repo that follows the newly documented standard setup without setting REVIEW_APP_PREFIX. Deploys still work — only the help message is lost — but it directly contradicts the intent of this PR. .github/workflows/cpflow-review-app-help.yml — the REVIEW_APP_PREFIX guard needs to be removed or updated to match the new optional-variable documentation.
|
| Filename | Overview |
|---|---|
| .github/workflows/cpflow-review-app-help.yml | SHA bumped to db013e1; the pre-existing if: vars.REVIEW_APP_PREFIX != '' guard now contradicts the updated docs that say REVIEW_APP_PREFIX is optional, meaning the help job is silently skipped for standard setups. |
| .github/workflows/cpflow-promote-staging-to-production.yml | Added production_environment: production input to gate production secrets behind the protected GitHub Environment; added explanatory comments; SHA bumped. The acknowledged secrets: inherit scope is worth verifying against org policy. |
| .controlplane/readme.md | Large documentation addition covering GitHub secrets/variables, Control Plane org/app/secret-dictionary setup, and production promotion isolation. Content is accurate and consistent with the workflow changes. |
| .github/cpflow-help.md | Updated secrets/variables tables to mark REVIEW_APP_PREFIX optional, clarified CPLN_TOKEN_PRODUCTION placement, and added an advanced section on testing upstream PRs with commit-SHA pins. |
| .github/workflows/cpflow-cleanup-stale-review-apps.yml | SHA bumped from 3e0e7e1 to db013e1 in both uses: and control_plane_flow_ref; no other changes. |
| .github/workflows/cpflow-delete-review-app.yml | SHA bumped consistently; no other changes. |
| .github/workflows/cpflow-deploy-review-app.yml | SHA bumped consistently; no other changes. No REVIEW_APP_PREFIX guard here (intentional asymmetry with cpflow-review-app-help.yml). |
| .github/workflows/cpflow-deploy-staging.yml | SHA bumped consistently; no other changes. |
| .github/workflows/cpflow-help-command.yml | SHA bumped consistently; no other changes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
PR[Pull Request opened/sync] --> DeployJob[cpflow-deploy-review-app\nno REVIEW_APP_PREFIX guard]
PR --> HelpJob{cpflow-review-app-help\nif REVIEW_APP_PREFIX != ''}
HelpJob -- "var set" --> PostHelp[Post help comment on PR]
HelpJob -- "var unset\n(new standard setup)" --> Skip[Job skipped — no help posted]
DeployJob --> UpstreamDeploy[Upstream reusable workflow\ninfers prefix from controlplane.yml]
Master[Push to master] --> StagingDeploy[cpflow-deploy-staging]
StagingDeploy --> UpstreamStaging[Upstream staging workflow]
ManualTrigger[workflow_dispatch\nconfirm_promotion=promote] --> PromoteJob[cpflow-promote-staging-to-production]
PromoteJob --> EnvGate{Protected GitHub Environment\nproduction — requires reviewer approval}
EnvGate -- approved --> UpstreamPromote[Upstream promote workflow\nreads CPLN_TOKEN_PRODUCTION]
EnvGate -- rejected --> Blocked[Promotion blocked]
Reviews (1): Last reviewed commit: "Document cpflow downstream setup" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -15,4 +15,4 @@ permissions: | |||
| jobs: | |||
| show-help: | |||
| if: vars.REVIEW_APP_PREFIX != '' | |||
There was a problem hiding this comment.
The updated docs explicitly say REVIEW_APP_PREFIX is optional and inferred from controlplane.yml, but the guard if: vars.REVIEW_APP_PREFIX != '' remains. Any repo that follows the new standard setup (leaving REVIEW_APP_PREFIX unset) will never see the help comment posted on opened PRs — the job is skipped entirely. The deploy workflow (cpflow-deploy-review-app.yml) has no such guard, so deploys work but onboarding help is silently absent.
| # `secrets: inherit` passes all caller repository secrets to the trusted | ||
| # upstream workflow. The upstream workflow only reads the named secrets it | ||
| # references, but GitHub does not enforce that boundary. Strict consumers can | ||
| # set CPFLOW_GITHUB_ACTIONS_REF to an immutable commit SHA. | ||
| secrets: inherit |
There was a problem hiding this comment.
secrets: inherit exposes all repository secrets to the upstream workflow
The inline comment correctly notes that secrets: inherit passes every caller repository secret to the upstream reusable workflow, and that GitHub does not enforce the named-secrets boundary. In the current setup this includes CPLN_TOKEN_STAGING, any DOCKER_BUILD_SSH_KEY, and any other repository secrets. If the upstream SHA (db013e...) ever changes to include malicious or compromised code, those secrets are exposed during a production-promotion run. The PR already pins to an immutable SHA which limits supply-chain risk, but consider whether your org's policy requires explicit secret whitelisting via environment-level scoping rather than secrets: inherit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 @.controlplane/readme.md:
- Around line 489-493: The README guidance and shakacode-team doc disagree about
review-app naming: update .controlplane/shakacode-team.md to match
.controlplane/readme.md by changing the instructions that set REVIEW_APP_PREFIX
to instead instruct leaving REVIEW_APP_PREFIX unset (or to document the inferred
name qa-react-webpack-rails-tutorial) and reference
.controlplane/controlplane.yml for app name alignment; ensure the text mentions
the generated review app pattern qa-react-webpack-rails-tutorial-<PR number> and
remove the conflicting example
REVIEW_APP_PREFIX=qa-react-webpack-rails-tutorial-pr so both docs are
consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 339f15eb-4b09-48f3-897d-86bcf9c18dec
📒 Files selected for processing (9)
.controlplane/readme.md.github/cpflow-help.md.github/workflows/cpflow-cleanup-stale-review-apps.yml.github/workflows/cpflow-delete-review-app.yml.github/workflows/cpflow-deploy-review-app.yml.github/workflows/cpflow-deploy-staging.yml.github/workflows/cpflow-help-command.yml.github/workflows/cpflow-promote-staging-to-production.yml.github/workflows/cpflow-review-app-help.yml
Code Review: Document cpflow downstream setupOverviewThis PR improves the setup experience for downstream cpflow users by consolidating documentation and fixing the production token exposure model. The security change—moving SecurityPositive: Gating Concern — Breaking Change: Review App Naming ConventionThe PR changes the review app naming scheme from Documentation QualityThe new The advanced section in Workflow Changes
Summary
Overall this is a solid improvement. Address the naming-convention migration note and clarify the |
| # references, but GitHub does not enforce that boundary. Strict consumers can | ||
| # keep both the `uses:` ref and `control_plane_flow_ref` pinned to an | ||
| # immutable commit SHA. | ||
| secrets: inherit |
There was a problem hiding this comment.
The comment says the upstream workflow "only reads the named secrets it references," which is accurate for repository secrets. Worth clarifying here that environment secrets (e.g. CPLN_TOKEN_PRODUCTION) are not passed via secrets: inherit—they're only injected when the job itself directly references the production environment. So the production token isolation is actually stronger than this comment implies; a reader might incorrectly assume CPLN_TOKEN_PRODUCTION leaks through secrets: inherit.
| `qa-react-webpack-rails-tutorial` and staging org | ||
| `shakacode-open-source-examples-staging` from `.controlplane/controlplane.yml` | ||
| because that file defines exactly one app with | ||
| `match_if_app_name_starts_with: true`. |
There was a problem hiding this comment.
This inference assumption is fragile: if controlplane.yml ever gains a second app entry with match_if_app_name_starts_with: true, the inference will fail (or pick the wrong one) with no clear error. Consider adding a sentence here directing readers to set CPLN_ORG_STAGING and REVIEW_APP_PREFIX explicitly if they hit ambiguous inference, so the failure mode is less opaque.
| - Leave `REVIEW_APP_PREFIX` unset for the standard path. The workflow infers | ||
| `qa-react-webpack-rails-tutorial` from `.controlplane/controlplane.yml`, so | ||
| generated review apps use names like | ||
| `qa-react-webpack-rails-tutorial-1234`. |
There was a problem hiding this comment.
Breaking change — naming convention. The old scheme was qa-react-webpack-rails-tutorial-pr-<N>; the new scheme omits the -pr segment. Any live review apps created under the old convention become orphaned after this merges: the cleanup and delete workflows query by the new prefix and won't find them. Recommend adding a migration note (e.g. in the PR description or a one-time manual cleanup step) so operators know to delete old-style apps before merging.
Code Review: Document cpflow downstream setupOverall assessment: Low-risk, well-structured documentation PR. The shift of `CPLN_TOKEN_PRODUCTION` from a repository secret to a protected GitHub Environment is a genuine security improvement. SHA pinning and the two-pin comment in the promotion workflow are both good practices. ✅ Strengths
Issues1. Contradictory `Required` column for production variables in `cpflow-help.md` The table rows for `CPLN_ORG_PRODUCTION` and `PRODUCTION_APP_NAME` say `yes (for promote)` in the Required column — implying repository variables — but the Notes column says "Prefer a `production` environment variable." The recommendation and the column label contradict each other. Readers who follow the `yes` column will put these in repo variables; readers who follow the note will put them in the environment. The table should be updated to say something like `yes (for promote) — as environment variable` or the Required column should use a different marker for environment variables. (See inline comment.) 2. `STAGING_APP_BRANCH` / `PRIMARY_WORKLOAD` required vs. optional discrepancy `shakacode-team.md` lists both under "Required repository variables for staging deploys", while `cpflow-help.md` marks both as optional. At minimum `PRIMARY_WORKLOAD` defaults to `rails`, so listing it as required in one doc and optional in the other will confuse fork maintainers. 3. `SECRET_KEY_BASE` test placeholder The readme documents that the review/staging template "currently contains a test placeholder for `SECRET_KEY_BASE`" and asks readers to replace it before production. This is a hard security requirement, not just guidance — a placeholder key in staging means any staging environment is running with a known, weak secret. Consider either removing the placeholder in a follow-up PR and replacing it with a proper secret reference, or at minimum adding a CI check that fails if the placeholder value is detected. 4. Minor `secrets: inherit` comment improvement The acknowledgment in the promotion workflow is transparent and appreciated. One thing worth making explicit: because `CPLN_TOKEN_PRODUCTION` is now an environment secret (not a repo secret), `secrets: inherit` in the staging and review-app workflows won't inadvertently expose it — the environment gate is the actual isolation boundary, not just the upstream workflow's access pattern. Adding that note would help future readers understand why the security model holds. |
| | `CPLN_ORG_STAGING` | yes | Control Plane org on controlplane.com for staging and review apps. | | ||
| | `CPLN_ORG_PRODUCTION` | yes (for promote) | Control Plane org on controlplane.com for production. | | ||
| | `CPLN_ORG_STAGING` | optional for review apps; yes for staging | Override the staging/review Control Plane org inferred from `controlplane.yml`. | | ||
| | `CPLN_ORG_PRODUCTION` | yes (for promote) | Control Plane org on controlplane.com for production. Prefer a `production` environment variable. | |
There was a problem hiding this comment.
The Required column says yes (for promote) — which implies a repository variable — but the Notes column says "Prefer a production environment variable." These two signals contradict each other. Readers who scan the Required column will put this in repo variables; readers who follow the note will put it in the protected environment. Suggest picking one canonical placement and stating it clearly, e.g. updating the Required cell to yes — environment variable or splitting the secrets and environment-variable rows into separate tables.
| | `PRODUCTION_APP_NAME` | yes (for promote) | App name in `controlplane.yml` used as the production deploy target. | | ||
| | `REVIEW_APP_PREFIX` | yes | Prefix for per-PR review app names (e.g. `review-app`). | | ||
| | `REVIEW_APP_DEPLOYING_ICON_URL` | optional | Custom image URL for the animated deploying icon in review-app PR comments. Set to `none` to use the text fallback icon. | | ||
| | `PRODUCTION_APP_NAME` | yes (for promote) | App name in `controlplane.yml` used as the production deploy target. Prefer a `production` environment variable. | |
There was a problem hiding this comment.
Same issue as CPLN_ORG_PRODUCTION above: yes (for promote) implies a repo variable, but the Notes column recommends a production environment variable. Both entries need a consistent placement signal.
| # `secrets: inherit` passes all caller repository secrets to the trusted | ||
| # upstream workflow. The upstream workflow only reads the named secrets it | ||
| # references, but GitHub does not enforce that boundary. Strict consumers can | ||
| # keep both the `uses:` ref and `control_plane_flow_ref` pinned to an | ||
| # immutable commit SHA. |
There was a problem hiding this comment.
The transparency here is good. One clarification worth adding to the comment: because CPLN_TOKEN_PRODUCTION is now an environment secret (not a repo secret), secrets: inherit in the staging/review-app caller workflows won't expose it — those jobs never acquire the production environment context, so GitHub never injects the environment secret into them. The environment gate is the actual isolation boundary, not just the upstream workflow's declared access pattern. This makes the security model more robust than the comment currently implies.
|
|
||
| Generate `SECRET_KEY_BASE` with `openssl rand -hex 64` and | ||
| `RENDERER_PASSWORD` with `openssl rand -hex 32`. The review/staging template | ||
| currently contains a test placeholder for `SECRET_KEY_BASE`; replace it with a |
There was a problem hiding this comment.
Documenting the placeholder is good, but this phrasing ("replace it with a secret-backed value before promoting production") leaves staging running with a known, weak SECRET_KEY_BASE. If the staging environment is internet-accessible (which review apps typically are), that's a real risk — anyone with the placeholder value can forge session cookies. Consider either removing the placeholder from the template in a follow-up and requiring an explicit secret reference, or adding a CI/readiness check that rejects deploys when SECRET_KEY_BASE matches the placeholder value.
Code ReviewOverview: Documentation-heavy PR that improves the cpflow downstream setup story — moves the production token into a protected GitHub Environment, infers review-app config from Issues1.
2. Removing 3. Naming change from The PR removes the 4. The inline comment acknowledges the issue but leaves it as-is. Minor
Positives
|
| contains(fromJson('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association)) || | ||
| github.event_name == 'workflow_dispatch' | ||
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-help-command.yml@3e0e7e1f0a35c15648cc9254b573b058d77ca8c4 | ||
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-help-command.yml@cfe494bf32925d49508380e03856d97bd71f6689 |
There was a problem hiding this comment.
All the operational cpflow workflows (cleanup, delete, deploy, staging, promote) pass control_plane_flow_ref as a with: input alongside the uses: SHA. This one does not. If the upstream help workflow has a setup action that uses control_plane_flow_ref to check out and build the gem, it will silently fall back to its own default rather than the pinned SHA.
Unless the upstream cpflow-help-command.yml workflow is known to not have a gem-build step, add:
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-help-command.yml@cfe494bf32925d49508380e03856d97bd71f6689 | |
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-help-command.yml@cfe494bf32925d49508380e03856d97bd71f6689 | |
| with: | |
| control_plane_flow_ref: cfe494bf32925d49508380e03856d97bd71f6689 |
Or add a comment explaining why this workflow doesn't need it.
| show-help: | ||
| if: vars.REVIEW_APP_PREFIX != '' | ||
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-review-app-help.yml@3e0e7e1f0a35c15648cc9254b573b058d77ca8c4 | ||
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-review-app-help.yml@cfe494bf32925d49508380e03856d97bd71f6689 |
There was a problem hiding this comment.
Same as cpflow-help-command.yml — no with: block means control_plane_flow_ref is not passed, unlike all the operational workflows. If the upstream help workflow doesn't need it, a short comment here would prevent future maintainers from treating this as an oversight during the next SHA repin.
| # references, but GitHub does not enforce that boundary. Strict consumers can | ||
| # keep both the `uses:` ref and `control_plane_flow_ref` pinned to an | ||
| # immutable commit SHA. | ||
| secrets: inherit |
There was a problem hiding this comment.
secrets: inherit sends all caller repository secrets — including CPLN_TOKEN_STAGING, SSH keys, and any other repository secrets — to the upstream reusable workflow. The SHA pin limits exposure to a known-good commit, but it is still a broader grant than strictly necessary for a production promotion job.
If the upstream cpflow-promote-staging-to-production.yml workflow supports receiving named secrets explicitly (e.g., secrets: { CPLN_TOKEN_PRODUCTION: ${{ secrets.CPLN_TOKEN_PRODUCTION }} }), that would be strictly tighter. If it requires secrets: inherit, the current approach is the practical maximum — just worth documenting explicitly so the next maintainer doesn't widen the SHA pin without realizing the implication.
| `qa-react-webpack-rails-tutorial-pr-1234`. | ||
| The GitHub settings and Control Plane resources must match the app names in | ||
| `.controlplane/controlplane.yml`. For the standard review-app path, leave | ||
| `REVIEW_APP_PREFIX` unset and let the workflow infer |
There was a problem hiding this comment.
This silently changes the naming convention from the previously documented qa-react-webpack-rails-tutorial-pr-<N> (with the -pr suffix) to qa-react-webpack-rails-tutorial-<N>. Any review apps currently running under the old names, bookmark links, or CI output referencing them will break without warning.
Consider adding a migration note here, even one sentence: "If you previously had REVIEW_APP_PREFIX set to qa-react-webpack-rails-tutorial-pr, delete existing review apps before switching — the old names will become orphaned resources in Control Plane."
Code Review: Document cpflow downstream setupOverviewThis PR improves the security posture of the cpflow GitHub Actions integration in three meaningful ways: (1) replaces broad Security — Positive Changes
Issues1. Implicit In 2. Breaking naming convention — no automated migration The switch from to help operators find orphaned apps. 3. The old 4. Minor: missing blank line in Two conceptually separate paragraphs run together: Add a blank line between them. Nits
SummaryThe security improvements are correct and the documentation is thorough. The two functional items worth addressing before merge are: (a) documenting or testing the implicit production-secret forwarding chain via |
| Current workflow wrappers are pinned to upstream `control-plane-flow` PR #318 at | ||
| `19ca93eadcba81e7438f26ab55a0f2ca7ace82b0` for downstream testing. After 5.0.1 | ||
| ships, repin the wrappers to `v5.0.1`; do not treat the temporary SHA as the | ||
| steady-state master configuration. |
There was a problem hiding this comment.
This SHA is a temporary test pin and must be replaced with v5.0.1 once the upstream release ships. Please track this as a follow-up issue before merging — without an issue, the repin is easy to forget and the repo will stay on an unversioned pre-release commit indefinitely.
| # This is intentionally unconditional: adding this wrapper opts the repo in | ||
| # to PR-open help. Remove the wrapper or add a repo-specific `if:` guard if | ||
| # review apps should not be advertised before Control Plane is configured. |
There was a problem hiding this comment.
Removing the if: vars.REVIEW_APP_PREFIX != '' guard is fine for this repo since Control Plane is expected to be configured. The comment is accurate and clear.
Just note that any fork that clones this repo and hasn't yet set up CP will immediately start posting help banners on every PR open. The comment's suggestion to remove the wrapper or add an if: guard should probably be repeated in the setup docs for fork users.
Code Review — PR #747: Document cpflow downstream setupOverviewThis PR does three things together:
Overall direction is correct and the security posture improves meaningfully. A few specific notes below. Security: Explicit Secrets ✅Dropping One nuance: DOCKER_BUILD_SSH_KEY: ${{ secrets.DOCKER_BUILD_SSH_KEY }}GitHub resolves a non-existent secret to an empty string rather than erroring, so repos without SSH build dependencies should be fine — but the upstream reusable workflow needs to handle an empty value gracefully (e.g. skip the
|
| Finding | Severity |
|---|---|
bin/test-cpflow-github-flow drops conductor-exec — may hit wrong Ruby/cpflow in Conductor |
Medium |
Temporary SHA c0ca750c needs tracking before it becomes permanent |
Medium |
DOCKER_BUILD_SSH_KEY always forwarded — upstream workflow must tolerate empty value |
Low |
Dir[] without .sort — non-deterministic output order |
Cosmetic |
| @@ -10,10 +10,10 @@ if [[ $# -gt 0 ]]; then | |||
| fi | |||
|
|
|||
| echo "==> cpflow github-flow-readiness" | |||
There was a problem hiding this comment.
The outer bin/conductor-exec bin/test-cpflow-github-flow call sets up the environment for the outer process, but each ruby <<'RUBY' here spawns a new subprocess. Whether the correct Ruby is inherited depends on whether mise exec (or your version manager) rewrites PATH for the whole process tree or only for the immediate command.
If mise exec does propagate the correct PATH transitively, this is fine. If it doesn't, these inline ruby heredocs and the "${cpflow_cmd[@]}" call will hit system Ruby/cpflow. Worth a quick sanity-check: bin/conductor-exec bin/test-cpflow-github-flow → does ruby --version inside match .tool-versions?
| echo "==> cpflow github-flow-readiness" | |
| "${cpflow_cmd[@]}" github-flow-readiness |
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-deploy-review-app.yml@c0ca750c4838dacf6a1d81d52cb151301516cb77 | ||
| secrets: | ||
| CPLN_TOKEN_STAGING: ${{ secrets.CPLN_TOKEN_STAGING }} | ||
| DOCKER_BUILD_SSH_KEY: ${{ secrets.DOCKER_BUILD_SSH_KEY }} |
There was a problem hiding this comment.
If DOCKER_BUILD_SSH_KEY is not configured as a repo secret, GitHub resolves it to an empty string and passes that through. Confirm the upstream cpflow-deploy-review-app.yml reusable workflow treats an empty value as "no SSH key" (i.e. skips --mount=type=ssh) rather than failing the build.
| [`control-plane-flow` CI automation guide](https://github.com/shakacode/control-plane-flow/blob/main/docs/ci-automation.md). | ||
|
|
||
| Current workflow wrappers are pinned to merged upstream `control-plane-flow` | ||
| commit `c0ca750c4838dacf6a1d81d52cb151301516cb77` for downstream testing. After |
There was a problem hiding this comment.
This SHA is a temporary test pin acknowledged as needing a follow-up repinning. Consider opening a tracking issue or adding a # TODO: repin to v5.0.1 comment directly on each uses: line in the workflow wrappers so the follow-up is harder to miss after v5.0.1 ships.
|
|
||
| root = Pathname.new(__dir__).join("..").expand_path | ||
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")].sort | ||
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")] |
There was a problem hiding this comment.
Removing .sort makes the changed list printed at the end non-deterministic across filesystems. No correctness impact, but CI output becomes harder to compare across runs.
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")] | |
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")].sort |
|
+review-app-deploy |
🎉 Deploy Complete!Open Review AppDeployment successful for PR #747, commit 9e5a0dd 🎮 Control Plane Console |
github-actions
Bot
temporarily deployed
to
review/qa-react-webpack-rails-tutorial-747
🎉 Deploy Complete!Open Review AppDeployment successful for PR #747, commit c659043 🎮 Control Plane Console |
github-actions
Bot
temporarily deployed
to
review/qa-react-webpack-rails-tutorial-747
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c659043f00
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ## Staging And Production | ||
|
|
||
| ### GitHub Actions variables | ||
| Staging deploys use the same `CPLN_TOKEN_STAGING` secret plus `STAGING_APP_NAME`. |
There was a problem hiding this comment.
Update this setup note to include CPLN_ORG_STAGING as required for staging deploys. The pinned upstream reusable workflow validates both CPLN_ORG_STAGING and STAGING_APP_NAME before running, so users who follow this text literally will configure only STAGING_APP_NAME and hit a hard validation failure in cpflow-deploy-staging.
Useful? React with 👍 / 👎.
|
Code Review - Overview: consolidates cpflow docs, replaces secrets:inherit with explicit secrets, gates CPLN_TOKEN_PRODUCTION behind protected GitHub Environment. Repinned from 3e0e7e1 to 54a6b8a. |
|
Code Review - Full Report Overview This PR consolidates downstream cpflow setup documentation into Security - Strong improvements
Issues
Minor / Positive
|
| You asked for review app help. These commands are generated by [cpflow](https://github.com/shakacode/control-plane-flow). | ||
| These commands are generated by [cpflow](https://github.com/shakacode/control-plane-flow). | ||
| For full setup, version-pinning, and troubleshooting details, see the upstream | ||
| [CI automation guide](https://github.com/shakacode/control-plane-flow/blob/54a6b8a066ac0a59c49e95a866e21b5633e2b1f9/docs/ci-automation.md). |
There was a problem hiding this comment.
This documentation link is pinned to the same 54a6b8a commit SHA used in the workflow uses: refs. Unlike workflow pins, documentation links don't benefit from immutability — they just become stale. When the wrappers are eventually repinned to v5.0.1, this link will silently point to old docs. Consider linking to main instead:
| [CI automation guide](https://github.com/shakacode/control-plane-flow/blob/54a6b8a066ac0a59c49e95a866e21b5633e2b1f9/docs/ci-automation.md). | |
| [CI automation guide](https://github.com/shakacode/control-plane-flow/blob/main/docs/ci-automation.md). |
|
|
||
| root = Pathname.new(__dir__).join("..").expand_path | ||
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")].sort | ||
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")] |
There was a problem hiding this comment.
The .sort was removed here, making the order in which files are processed — and the updated <path> lines printed to stdout — non-deterministic (filesystem order varies by OS and filesystem). The functional result is the same, but the output is less predictable, which can confuse diffs in CI logs. Consider restoring the sort:
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")] | |
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")].sort |
| if: github.event.inputs.confirm_promotion == 'promote' | ||
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-promote-staging-to-production.yml@3e0e7e1f0a35c15648cc9254b573b058d77ca8c4 | ||
| uses: shakacode/control-plane-flow/.github/workflows/cpflow-promote-staging-to-production.yml@54a6b8a066ac0a59c49e95a866e21b5633e2b1f9 | ||
| with: |
There was a problem hiding this comment.
Good pattern. One thing to confirm: the upstream reusable workflow's production job reads CPLN_ORG_PRODUCTION and PRODUCTION_APP_NAME from Environment variables (not secrets), so GitHub will auto-inject those from the production Environment after the approval gate. This only works if those values are set as Environment variables on the production Environment (not as repository variables). The docs spell this out, but it's easy to misconfigure as repo-level variables and get a silent failure at promotion time.
🎉 Deploy Complete!Open Review AppDeployment successful for PR #747, commit 3319280 🎮 Control Plane Console |
Code ReviewOverviewThis PR makes three coordinated changes: (1) repins all generated cpflow workflow wrappers from a temporary commit SHA to the The security direction is clearly correct. A few things worth noting: Security: Production Token Gating ✅Moving Security:
|
|
|
||
| root = Pathname.new(__dir__).join("..").expand_path | ||
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")].sort | ||
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")] |
There was a problem hiding this comment.
.sort was removed, so workflow_paths is now in filesystem-dependent order. The replacement itself is idempotent, but sorted output made the "Changed:" log easier to audit in CI. Consider restoring it:
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")] | |
| workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")].sort |
| control_plane_flow_ref: 3e0e7e1f0a35c15648cc9254b573b058d77ca8c4 | ||
| staging_app_branch_default: "" | ||
| secrets: inherit | ||
| staging_app_branch_default: "master" |
There was a problem hiding this comment.
Previously staging_app_branch_default: "" let the upstream reusable workflow use its own default. Now "master" is explicitly hardcoded, so this repo's staging branch no longer tracks the upstream default if it ever changes. That's almost certainly the right call for this repo — just flagging the subtle semantic shift so it's a conscious choice rather than an accidental one.
| actionlint -ignore "SC2129" .github/workflows/cpflow-*.yml | ||
| actionlint \ | ||
| -ignore "SC2129" \ | ||
| -ignore 'property "workflow_(ref|sha|repository|file_path)" is not defined in object type' \ |
There was a problem hiding this comment.
The alternation in this actionlint -ignore pattern (workflow_(ref|sha|repository|file_path)) relies on Go's regexp package supporting |. That works fine — just noting it so future maintainers know why it's a regex string rather than four separate -ignore flags.
✅ Review App DeletedReview app for PR #747 is deleted |
1 participant
Summary
Validation
Note
Medium Risk
Changes CI workflow wiring and production token handling (environment-gated promotion), which affects deploy/promote behavior even though application runtime code is untouched.
Overview
This PR documents downstream cpflow setup for this demo repo and aligns generated GitHub Actions with Control Plane Flow
v5.0.1.Documentation now spells out the minimal GitHub path (staging token only for review apps; staging variables for auto-deploy), matching Control Plane orgs/apps/secret dictionaries, and production promotion via a protected
productionEnvironment (not repo-levelCPLN_TOKEN_PRODUCTION). Generic reusable-workflow mechanics are delegated to the upstream CI guide; local docs are shortened to app-specific values and a canary checklist. Review-app naming drops the-prsegment: leaveREVIEW_APP_PREFIXunset so apps areqa-react-webpack-rails-tutorial-<PR>; old…-pr-…apps need manual cleanup.Workflow wrappers repin from a commit SHA to
@v5.0.1, drop obsoletecontrol_plane_flow_ref, replacesecrets: inheritwith named secrets, and wire promotion withproduction_environment: production(staging token only from the caller). Staging deploy triggersmasteronly.bin/pin-cpflow-github-refandbin/test-cpflow-github-flowenforce the new pinning and secret-passing rules.Reviewed by Cursor Bugbot for commit 3319280. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit