Validate selected setup templates and review-app health

.github/workflows/cpflow-deploy-review-app.yml

@@ -439,8 +439,23 @@ jobs:
 
           cpflow deploy-image "${deploy_args[@]}"
 
-      - name: Retrieve app URL
+      - name: Wait for deployment health
         if: steps.config.outputs.ready == 'true' && steps.source.outputs.allowed == 'true' && (steps.check-app.outputs.exists == 'true' || steps.setup-review-app.outcome == 'success')
+        id: health-check
+        uses: ./.cpflow/.github/actions/cpflow-wait-for-health
+        with:
+          workload_name: ${{ env.PRIMARY_WORKLOAD || 'rails' }}
+          app_name: ${{ steps.review-config.outputs.app_name }}
+          org: ${{ steps.review-config.outputs.cpln_org }}
+          # Review apps use their own health knobs so teams can keep production
+          # promotion stricter or slower without changing PR feedback behavior.
+          max_retries: ${{ vars.REVIEW_APP_HEALTH_CHECK_RETRIES || '24' }}
+          interval_seconds: ${{ vars.REVIEW_APP_HEALTH_CHECK_INTERVAL || '15' }}
+          accepted_statuses: ${{ vars.REVIEW_APP_HEALTH_CHECK_ACCEPTED_STATUSES || '200 301 302' }}
+          curl_max_time: ${{ vars.REVIEW_APP_HEALTH_CHECK_CURL_MAX_TIME || '10' }}
+
+      - 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') && steps.health-check.outcome == 'success'
         id: workload
         working-directory: app
         shell: bash

docs/ci-automation.md

@@ -173,7 +173,11 @@ disambiguate generated review-app config:
 
 - `CPLN_ORG_STAGING`: override the staging/review org inferred from `cpln_org`, for example `company-staging`
 - `REVIEW_APP_PREFIX`: override the inferred review-app prefix; required only when multiple review app prefixes exist in `controlplane.yml`
-- `PRIMARY_WORKLOAD`: override the public workload used to discover the public endpoint and do production health checks; defaults to `rails`
+- `PRIMARY_WORKLOAD`: override the public workload used to discover the public endpoint and do review/production health checks; defaults to `rails`
+- `REVIEW_APP_HEALTH_CHECK_RETRIES`: override review-app health polling attempts; defaults to `24`
+- `REVIEW_APP_HEALTH_CHECK_INTERVAL`: override seconds between review-app health attempts; defaults to `15`
+- `REVIEW_APP_HEALTH_CHECK_ACCEPTED_STATUSES`: override space-separated healthy HTTP statuses for review apps; defaults to `200 301 302`
+- `REVIEW_APP_HEALTH_CHECK_CURL_MAX_TIME`: override per-request review-app curl timeout in seconds; defaults to `10`
 
 If `controlplane.yml` defines more than one app with
 `match_if_app_name_starts_with: true`, inference intentionally fails. Set
