diff --git a/.github/workflows/cpflow-deploy-review-app.yml b/.github/workflows/cpflow-deploy-review-app.yml index 2f9e1698..a879eba8 100644 --- a/.github/workflows/cpflow-deploy-review-app.yml +++ b/.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 diff --git a/docs/ci-automation.md b/docs/ci-automation.md index 195af935..1d41f43c 100644 --- a/docs/ci-automation.md +++ b/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. diff --git a/docs/commands.md b/docs/commands.md index 1c52b0de..a2a9ff8c 100644 --- a/docs/commands.md +++ b/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 diff --git a/lib/core/doctor_service.rb b/lib/core/doctor_service.rb index 7bcdfc2d..c6b1b183 100644 --- a/lib/core/doctor_service.rb +++ b/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,41 @@ 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) + unique_template_names = template_names.uniq + filenames = unique_template_names.map { |name| @template_parser.template_filename(name) } + ensure_templates_exist!(unique_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? diff --git a/lib/core/template_parser.rb b/lib/core/template_parser.rb index a01984e8..7861d73f 100644 --- a/lib/core/template_parser.rb +++ b/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) + /(? "{{APP_ORG}}", diff --git a/spec/command/doctor_spec.rb b/spec/command/doctor_spec.rb index c8bba2fe..3cd0e8d8 100644 --- a/spec/command/doctor_spec.rb +++ b/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,8 +52,64 @@ 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 when all templates render cleanly without setup templates selected" do + app = dummy_test_app("nothing") + stub_template_filenames("app") + + result = run_cpflow_command("doctor", "--validations", "templates", "-a", app) + + expect(result[:status]).to eq(0) + expect(result[:stderr]).to include("[PASS] templates") + 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) @@ -62,8 +118,16 @@ 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 +137,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 diff --git a/spec/command/generate_github_actions_spec.rb b/spec/command/generate_github_actions_spec.rb index f51a9cd9..257ac15e 100644 --- a/spec/command/generate_github_actions_spec.rb +++ b/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 diff --git a/spec/core/template_parser_spec.rb b/spec/core/template_parser_spec.rb index 7a396423..9f956373 100644 --- a/spec/core/template_parser_spec.rb +++ b/spec/core/template_parser_spec.rb @@ -11,7 +11,6 @@ app: "test-review-123", location: "aws-us-east-2", location_link: "/org/test-org/location/aws-us-east-2", - image_link: "/org/test-org/image/test-review-123:1", identity: "test-review-123-identity", identity_link: "/org/test-org/gvc/test-review-123/identity/test-review-123-identity", secrets: "test-review-secrets", @@ -19,11 +18,18 @@ shared_secret_placeholders: { "{{SHARED_SECRET_DATABASE}}" => "test-shared-database-secrets" } - ) + ).tap do |config| + allow(config).to receive(:image_link) + .with(latest_image) + .and_return("/org/test-org/image/#{latest_image}") + end + end + let(:latest_image) { "test-review-123:1" } + let(:cp) { instance_double(Controlplane, latest_image: latest_image) } + let(:parser) do + command = instance_double(Command::ApplyTemplate, config: config, cp: cp) + described_class.new(command) end - let(:cp) { instance_double(Controlplane, latest_image: "test-review-123:1") } - let(:command) { instance_double(Command::ApplyTemplate, config: config, cp: cp) } - let(:parser) { described_class.new(command) } let(:template_file) do Tempfile.create(["shared-secret-template", ".yml"]).tap do |file| file.write(<<~YAML) @@ -57,5 +63,80 @@ } ) end + + it "does not fetch the latest image when templates do not use image placeholders" do + allow(cp).to receive(:latest_image) + + parser.parse([template_file.path]) + + expect(cp).not_to have_received(:latest_image) + end + + it "does not treat APP_IMAGE_LINK as the bare APP_IMAGE legacy variable" do + template_file.rewind + template_file.truncate(0) + template_file.write(<<~YAML) + kind: workload + name: rails + spec: + containers: + - name: rails + image: "{{APP_IMAGE_LINK}}" + YAML + template_file.rewind + + parsed_template = parser.parse([template_file.path]).first + + expect(parsed_template.dig("spec", "containers", 0, "image")) + .to eq("/org/test-org/image/test-review-123:1") + expect(parser.deprecated_variables).not_to include("APP_IMAGE") + end + + context "when the modern image replacement contains deprecated variable text" do + let(:latest_image) { "ghcr.io/company/APP_IMAGE-utils:v1" } + + it "does not warn about the modern placeholder" do + template_file.rewind + template_file.truncate(0) + template_file.write(<<~YAML) + kind: workload + name: rails + spec: + containers: + - name: rails + image: "{{APP_IMAGE}}" + YAML + template_file.rewind + + parsed_template = parser.parse([template_file.path]).first + + expect(parsed_template.dig("spec", "containers", 0, "image")).to eq(latest_image) + expect(parser.deprecated_variables).not_to include("APP_IMAGE") + end + end + + it "fetches the latest image only once when modern and legacy image variables are present" do + template_file.rewind + template_file.truncate(0) + template_file.write(<<~YAML) + kind: workload + name: rails + spec: + containers: + - name: rails + image: "{{APP_IMAGE_LINK}}" + env: + - name: LEGACY_IMAGE + value: APP_IMAGE + YAML + template_file.rewind + + parsed_template = parser.parse([template_file.path]).first + + expect(parsed_template.dig("spec", "containers", 0, "image")) + .to eq("/org/test-org/image/test-review-123:1") + expect(parsed_template.dig("spec", "env", 0, "value")).to eq("test-review-123:1") + expect(cp).to have_received(:latest_image).once + end end end diff --git a/spec/dummy/.controlplane/controlplane.yml b/spec/dummy/.controlplane/controlplane.yml index 261658be..51cd1b93 100644 --- a/spec/dummy/.controlplane/controlplane.yml +++ b/spec/dummy/.controlplane/controlplane.yml @@ -48,6 +48,35 @@ apps: - app - rails + dummy-test-duplicate-templates-{GLOBAL_IDENTIFIER}: + <<: *common + + match_if_app_name_starts_with: true + setup_app_templates: + - app + - app-without-identity + + dummy-test-alternate-app-template-{GLOBAL_IDENTIFIER}: + <<: *common + + match_if_app_name_starts_with: true + setup_app_templates: + - app-review + + dummy-test-deprecated-template-{GLOBAL_IDENTIFIER}: + <<: *common + + match_if_app_name_starts_with: true + setup_app_templates: + - app-with-deprecated-variables-no-image + + dummy-test-missing-template-{GLOBAL_IDENTIFIER}: + <<: *common + + match_if_app_name_starts_with: true + setup_app_templates: + - nonexistent + dummy-test-full-{GLOBAL_IDENTIFIER}: <<: *common diff --git a/spec/dummy/.controlplane/templates/app-review.yml b/spec/dummy/.controlplane/templates/app-review.yml new file mode 100644 index 00000000..03e64289 --- /dev/null +++ b/spec/dummy/.controlplane/templates/app-review.yml @@ -0,0 +1,11 @@ +kind: gvc +name: {{APP_NAME}} +spec: + env: + - name: DATABASE_URL + value: postgres://postgres:password@shared-postgres.cpln.local:5432/{{APP_NAME}} + - name: SECRET_KEY_BASE + value: "123" + staticPlacement: + locationLinks: + - {{APP_LOCATION_LINK}} diff --git a/spec/dummy/.controlplane/templates/app-with-deprecated-variables-no-image.yml b/spec/dummy/.controlplane/templates/app-with-deprecated-variables-no-image.yml new file mode 100644 index 00000000..c2c3b807 --- /dev/null +++ b/spec/dummy/.controlplane/templates/app-with-deprecated-variables-no-image.yml @@ -0,0 +1,16 @@ +kind: gvc +name: {{APP_NAME}} +spec: + env: + - name: ORG + value: APP_ORG + - name: NAME + value: APP_GVC + - name: LOCATION + value: APP_LOCATION + staticPlacement: + locationLinks: + - {{APP_LOCATION_LINK}} +--- +kind: identity +name: {{APP_IDENTITY}}