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
78 changes: 77 additions & 1 deletion .controlplane/docs/testing-cpflow-github-actions.md
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,73 @@ workflow change was tested before a new `cpflow` gem release existed. That is
safer than pinning a branch, but it should be treated as a temporary test pin
until the next upstream release tag is available.

What is tied to the upstream repo:

- The downstream workflow wrapper hardcodes `shakacode/control-plane-flow` in
`uses:`.
- The reusable workflow file comes from the ref after `@`.
- The reusable workflow checks out `control-plane-flow` at
`control_plane_flow_ref` to load shared composite actions.
- When `CPFLOW_VERSION` is empty, the setup action builds and installs the
`cpflow` gem from that checked-out repository ref.

What is tied to RubyGems:

- A released `cpflow` gem is the normal source used to generate downstream
workflow wrappers.
- The `CPFLOW_VERSION` repository variable is a runtime override that runs
`gem install cpflow -v <version>` inside workflows.

For stable downstream automation, prefer the release path: generate from a
released gem and pin wrappers to the matching upstream release tag. For
pre-release validation, pin to a full commit SHA from the upstream PR, never a
moving branch.

## Testing An Unmerged Upstream PR Downstream

You can test an upstream `control-plane-flow` PR in this downstream app before
merging upstream, without publishing a gem. Use an immutable commit SHA from the
upstream PR branch:

1. Push the upstream PR branch and copy its head commit SHA.
2. In a downstream test branch, pin every generated wrapper ref:

```sh
bin/pin-cpflow-github-ref <upstream-pr-sha>
```

The helper accepts release tags and full 40-character commit SHAs by default.
It rejects branch names such as `main` or `feature/foo`; use
`--allow-moving-ref` only for short-lived local experiments that will not be
committed. The resulting diff should replace both pins in each reusable
workflow call:

```yaml
uses: shakacode/control-plane-flow/.github/workflows/cpflow-deploy-review-app.yml@<upstream-pr-sha>
with:
control_plane_flow_ref: <upstream-pr-sha>
```

3. Keep `CPFLOW_VERSION` unset unless you intentionally want to test a released
RubyGems version instead of building `cpflow` from the upstream PR SHA.
4. Run `bin/test-cpflow-github-flow`.
5. Open a downstream PR and trigger a real review app with a comment whose body
is exactly:

```text
+review-app-deploy
```

6. Verify the deploy logs show the expected upstream commit SHA, the setup step
prints the expected `cpflow` version/source, and the review app URL returns
HTTP 200.
7. After the upstream PR merges and releases, regenerate or repin downstream to
the release tag instead of leaving the temporary commit SHA forever.

This tests the real reusable workflow and shared composite actions from the
upstream PR. It avoids merging upstream blind while also avoiding a mutable
branch ref in downstream automation.

## Local Checks

After regenerating the flow, run these checks from the repository root. If
Expand All @@ -83,7 +150,10 @@ inside composite action metadata, including `description:` fields. Literal
examples such as `${{ vars.SOME_VALUE }}` can fail action loading before any
shell step starts. The wrapper runs `cpflow github-flow-readiness`, parses the
generated YAML, checks action input descriptions for literal GitHub expressions,
and runs `actionlint -ignore 'SC2129' .github/workflows/cpflow-*.yml`.
checks that every generated wrapper keeps `uses:` and `control_plane_flow_ref`
on the same upstream ref across all `cpflow-*` wrappers, checks that any
secret-inheriting reusable workflow passes `control_plane_flow_ref`, and runs
`actionlint -ignore 'SC2129' .github/workflows/cpflow-*.yml`.

## PR Checks

Expand Down Expand Up @@ -168,6 +238,12 @@ Create the first review app by commenting exactly:

## Ways To Make This Easier