@@ -461,6 +465,12 @@ The action will start an SSH agent, add the key, write `known_hosts`, and pass `
 - Attempts a rollback of every configured application workload if the new production image does not come up healthy.
 - Creates a GitHub release after a successful promotion.
 
+`cpflow-deploy-review-app.yml`
+
+- Builds and deploys the pull request image after the review app exists or is created.
+- Waits for `PRIMARY_WORKLOAD` to report `readyLatest=true` and return an accepted HTTP status before marking the GitHub deployment successful.
+- Uses `REVIEW_APP_HEALTH_CHECK_*` repository variables to tune review-app health checks independently from production promotion.
+
 `cpflow-cleanup-stale-review-apps.yml`
 
 - Runs nightly and on demand.

docs/commands.md

@@ -554,7 +554,7 @@ cpflow terraform import
 Regenerates the generated cpflow GitHub Actions wrappers and helper files
 from the currently installed cpflow gem. Use this after updating the
 cpflow gem so checked-in workflow wrappers move to the matching upstream
-release tag, for example `v5.1.0`.
+release tag, for example `v5.1.1`.
 
 If the existing generated staging workflow uses a custom single staging
 branch, the command preserves it. Pass `--staging-branch BRANCH` to set or

lib/core/doctor_service.rb

@@ -34,14 +34,11 @@ def run_validations(validations, silent_if_passing: false) # rubocop:disable Met
     exit(ExitCode::ERROR_DEFAULT) if @any_failed_validation
   end
 
-  def validate_config
-    check_for_app_names_contained_in_others
-  end
+  def validate_config = check_for_app_names_contained_in_others
 
   def validate_templates
     @template_parser = TemplateParser.new(@command)
-    filenames = Dir.glob("#{@template_parser.template_dir}/*.yml")
-    templates = @template_parser.parse(filenames)
+    templates = @template_parser.parse(template_filenames)
 
     check_for_duplicate_templates(templates)
     warn_deprecated_template_variables
@@ -88,6 +85,40 @@ def check_for_duplicate_templates(templates)
     raise ValidationError, "#{Shell.color("ERROR: #{message}", :red)}\n#{list}"
   end
 
+  def template_filenames
+    return existing_arg_template_filenames if config.args.any?
+
+    message = "ERROR: Can't find current config, please specify an app."
+    raise ValidationError, Shell.color(message, :red) if config.current.nil?
+
+    template_names = config.current[:setup_app_templates]
+    # Fall back to every template in the directory when setup_app_templates is unconfigured.
+    return Dir.glob("#{@template_parser.template_dir}/*.yml") if template_names.nil? || template_names.empty?
+
+    # When setup_app_templates is configured, validate only that selected subset,
+    # including the deprecation scan.
+    resolve_template_filenames(template_names)
+  end
+
+  def existing_arg_template_filenames = resolve_template_filenames(config.args)
+
+  def resolve_template_filenames(template_names)
+    filenames = template_names.map { |name| @template_parser.template_filename(name) }
+    ensure_templates_exist!(template_names, filenames)
+    filenames
+  end
+
+  def ensure_templates_exist!(template_names, filenames)
+    missing_templates = template_names.zip(filenames).reject { |_, filename| File.exist?(filename) }
+    return if missing_templates.empty?
+
+    missing_templates_str = missing_templates.map do |name, filename|
+      "  - #{name} (#{filename})"
+    end.join("\n")
+    message = "#{Shell.color('Missing templates:', :red)}\n#{missing_templates_str}"
+    raise ValidationError, message
+  end
+
   def warn_deprecated_template_variables
     deprecated_variables = @template_parser.deprecated_variables
     return if deprecated_variables.empty?

lib/core/template_parser.rb

@@ -37,38 +37,72 @@ def parse(filenames)
   private
 
   def replace_variables(yaml_file) # rubocop:disable Metrics/MethodLength
+    original_yaml_file = yaml_file
+    yaml_file = replace_legacy_variables(yaml_file)
+
     yaml_file = yaml_file
                 .gsub("{{APP_ORG}}", config.org)
                 .gsub("{{APP_NAME}}", config.app)
                 .gsub("{{APP_LOCATION}}", config.location)
                 .gsub("{{APP_LOCATION_LINK}}", config.location_link)
-                .gsub("{{APP_IMAGE}}", cp.latest_image)
-                .gsub("{{APP_IMAGE_LINK}}", config.image_link(cp.latest_image))
                 .gsub("{{APP_IDENTITY}}", config.identity)
                 .gsub("{{APP_IDENTITY_LINK}}", config.identity_link)
                 .gsub("{{APP_SECRETS}}", config.secrets)
                 .gsub("{{APP_SECRETS_POLICY}}", config.secrets_policy)
+    yaml_file = replace_image_variables(yaml_file)
 
     config.shared_secret_placeholders.each do |placeholder, secret_name|
       yaml_file = yaml_file.gsub(placeholder, secret_name)
     end
 
-    find_deprecated_variables(yaml_file)
+    find_deprecated_variables(original_yaml_file)
+    yaml_file
+  end
+
+  def replace_image_variables(yaml_file)
+    has_image = yaml_file.include?("{{APP_IMAGE}}")
+    has_image_link = yaml_file.include?("{{APP_IMAGE_LINK}}")
+    return yaml_file unless has_image || has_image_link
 
-    # Kept for backwards compatibility
+    yaml_file = yaml_file.gsub("{{APP_IMAGE}}", latest_image) if has_image
+    yaml_file = yaml_file.gsub("{{APP_IMAGE_LINK}}", config.image_link(latest_image)) if has_image_link
     yaml_file
-      .gsub("APP_ORG", config.org)
-      .gsub("APP_GVC", config.app)
-      .gsub("APP_LOCATION", config.location)
-      .gsub("APP_IMAGE", cp.latest_image)
+  end
+
+  # Kept for backwards compatibility.
+  def replace_legacy_variables(yaml_file)
+    yaml_file
+      .gsub(deprecated_variable_pattern("APP_ORG"), config.org)
+      .gsub(deprecated_variable_pattern("APP_GVC"), config.app)
+      .gsub(deprecated_variable_pattern("APP_LOCATION"), config.location)
+      .then { |updated_yaml| replace_legacy_image_variable(updated_yaml) }
+  end
+
+  def replace_legacy_image_variable(yaml_file)
+    return yaml_file unless deprecated_variable_used?(yaml_file, "APP_IMAGE")
+
+    yaml_file.gsub(deprecated_variable_pattern("APP_IMAGE"), latest_image)
+  end
+
+  def latest_image
+    # Share one image value across modern and legacy image replacements in this parser instance.
+    @latest_image ||= cp.latest_image
   end
 
   def find_deprecated_variables(yaml_file)
     new_variables.each do |old_key, new_key|
-      @deprecated_variables[old_key] = new_key if yaml_file.include?(old_key)
+      @deprecated_variables[old_key] = new_key if deprecated_variable_used?(yaml_file, old_key)
     end
   end
 
+  def deprecated_variable_used?(yaml_file, old_key)
+    yaml_file.match?(deprecated_variable_pattern(old_key))
+  end
+
+  def deprecated_variable_pattern(old_key)
+    /(?<!\{\{)\b#{Regexp.escape(old_key)}\b(?!\}\})/
+  end
+
   def new_variables
     {
       "APP_ORG" => "{{APP_ORG}}",

spec/command/doctor_spec.rb

@@ -33,7 +33,7 @@
   end
 
   context "when validating templates" do
-    let!(:app) { dummy_test_app }
+    let!(:app) { dummy_test_app("default") }
 
     it "raises error if app is not provided" do
       result = run_cpflow_command("doctor", "--validations", "templates")
@@ -42,8 +42,8 @@
       expect(result[:stderr]).to include("App is required for templates validation")
     end
 
-    it "fails if there are duplicate templates" do
-      stub_template_filenames("app", "app-without-identity", "rails")
+    it "fails if selected templates contain duplicate rendered kind/names" do
+      app = dummy_test_app("duplicate-templates")
 
       result = run_cpflow_command("doctor", "--validations", "templates", "-a", app)
 
@@ -52,18 +52,72 @@
       expect(result[:stderr]).to include("- kind: gvc, name: #{app}")
     end
 
-    it "passes if there are no issues" do
-      stub_template_filenames("app", "rails")
+    it "fails cleanly if the current app config is missing" do
+      progress = StringIO.new
+      config = instance_double(Config, args: [], current: nil)
+      command = instance_double(described_class, config: config, progress: progress)
+
+      expect { DoctorService.new(command).run_validations(["templates"]) }.to raise_error(SystemExit)
+      expect(progress.string).to include("[FAIL] templates")
+      expect(progress.string).to include("Can't find current config, please specify an app.")
+      expect(progress.string).not_to include("NoMethodError")
+    end
+
+    it "fails if a selected setup template is missing" do
+      app = dummy_test_app("missing-template")
 
       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("Missing templates")
+      expect(result[:stderr]).to include("- nonexistent")
+    end
+
+    it "fails if an explicitly named template is missing" do
+      Dir.mktmpdir do |dir|
+        progress = StringIO.new
+        config = instance_double(Config, args: ["nonexistent"], app_cpln_dir: dir)
+        command = instance_double(described_class, config: config, progress: progress)
+
+        expect { DoctorService.new(command).run_validations(["templates"]) }.to raise_error(SystemExit)
+        expect(progress.string).to include("[FAIL] templates")
+        expect(progress.string).to include("Missing templates")
+        expect(progress.string).to include("- nonexistent")
+      end
+    end
+
+    it "validates all templates if no setup templates are selected" do
+      app = dummy_test_app("nothing")
+      stub_template_filenames("app", "app-without-identity")
+
+      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
+
+    it "passes if unselected templates render duplicate kind/names" do
+      app = dummy_test_app("alternate-app-template")
+
+      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 "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 "warns about deprecated variables" do
-      stub_template_filenames("app-with-deprecated-variables")
+      app = dummy_test_app("deprecated-template")
 
       result = run_cpflow_command("doctor", "--validations", "templates", "-a", app)
 
@@ -73,7 +127,41 @@
       expect(result[:stderr]).to include("APP_ORG -> {{APP_ORG}}")
       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}}")
+    end
+
+    it "warns about a deprecated bare APP_IMAGE variable" do
+      Dir.mktmpdir do |dir|
+        FileUtils.mkdir_p("#{dir}/templates")
+        File.write("#{dir}/templates/legacy.yml", <<~YAML)
+          kind: identity
+          name: APP_IMAGE
+        YAML
+
+        progress = StringIO.new
+        config = instance_double(
+          Config,
+          args: [],
+          current: { setup_app_templates: ["legacy"] },
+          app_cpln_dir: dir,
+          org: "test-org",
+          app: "test-app",
+          location: "aws-us-east-2",
+          location_link: "/org/test-org/location/aws-us-east-2",
+          identity: "test-app-identity",
+          identity_link: "/org/test-org/gvc/test-app/identity/test-app-identity",
+          secrets: "test-app-secrets",
+          secrets_policy: "test-app-secrets-policy",
+          shared_secret_placeholders: {}
+        )
+        cp = instance_double(Controlplane, latest_image: "test-app:1")
+        command = instance_double(described_class, config: config, progress: progress, cp: cp)
+
+        DoctorService.new(command).run_validations(["templates"])
+
+        expect(progress.string).to include("[PASS] templates")
+        expect(progress.string).to include("APP_IMAGE -> {{APP_IMAGE}}")
+      end
     end
   end
 

spec/command/generate_github_actions_spec.rb

@@ -636,6 +636,26 @@ def shared_workflow_path(name)
       expect(contents).to include("📋 [View Completed Action Build and Deploy Logs]")
     end
 
+    it "waits for review-app workload health before finalizing deployment success" do
+      contents = reusable_review_app_workflow_path.read
+
+      expect(contents).to include("- name: Wait for deployment health")
+      expect(contents).to include("id: health-check")
+      expect(contents).to include("uses: ./.cpflow/.github/actions/cpflow-wait-for-health")
+      expect(contents).to include("workload_name: ${{ env.PRIMARY_WORKLOAD || 'rails' }}")
+      expect(contents).to include("app_name: ${{ steps.review-config.outputs.app_name }}")
+      expect(contents).to include("org: ${{ steps.review-config.outputs.cpln_org }}")
+      expect(contents).not_to include("REVIEW_APP_HEALTH_CHECK_ACCEPTED_STATUSES:")
+      expect(contents).to include("max_retries: ${{ vars.REVIEW_APP_HEALTH_CHECK_RETRIES || '24' }}")
+      expect(contents).to include(
+        "interval_seconds: ${{ vars.REVIEW_APP_HEALTH_CHECK_INTERVAL || '15' }}"
+      )
+      expect(contents).to include(
+        "accepted_statuses: ${{ vars.REVIEW_APP_HEALTH_CHECK_ACCEPTED_STATUSES || '200 301 302' }}"
+      )
+      expect(contents).to include("curl_max_time: ${{ vars.REVIEW_APP_HEALTH_CHECK_CURL_MAX_TIME || '10' }}")
+    end
+
     it "supports an animated deploy status icon with a repository override" do
       contents = reusable_review_app_workflow_path.read