-
Notifications
You must be signed in to change notification settings - Fork 374
Document cpflow upstream testing #746
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| 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. |
| 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/) | ||||||
| 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}") | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
A tighter pattern that explicitly handles both quoted and unquoted forms would be safer:
Suggested change
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 | ||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Don’t silently Useful? React with 👍 / 👎. |
||||||||||
| end | ||||||||||
|
|
||||||||||
| uses_ref = uses_match[1] | ||||||||||
| refs[uses_ref] << "#{path}:#{job_name}" | ||||||||||
|
|
||||||||||
| if input_ref | ||||||||||
| refs[input_ref] << "#{path}:#{job_name}" | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When The mismatch guard on the next line already handles the error case, so this append can be dropped entirely:
Suggested change
|
||||||||||
| 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Enforce the missing-ref guard only for Line 70 currently triggers on any 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||
| 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 | ||||||||||
|
cursor[bot] marked this conversation as resolved.
|
||||||||||
| RUBY | ||||||||||
|
Comment on lines
+41
to
+86
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The guard aborts when |
||||||||||
|
|
||||||||||
| echo "==> actionlint" | ||||||||||
| actionlint -ignore "SC2129" .github/workflows/cpflow-*.yml | ||||||||||
There was a problem hiding this comment.
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._\/-]+\zallows forward slashes, so branch names likemainorfeature/my-branchpass 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: