Validate selected setup templates and review-app health#363
Conversation
WalkthroughThis PR makes template parsing selective (CLI args or configured setup_app_templates, with missing-template validation) and extends template variable replacement (identity/secrets/policy, refactored image handling). It also adds review-app health-check env/defaults, a workflow wait-for-health step, corresponding generator spec, docs, and test fixtures. ChangesTemplate selection and variable substitution
Review-app workflow health checks and docs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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 |
![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: 5c0e1617b4
ℹ️ 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".
| def template_filenames | ||
| return existing_arg_template_filenames if config.args.any? | ||
|
|
||
| template_names = config.current[:setup_app_templates] |
There was a problem hiding this comment.
When templates validation runs without explicit template args (for example cpflow doctor --validations templates -a <unknown-app> or the preflight validation in setup-app) and the app name does not match any entry in controlplane.yml, config.current is nil; indexing it here raises a NoMethodError that is not rescued by run_validations or the command wrapper, so the CLI crashes instead of reporting the existing “Can't find current config” style error. Guard the nil case or go through config[:setup_app_templates] so unknown apps fail cleanly.
Useful? React with 👍 / 👎.
Code ReviewOverviewThis PR fixes a real bug: the duplicate-template validator was scanning every The change is well-scoped and the core logic is correct. A few findings below. Issues1. When 2.
3. Test coverage gap: deprecated The Minor / Positive Notes
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| def existing_arg_template_filenames | ||
| filenames = config.args.map { |name| @template_parser.template_filename(name) } | ||
| return [] unless filenames.all? { |filename| File.exist?(filename) } |
There was a problem hiding this comment.
The silent [] return when any template is missing is intentional (the calling command — e.g. apply-template — handles missing-template errors itself and produces a better message). But nothing in the code communicates that intent, making this look like a bug to future readers. A short comment would help:
| return [] unless filenames.all? { |filename| File.exist?(filename) } | |
| # Return [] so that apply-template can report its own "Missing templates" error; | |
| # don't raise a ValidationError here. | |
| return [] unless filenames.all? { |filename| File.exist?(filename) } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
spec/command/doctor_spec.rb (1)
36-77: ⚡ Quick winAdd explicit regression coverage for fallback-to-all template validation.
The updated tests now focus on selected-template behavior, but there’s no direct example asserting the fallback path when
setup_app_templatesis unset. Since that fallback is a stated objective, adding one case here would guard against regressions.Proposed spec addition
it "passes if selected templates have no issues" do result = run_cpflow_command("doctor", "--validations", "templates", "-a", app) expect(result[:status]).to eq(0) expect(result[:stderr]).to include("[PASS] templates") expect(result[:stderr]).not_to include("DEPRECATED") end + + it "falls back to validating all templates when setup_app_templates is not configured" do + app = dummy_test_app("nothing") + + result = run_cpflow_command("doctor", "--validations", "templates", "-a", app) + + expect(result[:status]).not_to eq(0) + expect(result[:stderr]).to include("[FAIL] templates") + expect(result[:stderr]).to include("- kind: gvc, name: #{app}") + end🤖 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 `@spec/command/doctor_spec.rb` around lines 36 - 77, Add a regression spec that verifies the fallback-to-all template validation when setup_app_templates is unset: create a new example in the doctor_spec that uses run_cpflow_command("doctor", "--validations", "templates", "-a", app) (or without -a to emulate unset setup_app_templates) against a dummy_test_app that relies on the fallback (e.g., "default" or a new fixture) and assert the command exits with success or failure as expected and that stderr contains the fallback behavior markers (e.g., "[PASS] templates" or expected failure messages); specifically reference setup_app_templates being nil/unset and ensure the test checks that templates validation runs over all templates rather than only selected ones to guard the fallback path.
🤖 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.
Nitpick comments:
In `@spec/command/doctor_spec.rb`:
- Around line 36-77: Add a regression spec that verifies the fallback-to-all
template validation when setup_app_templates is unset: create a new example in
the doctor_spec that uses run_cpflow_command("doctor", "--validations",
"templates", "-a", app) (or without -a to emulate unset setup_app_templates)
against a dummy_test_app that relies on the fallback (e.g., "default" or a new
fixture) and assert the command exits with success or failure as expected and
that stderr contains the fallback behavior markers (e.g., "[PASS] templates" or
expected failure messages); specifically reference setup_app_templates being
nil/unset and ensure the test checks that templates validation runs over all
templates rather than only selected ones to guard the fallback path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ef8ae8ec-721e-4119-8bac-9b1f8d39bf8e
📒 Files selected for processing (7)
lib/core/doctor_service.rblib/core/template_parser.rbspec/command/doctor_spec.rbspec/core/template_parser_spec.rbspec/dummy/.controlplane/controlplane.ymlspec/dummy/.controlplane/templates/app-review.ymlspec/dummy/.controlplane/templates/app-with-deprecated-variables-no-image.yml
| end | ||
|
|
||
| def replace_legacy_image_variable(yaml_file) | ||
| return yaml_file unless yaml_file.include?("APP_IMAGE") |
There was a problem hiding this comment.
"APP_IMAGE" is a substring of both "{{APP_IMAGE}}" and "{{APP_IMAGE_LINK}}". The current call order (modern vars replaced first in replace_image_variables) keeps this safe, but the guard is fragile — if ordering ever shifts, a template containing only {{APP_IMAGE_LINK}} would pass this check and gsub("APP_IMAGE", ...) would corrupt APP_IMAGE_LINK into {latest_image}_LINK.
A more defensive check:
| return yaml_file unless yaml_file.include?("APP_IMAGE") | |
| return yaml_file unless yaml_file.include?("APP_IMAGE") && !yaml_file.include?("{{APP_IMAGE") |
Or simply invert the responsibility: since replace_image_variables already detects the modern patterns, the legacy check only needs to fire when the modern patterns were not present — which could be expressed by checking for APP_IMAGE not surrounded by {{.
| expect(result[:stderr]).to include("APP_GVC -> {{APP_NAME}}") | ||
| expect(result[:stderr]).to include("APP_LOCATION -> {{APP_LOCATION}}") | ||
| expect(result[:stderr]).to include("APP_IMAGE -> {{APP_IMAGE}}") | ||
| expect(result[:stderr]).not_to include("APP_IMAGE -> {{APP_IMAGE}}") |
There was a problem hiding this comment.
Flipping this from to include to not_to include is correct for the new app-with-deprecated-variables-no-image.yml fixture, but it removes the only test that verified the APP_IMAGE → {{APP_IMAGE}} deprecation warning fires at all. Consider either:
- Adding a dedicated
it "warns about deprecated APP_IMAGE variable"spec backed by a fixture that contains the legacyAPP_IMAGEplaceholder (the existingapp-with-deprecated-variables.ymlalready has it, but that file triggerscp.latest_image— a small targeted stub in that one test would be enough), or - Verifying coverage via the
template_parser_specfind_deprecated_variablespath directly.
Without this, a regression that silently drops the APP_IMAGE deprecation path would go undetected.
Greptile SummaryThis PR fixes a false-positive duplicate-template validation error triggered when an app's
Confidence Score: 4/5Safe to merge; the core validation logic is correct and the two-phase missing-template flow for apply-template is intentional. The selected-template filtering, the lazy image fetch, and the lib/core/doctor_service.rb — the Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[DoctorService#validate_templates] --> B[template_filenames]
B --> C{config.args.any?}
C -- Yes\napply-template path --> D[existing_arg_template_filenames]
D --> E{All files exist?}
E -- No --> F[return empty array\nlet apply-template handle error]
E -- Yes --> G[return filenames]
C -- No\ndoctor / setup-app path --> H{setup_app_templates\nconfigured?}
H -- No --> I[Dir.glob all *.yml\nfull fallback]
H -- Yes --> J[map names to filenames]
J --> K[ensure_templates_exist!]
K --> L{Any missing?}
L -- Yes --> M[raise ValidationError\nMissing templates]
L -- No --> G
G --> N[TemplateParser#parse filenames]
I --> N
F --> N
N --> O[replace_variables per file]
O --> P{includes APP_IMAGE\nor APP_IMAGE_LINK?}
P -- Yes --> Q[cp.latest_image\nreplace image vars]
P -- No --> R[skip image fetch]
Q --> S[check_for_duplicate_templates]
R --> S
S --> T{Duplicates?}
T -- Yes --> U[raise ValidationError\nDuplicate templates]
T -- No --> V[warn_deprecated_template_variables]
Reviews (1): Last reviewed commit: "Validate selected setup templates" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def existing_arg_template_filenames | ||
| filenames = config.args.map { |name| @template_parser.template_filename(name) } | ||
| return [] unless filenames.all? { |filename| File.exist?(filename) } | ||
|
|
||
| filenames | ||
| end |
There was a problem hiding this comment.
When called in the apply-template pre-flight path, this returns [] if even one template file is absent, causing validate_templates to run duplicate/deprecation checks against zero templates and silently succeed. That's intentional because ApplyTemplate#ensure_templates! handles the error itself. However, if any future command gains positional-arg template names (REQUIRES_ARGS = true) and also uses VALIDATIONS = %w[templates] without its own missing-template guard, the validation will silently pass and the error will be swallowed. A brief comment documenting that this deliberately delegates to the caller's own error handling would prevent that footgun.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| def existing_arg_template_filenames | ||
| filenames = config.args.map { |name| @template_parser.template_filename(name) } | ||
| return [] unless filenames.all? { |filename| File.exist?(filename) } | ||
|
|
||
| filenames | ||
| end |
There was a problem hiding this comment.
Bug: silent pass when CLI-arg templates are missing
When any template file doesn't exist, this returns [] instead of raising a ValidationError. parse([]) then produces zero templates, check_for_duplicate_templates([]) trivially passes, and the user sees [PASS] templates — with no indication that their template arguments were never validated.
This is inconsistent with ensure_templates_exist! (used for setup_app_templates) and with apply-template's ensure_templates!, both of which raise an error on missing files.
| def existing_arg_template_filenames | |
| filenames = config.args.map { |name| @template_parser.template_filename(name) } | |
| return [] unless filenames.all? { |filename| File.exist?(filename) } | |
| filenames | |
| end | |
| def existing_arg_template_filenames | |
| filenames = config.args.map { |name| @template_parser.template_filename(name) } | |
| ensure_templates_exist!(config.args, filenames) | |
| filenames | |
| end |
| expect(result[:stderr]).to include("APP_GVC -> {{APP_NAME}}") | ||
| expect(result[:stderr]).to include("APP_LOCATION -> {{APP_LOCATION}}") | ||
| expect(result[:stderr]).to include("APP_IMAGE -> {{APP_IMAGE}}") | ||
| expect(result[:stderr]).not_to include("APP_IMAGE -> {{APP_IMAGE}}") |
There was a problem hiding this comment.
Test coverage gap: APP_IMAGE deprecation warning
The old test verified that APP_IMAGE -> {{APP_IMAGE}} is reported as deprecated (using the app-with-deprecated-variables fixture which contained bare APP_IMAGE). Switching to the -no-image fixture and asserting not_to include removes the only test for that deprecation path.
new_variables in TemplateParser still maps "APP_IMAGE" => "{{APP_IMAGE}}", so the deprecation is still detected — it just goes untested. Consider keeping a separate test (or a second fixture) that covers a template with a bare APP_IMAGE to confirm the warning still fires when expected.
| end | ||
|
|
||
| def replace_legacy_image_variable(yaml_file) | ||
| return yaml_file unless yaml_file.include?("APP_IMAGE") |
There was a problem hiding this comment.
Guard check is correct but worth a note on ordering
yaml_file here has already had {{APP_IMAGE}} replaced by replace_image_variables, so any remaining "APP_IMAGE" substring is genuinely the bare legacy form. The guard is safe.
One subtle point: "APP_IMAGE" is a substring of "{{APP_IMAGE}}", so if replace_image_variables were ever skipped (e.g., the include? guard there was wrong), this guard could false-positive on a {{APP_IMAGE}} literal. The current design prevents that because both methods are called in strict order, but it's worth keeping in mind if the chain is ever refactored.
Code ReviewOverviewThis PR scopes template validation in Bug:
|
justin808
changed the title
|
|
||
| def existing_arg_template_filenames | ||
| filenames = config.args.map { |name| @template_parser.template_filename(name) } | ||
| return [] unless filenames.all? { |filename| File.exist?(filename) } |
There was a problem hiding this comment.
Silent empty return masks missing-template errors for explicit args
When doctor --validations templates -a myapp some-missing-template is invoked, this silently returns [], so check_for_duplicate_templates([]) and warn_deprecated_template_variables both no-op and [PASS] templates is printed. The user gets no indication that the specified template was not found.
This asymmetry — setup_app_templates raises ValidationError via ensure_templates_exist!, but explicit CLI args silently skip — could be surprising. If the intent is to preserve apply-template's upstream behavior (where the command itself handles missing args), a short comment would help future readers understand the deliberate choice.
| return [] unless filenames.all? { |filename| File.exist?(filename) } | |
| # apply-template handles missing explicit args itself; return empty so doctor | |
| # skips validation rather than double-erroring. | |
| return [] unless filenames.all? { |filename| File.exist?(filename) } |
| def replace_legacy_image_variable(yaml_file) | ||
| return yaml_file unless yaml_file.include?("APP_IMAGE") | ||
|
|
||
| yaml_file.gsub("APP_IMAGE", cp.latest_image) |
There was a problem hiding this comment.
Two calls to cp.latest_image are possible
replace_image_variables fetches latest_image at line 68. If the same template file also contains the legacy bare APP_IMAGE token, replace_legacy_image_variable calls cp.latest_image again — two API calls for what should be one value. In theory they could even return different images if a new image is pushed between the two calls.
Consider caching the result at the replace_variables call-site and threading it through:
def replace_legacy_image_variable(yaml_file, latest_image = nil)
return yaml_file unless yaml_file.include?("APP_IMAGE")
latest_image ||= cp.latest_image
yaml_file.gsub("APP_IMAGE", latest_image)
endOr resolve once in replace_variables and pass it down to both helpers.
| curl_max_time: ${{ env.REVIEW_APP_HEALTH_CHECK_CURL_MAX_TIME }} | ||
|
|
||
| - name: Retrieve app URL | ||
| if: steps.config.outputs.ready == 'true' && steps.source.outputs.allowed == 'true' && (steps.check-app.outputs.exists == 'true' || steps.setup-review-app.outcome == 'success') |
There was a problem hiding this comment.
"Retrieve app URL" doesn't guard on health-check success
The if: condition here doesn't include steps.health-check.outputs.healthy == 'true'. This is safe today because the health-check step exits non-zero on failure, causing GitHub Actions to skip subsequent steps. However, if continue-on-error: true were ever added to the health-check step, this step would run after an unhealthy deploy and the finalize step would still mark the deployment as success (since JOB_STATUS would be "success").
Consider adding an explicit guard for resilience:
| if: steps.config.outputs.ready == 'true' && steps.source.outputs.allowed == 'true' && (steps.check-app.outputs.exists == 'true' || steps.setup-review-app.outcome == 'success') | |
| if: steps.config.outputs.ready == 'true' && steps.source.outputs.allowed == 'true' && (steps.check-app.outputs.exists == 'true' || steps.setup-review-app.outcome == 'success') && steps.health-check.outputs.healthy == 'true' |
Code ReviewOverviewThis PR fixes a real-world bug (TanStack review apps failing cpflow 5.1.1 duplicate-template validation) and closes a CI gap (review-app deploys marking success before the workload was actually healthy). The two changes are independent and well-scoped. What's Good
IssuesThree inline comments were posted; summaries below:
Minor Notes
|
1 participant
Summary
setup_app_templatesfor doctor/setup-appsetup_app_templatesare configuredapply-template's existing missing-template handling for explicit argsWhy
TanStack review apps hit cpflow 5.1.1 duplicate-template validation because the app intentionally has
app.ymlandapp-review.ymlthat both render the same GVC name, but only one is selected for a review app setup. The validator was parsing every template file instead of the selected set.While validating the rollout, the legacy review app also exposed a deploy-status gap: the reusable review-app deploy workflow could mark a deployment successful after updating the image and finding an endpoint, even if the primary workload was crash-looping. Review-app deploys now reuse the existing
cpflow-wait-for-healthaction, withREVIEW_APP_HEALTH_CHECK_*repo-variable overrides.Verification
CPLN_ORG=dummy-test-org bundle exec rspec spec/command/doctor_spec.rb spec/core/template_parser_spec.rb spec/command/apply_template_spec.rb:8bundle exec rubocop lib/core/doctor_service.rb lib/core/template_parser.rb spec/command/doctor_spec.rb spec/core/template_parser_spec.rb spec/command/apply_template_spec.rbruby -c lib/core/doctor_service.rb && ruby -c lib/core/template_parser.rbCPLN_ORG=dummy-test-org bundle exec rspec spec/command/generate_github_actions_spec.rb spec/command/deploy_image_unit_spec.rbbundle exec rubocop spec/command/generate_github_actions_spec.rb.github/workflows/cpflow-*.ymland.github/actions/cpflow-*/*.ymlgit diff --checkNotes
actionlint .github/workflows/cpflow-deploy-review-app.ymlstill reports pre-existing false positives for GitHub'sjob.workflow_repository,job.workflow_sha, andjob.workflow_refcontext fields; it did not report issues for the new health-check expressions.Summary by CodeRabbit
New Features
Bug Fixes
CI
Tests