Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .github/actions/cpflow-delete-control-plane-app/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,17 @@ inputs:
review_app_prefix:
description: Prefix used for review app names
required: true
working_directory:
description: Directory containing the downstream project's .controlplane/controlplane.yml
required: false
default: "."

runs:
using: composite
steps:
- name: Delete application
shell: bash
working-directory: ${{ inputs.working_directory }}
run: ${{ github.action_path }}/delete-app.sh
env:
APP_NAME: ${{ inputs.app_name }}
Expand Down
10 changes: 9 additions & 1 deletion .github/workflows/cpflow-delete-review-app.yml
Original file line number Diff line number Diff line change
Expand Up @@ -81,13 +81,20 @@ jobs:
variable:REVIEW_APP_PREFIX
pull_request_friendly: "true"

- name: Checkout repository
if: steps.config.outputs.ready == 'true'
uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd
with:
path: app
persist-credentials: false

- name: Setup environment
if: steps.config.outputs.ready == 'true'
uses: ./.cpflow/.github/actions/cpflow-setup-environment
with:
token: ${{ secrets.CPLN_TOKEN_STAGING }}
org: ${{ vars.CPLN_ORG_STAGING }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The working_directory was previously .cpflow (the cpflow repo checkout). Changing it to app (the downstream caller repo) means Ruby version detection now reads the downstream project's .ruby-version / .tool-versions / Gemfile — which is the correct behaviour since cpflow needs to run in the app's Ruby environment. A short comment here would make this non-obvious choice self-documenting for future maintainers.

Suggested change
org: ${{ vars.CPLN_ORG_STAGING }}
working_directory: app # app's Ruby version files, not cpflow's

working_directory: .cpflow
working_directory: app
cpln_cli_version: ${{ vars.CPLN_CLI_VERSION }}
cpflow_version: ${{ vars.CPFLOW_VERSION }}

Expand Down Expand Up @@ -130,6 +137,7 @@ jobs:
app_name: ${{ env.APP_NAME }}
cpln_org: ${{ vars.CPLN_ORG_STAGING }}
review_app_prefix: ${{ vars.REVIEW_APP_PREFIX }}
working_directory: app

# Finalizer still runs after delete failures, but only after config validation
# created the initial PR comment and workflow link env vars it updates.
Expand Down
4 changes: 3 additions & 1 deletion spec/command/generate_github_actions_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,9 @@ def shared_workflow_path(name)
contents = reusable_delete_review_workflow_path.read
expect(contents).to include("concurrency:")
expect(contents).to include('pull_request_friendly: "true"')
expect(contents).to include("working_directory: .cpflow")
expect(contents).to include("Checkout repository")
expect(contents).to include("path: app")
expect(contents).to include("working_directory: app")
expect(delete_review_workflow_path.read).to include("pull_request_target:")
expect(delete_review_workflow_path.read).to include("pull_request_target is intentional")
expect(delete_review_workflow_path.read).to include("mirrors the upstream job guard")
Expand Down
59 changes: 59 additions & 0 deletions spec/github_workflows_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
# frozen_string_literal: true

require "spec_helper"
require "yaml"

RSpec.describe "GitHub workflow definitions" do # rubocop:disable RSpec/DescribeClass
describe "Delete Review App workflow" do
let(:workflow) do
YAML.safe_load_file(
File.expand_path("../.github/workflows/cpflow-delete-review-app.yml", __dir__),
aliases: true
)
end

let(:steps) { workflow.fetch("jobs").fetch("delete-review-app").fetch("steps") }

def step_named(name)
steps.find { |step| step["name"] == name }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a step name is not found, find returns nil and the downstream expect(nil).to include(...) / nil.fetch("with") failure messages are opaque ("expected nil to include…"). Consider raising a step-not-found error immediately so the name mismatch is surfaced clearly:

Suggested change
steps.find { |step| step["name"] == name }
steps.find { |step| step["name"] == name } ||
raise("Step #{name.inspect} not found in workflow. Available: #{steps.filter_map { |s| s["name"] }.inspect}")

end

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a step is renamed or removed, step_named returns nil and the downstream .include(...) or .fetch("with") failure says expected nil to include ... — it doesn't tell you which step went missing. A small guard makes regressions easier to diagnose:

Suggested change
end
steps.find { |step| step["name"] == name } ||
raise("No workflow step named #{name.inspect}. Available: #{steps.filter_map { |s| s['name'] }.inspect}")


it "runs cpflow delete from a downstream app checkout" do
expect(step_named("Checkout repository")).to include(
"uses" => "actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd",
"with" => include(
"path" => "app",
"persist-credentials" => false
)
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec does not assert the if: guard on the checkout step. If the condition is accidentally removed or changed, the step would run even when config validation fails. Consider adding:

      expect(step_named("Checkout repository")).to include(
        "if" => "steps.config.outputs.ready == 'true'"
      )

expect(step_named("Setup environment").fetch("with")).to include(
"working_directory" => "app"
)
expect(step_named("Delete review app").fetch("with")).to include(
"working_directory" => "app"
)
end
Comment on lines +21 to +36

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Spec doesn't assert the if guard on the checkout step

The "Checkout repository" step carries if: steps.config.outputs.ready == 'true' in the workflow, which keeps it from running when config validation fails. The current spec uses include(...) so the if key is silently ignored. If that guard were accidentally dropped, the test would still pass while the step would check out into app/ even on invalid configs — causing a misleading empty working directory for subsequent steps.

Consider asserting "if" => "steps.config.outputs.ready == 'true'" alongside the other fields so the condition is pinned by the spec.

Comment on lines +21 to +36

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test validates the checkout step's uses and with values but not its if: guard. If someone removed the if: steps.config.outputs.ready == 'true' condition, this test would still pass — but the checkout would now run even when config validation failed. Consider adding:

expect(step_named("Checkout repository")).to include(
  "if" => "steps.config.outputs.ready == 'true'"
)

end

describe "Delete Control Plane App action" do
let(:action) do
YAML.safe_load_file(
File.expand_path("../.github/actions/cpflow-delete-control-plane-app/action.yml", __dir__),
aliases: true
)
end

it "allows callers to choose the project working directory" do
expect(action.fetch("inputs")).to include(
"working_directory" => include("default" => ".")
)

delete_step = action.fetch("runs").fetch("steps").find { |step| step["name"] == "Delete application" }

expect(delete_step).to include(
"working-directory" => "${{ inputs.working_directory }}"
)
end
end
end
Loading