- Extend `bin/pin-cpflow-github-ref` so it can also run
`bin/test-cpflow-github-flow`, open a downstream PR, and print or post the
exact `+review-app-deploy` command needed to start the canary deploy.
- Add CI coverage that runs `bin/test-cpflow-github-flow` on generated workflow
changes, so ref mismatches and action metadata parsing issues are caught
before review.
- Add a no-secret GitHub Actions smoke workflow that loads generated local
composite actions from the PR branch and fails fast on action metadata parsing.
- Extend `bin/test-cpflow-github-flow` as more local cpflow GitHub Actions
Expand Down
10 changes: 10 additions & 0 deletions .controlplane/readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -437,6 +437,16 @@ setup action installs that RubyGems version instead. For normal release pins,
either leave it unset while using the matching `v<version>` workflow tag, or set
it to the same gem version without the leading `v`.

To test unreleased upstream workflow changes before merging `control-plane-flow`,
pin a downstream PR to the upstream PR's full commit SHA in both `uses:` and
`control_plane_flow_ref`, leave `CPFLOW_VERSION` unset, and trigger a real review
app deploy. That tests the upstream reusable workflow, shared composite actions,
and source-built `cpflow` gem from the same immutable commit. After the upstream
change is released, regenerate or repin back to the matching release tag.
Use `bin/pin-cpflow-github-ref <ref>` to update the generated wrapper refs
together. The helper accepts release tags and full commit SHAs by default;
`--allow-moving-ref` is only for short-lived local experiments.

For this app, validate a regenerated flow with:

```bash
Expand Down
103 changes: 26 additions & 77 deletions .github/testing-github-actions.md
Original file line number Diff line number Diff line change
@@ -1,85 +1,34 @@
# Developing and Testing Github Actions
# Developing and Testing GitHub Actions

Testing Github Actions on an existing repository is tricky.

The main issue boils down to the fact that Github Actions uses the workflow files in the branch where the event originates. This is fine for push events, but it becomes a problem when you want to test workflows that are triggered by comments on a pull request.
GitHub Actions workflow testing depends on the event type:

Here's a summary of the behavior:

Behavior of push and pull_request Events
1. Push on a Branch:
• When you push changes to a branch (e.g., feature-branch), GitHub Actions uses the workflow files in that same branch.
• This is why changes to workflows work seamlessly when testing with push events.
2. Pull Request Events:
• For pull_request events (e.g., a PR from feature-branch into master), GitHub Actions will always use the workflow files from the target branch (e.g., master), not the source branch (e.g., feature-branch).
• This is a security feature to prevent someone from introducing malicious code in a PR that modifies the workflow files themselves.
- `push` runs workflow files from the pushed branch.
- `pull_request` runs workflow files from the base branch for sensitive cases.
- `issue_comment` runs workflow files from the default branch.
- `workflow_dispatch --ref <branch>` runs the workflow file from that ref.

Impact on Comment-Triggered Workflows
This matters for review-app automation because comment-triggered commands such
as `+review-app-deploy` use default-branch workflow code. A PR that changes
workflow files or action wiring is not fully proven by commenting on that same
PR until the trusted default-branch code has those changes.

When you want to trigger workflows via comments (issue_comment) in a pull request:
• The workflow code used will always come from the master branch (or the default branch), regardless of the branch where the PR originates.
• This means the PR’s changes to the workflow won’t be used, and the action invoked by the comment will also use code from master.
For cpflow review-app, staging, and promotion workflows, use the cpflow-specific
guide:

Workarounds to Test Comment-Triggered Workflows
[Testing cpflow GitHub Actions Changes](../.controlplane/docs/testing-cpflow-github-actions.md)

If you want to test workflows in a way that uses the changes in the pull request, here are your options:
## Practical Pattern

1. Use Push Events for Testing
• Test your changes on a branch with push triggers.
• Use workflow_dispatch to simulate the events you need (like invoking actions via comments).
1. Validate generated files locally with `bin/test-cpflow-github-flow`.
2. Open a PR and let regular CI prove GitHub can parse the workflow YAML.
3. For top-level workflow-file experiments, run `workflow_dispatch --ref`.
4. For comment-triggered review-app commands, test a real `+review-app-deploy`
after the trusted default-branch wrapper points at the code under test.
5. When testing unreleased upstream `control-plane-flow` changes downstream, pin
both the reusable workflow `uses:` ref and `control_plane_flow_ref` to the
same upstream commit SHA.

This allows you to confirm that your changes to the workflow file or actions behave as expected before merging into master.

2. Merge the Workflow to master Temporarily

If you absolutely need the workflow to run as part of a pull_request event:
1. Merge your workflow changes into master temporarily.
2. Open a PR to test your comment-triggered workflows.
3. Revert the changes in master if necessary.

This ensures the workflow changes are active in master while still testing with the pull_request context.

3. Add Logic to Detect the Source Branch

Use github.event.pull_request.head.ref to add custom logic in your workflow that behaves differently based on the source branch.
• Example:

jobs:
test-pr:
runs-on: ubuntu-latest
if: ${{ github.event.pull_request.head.ref == 'feature-branch' }}
steps:
- name: Checkout Code
uses: actions/checkout@v3

- name: Debug
run: echo "Testing workflow changes in feature-branch"

However, this still requires the workflow itself to exist in master.

4. Use a Fork or a Temporary Repo

Create a temporary repository or a fork to test workflows in isolation:
• Push your workflow changes to master in the test repository.
• Open a PR in the fork to test how workflows behave with issue_comment events and PR contexts.

Once confirmed, you can replicate the changes in your main repository.

6. Alternative Approach: Split Workflows

If your workflow includes comment-based triggers (issue_comment), consider splitting your workflows:
• A base workflow in master that handles triggering.
• A test-specific workflow for validating changes on a branch.

For example:
1. The base workflow triggers when a comment like /run-tests is added.
2. The test-specific workflow runs in response to the base workflow but uses the branch’s code.

Summary
• For push events: The branch-specific workflow is used, so testing changes is easy.
• For pull_request and issue_comment events: GitHub always uses workflows from the master branch, and there’s no direct way to bypass this.

To test comment-triggered workflows:
1. Use push or workflow_dispatch to validate changes.
2. Merge workflow changes temporarily into master to test with pull_request events.
3. Use tools like act for local simulation.
Avoid testing production automation against moving branch refs such as `main` or
a feature branch. Use release tags for normal operation and full commit SHAs for
temporary pre-release validation. `bin/pin-cpflow-github-ref` enforces that
default and requires `--allow-moving-ref` for one-off local branch experiments.
72 changes: 72 additions & 0 deletions bin/pin-cpflow-github-ref
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
#!/usr/bin/env ruby
# frozen_string_literal: true

require "pathname"

USAGE = <<~USAGE
Usage: bin/pin-cpflow-github-ref [--allow-moving-ref] <control-plane-flow-ref>

Use a release tag for normal operation, e.g. v5.0.0.
Use a full 40-character commit SHA for temporary unreleased upstream testing.
Use --allow-moving-ref only for short-lived local branch/ref experiments.
USAGE

ALLOWED_OPTIONS = ["--allow-moving-ref"].freeze
FULL_COMMIT_SHA = /\A[0-9a-f]{40}\z/i
RELEASE_TAG = /\Av\d+\.\d+\.\d+(?:[-.][0-9A-Za-z][0-9A-Za-z.-]*)?\z/

options, positional = ARGV.partition { |arg| arg.start_with?("--") }
unknown_options = options - ALLOWED_OPTIONS

unless unknown_options.empty?
warn "Unknown option(s): #{unknown_options.join(', ')}"
warn USAGE
exit 1
end

unless positional.length == 1
warn USAGE
exit 1
end

ref = positional.first
allow_moving_ref = options.include?("--allow-moving-ref")

unless ref.match?(/\A[0-9A-Za-z._\/-]+\z/)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The validation regex \A[0-9A-Za-z._\/-]+\z allows forward slashes, so branch names like main or feature/my-branch pass silently. The docs explicitly warn against mutable branch refs, but the script doesn't enforce it.

Consider adding a soft warning (or hard failure) when the ref is neither a full 40-char SHA nor a v-prefixed semver tag:

Suggested change
unless ref.match?(/\A[0-9A-Za-z._\/-]+\z/)
unless ref.match?(/\A[0-9A-Za-z._\/-]+\z/)
warn "Ref contains unsupported characters: #{ref.inspect}"
exit 1
end
if ref.include?("/") || (ref !~ /\A[0-9a-f]{40}\z/ && ref !~ /\Av\d/)
warn "Warning: #{ref.inspect} looks like a branch name — use a full commit SHA or release tag instead."
end

warn "Ref contains unsupported characters: #{ref.inspect}"
exit 1
end

unless ref.match?(FULL_COMMIT_SHA) || ref.match?(RELEASE_TAG) || allow_moving_ref
warn "Ref must be a full 40-character commit SHA or a v-prefixed release tag: #{ref.inspect}"
warn "Use --allow-moving-ref only for a short-lived branch/ref experiment."
exit 1
end

root = Pathname.new(__dir__).join("..").expand_path
workflow_paths = Dir[root.join(".github/workflows/cpflow-*.yml")].sort

if workflow_paths.empty?
warn "No cpflow workflow wrappers found."
exit 1
end

changed = []
workflow_paths.each do |path|
text = File.read(path)
updated = text
.gsub(%r{(uses:\s+shakacode/control-plane-flow/\.github/workflows/[^@\s]+@)[^\s]+}, "\\1#{ref}")
.gsub(/(\bcontrol_plane_flow_ref:\s*)\S+/, "\\1#{ref}")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

\S+ matches the entire non-whitespace token, including surrounding YAML quotes if the value was written as control_plane_flow_ref: "3e0e7e1f...". The replacement would then emit a bare unquoted ref, silently changing the YAML style.

A tighter pattern that explicitly handles both quoted and unquoted forms would be safer:

Suggested change
.gsub(/(\bcontrol_plane_flow_ref:\s*)\S+/, "\\1#{ref}")
.gsub(/(\bcontrol_plane_flow_ref:\s*)['"]?[0-9A-Za-z._\/-]+['"]?/, "\\1#{ref}")

The generated workflow files currently use unquoted values so this is not an active bug, but it is a latent risk if the generator ever changes style.


next if updated == text

File.write(path, updated)
changed << Pathname.new(path).relative_path_from(root).to_s
end

puts "Pinned cpflow GitHub ref to #{ref}"
if changed.empty?
puts "No files changed."
else
changed.each { |path| puts "updated #{path}" }
end
47 changes: 47 additions & 0 deletions bin/test-cpflow-github-flow
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,52 @@ abort bad.join("\n") unless bad.empty?
puts "no action metadata descriptions contain GitHub expressions"
RUBY

echo "==> check cpflow reusable workflow refs"
bin/conductor-exec ruby <<'RUBY'
require "yaml"

CONTROL_PLANE_FLOW_WORKFLOW = %r{\Ashakacode/control-plane-flow/\.github/workflows/[^@\s]+@([^\s]+)\z}

refs = Hash.new { |hash, key| hash[key] = [] }

Dir[".github/workflows/cpflow-*.yml"].sort.each do |path|
doc = YAML.load_file(path, aliases: true)
doc.fetch("jobs", {}).each do |job_name, job|
next unless job.is_a?(Hash)

with = job["with"].is_a?(Hash) ? job["with"] : {}
input_ref = with["control_plane_flow_ref"]
uses_match = job["uses"].to_s.match(CONTROL_PLANE_FLOW_WORKFLOW)

unless uses_match
abort "#{path}:#{job_name} has control_plane_flow_ref but no control-plane-flow reusable workflow" if input_ref

next
Comment on lines +58 to +61
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 Badge Fail when wrapper job lacks a parseable cpflow uses ref

Don’t silently next when uses_match is missing and control_plane_flow_ref is absent, because this lets a broken cpflow-*.yml wrapper pass the new guard entirely. If a reusable-workflow call is edited away from shakacode/control-plane-flow/...@ref (or otherwise becomes unparsable) in a job that doesn’t carry control_plane_flow_ref (for example wrappers without secrets: inherit), this check exits successfully and no ref-consistency error is reported.

Useful? React with 👍 / 👎.

end

uses_ref = uses_match[1]
refs[uses_ref] << "#{path}:#{job_name}"

if input_ref
refs[input_ref] << "#{path}:#{job_name}"
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

When uses_ref == input_ref (the valid case), refs[uses_ref] already received this path:job_name on line 65. Appending it again here means every valid job appears twice under the same key. paths.uniq in the display compensates, but the double-entry is confusing and could skew refs.length in edge cases.

The mismatch guard on the next line already handles the error case, so this append can be dropped entirely:

Suggested change
refs[input_ref] << "#{path}:#{job_name}"
abort "#{path}:#{job_name} mismatched cpflow refs: #{uses_ref}, #{input_ref}" if uses_ref != input_ref

abort "#{path}:#{job_name} mismatched cpflow refs: #{uses_ref}, #{input_ref}" if uses_ref != input_ref
elsif job.key?("secrets")
abort "#{path}:#{job_name} inherits secrets but is missing control_plane_flow_ref for #{uses_ref}"
Comment on lines +70 to +71
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Enforce the missing-ref guard only for secrets: inherit

Line 70 currently triggers on any secrets key, not specifically inherited secrets. That can cause false failures for wrappers that pass explicit secrets mappings.

Suggested fix
-    elsif job.key?("secrets")
+    elsif job["secrets"] == "inherit"
       abort "#{path}:#{job_name} inherits secrets but is missing control_plane_flow_ref for #{uses_ref}"
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
elsif job.key?("secrets")
abort "#{path}:#{job_name} inherits secrets but is missing control_plane_flow_ref for #{uses_ref}"
elsif job["secrets"] == "inherit"
abort "#{path}:#{job_name} inherits secrets but is missing control_plane_flow_ref for #{uses_ref}"
🤖 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 `@bin/test-cpflow-github-flow` around lines 70 - 71, The guard currently fires
for any presence of the secrets key; change it to only abort when secrets is
explicitly set to inherit by replacing the condition that references
job.key?("secrets") with a check that secrets equals "inherit" (e.g.,
job["secrets"] == "inherit" or equivalent), leaving the abort message that
mentions control_plane_flow_ref and uses_ref intact so only jobs with secrets:
inherit trigger the missing-ref error.

end
end
end

if refs.empty?
puts "no upstream cpflow reusable workflow refs found"
elsif refs.length > 1
refs.each do |ref, paths|
puts "#{ref}: #{paths.uniq.sort.join(', ')}"
end
abort "cpflow workflow wrappers use multiple upstream refs: #{refs.keys.sort.join(', ')}"
else
puts "cpflow refs: #{refs.keys.sort.join(', ')}"
end
Comment thread
cursor[bot] marked this conversation as resolved.
RUBY
Comment on lines +41 to +86
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Cross-file ref inconsistency not detected

The guard aborts when uses: and control_plane_flow_ref disagree within a single file, but if file_a.yml pins sha_a and file_b.yml pins sha_b, the loop passes both without aborting — the only evidence is the printed cpflow refs: sha_a, sha_b output. Any CI job that doesn't inspect stdout carefully would pass silently with mismatched refs across the generated wrappers, which is the exact split-ref scenario the guard is meant to prevent.


echo "==> actionlint"
actionlint -ignore "SC2129" .github/workflows/cpflow-*.yml
Loading