Migrate pipeline from CircleCI to GitHub Actions#273
Conversation
phelma
left a comment
phelma
left a comment
There was a problem hiding this comment.
Code Review: #273 - Migrate pipeline from CircleCI to GitHub Actions
Verdict: COMMENT
The cutover reproduces the Variant A family plan §4 essentially verbatim: workflow YAML, Rakefile provisioning swap, gemspec/lockfile refresh, README key-management updates, and full CircleCI decommission all check out against the plan, and the §5 self-verification points re-checked here (pure GPG-key rename, YAML parse, no residual CircleCI references, correct secrets deletions) all pass. No in-scope critical or major defects were found; the one genuine diff defect is an undocumented (though rubocop-forced) library hunk. Most lens findings challenge behaviour the plan documents as deliberate parity — they are recorded as plan concerns for post-migration hardening, not blockers.
Verification Notes / False Positives
concurrency.queue: maxis valid — three lenses flagged it as an unsupported key. Verified against the current GitHub Actions workflow schema:queueis documented, andqueue: maxkeeps up to 100 pending runs in FIFO order — exactly the behaviour the workflow comment claims. These findings are disproven and excluded from severity counts.lib/ruby_terraform/options/types.rbchange is rubocop-forced — verified by running the refreshed lockfile's rubocop (Style/ArgumentsForwarding) against the pre-PR file: 2 autocorrectable offences. The plan's §5.1 gate (./go library:checkmust pass) requires it. Residual defect is documentation only (see inline comment).
Cross-Cutting Themes
- Unreviewed-code-to-publish-credentials path (flagged by: security, correctness, safety) — dependabot auto-merge (no update-type filter, no human review) feeds pushes to
main, whose prerelease job unlocks git-crypt and writes RubyGems credentials before running./goover the freshly-merged dependency tree. Each link is plan-documented (D3, ungated prerelease parity), but the composed exfiltration path is worth a post-migration hardening issue. - Publish-before-push wedge (flagged by: correctness, safety) — a raced human push to main can fail the bump-commit push after the gem has published, leaving RubyGems ahead of the repo and wedging subsequent prereleases on duplicate versions. The ordering is the plan's documented D5 parity hazard; the wedge consequence deserves a runbook note.
- Approval-time release drift (flagged by: security, safety) — the release job pulls
mainat approval time, so the gate attests to a run, not the published artefact. Plan-documented parity with the oldrelease.sh.
Strengths
- ✅ Injection-safe skip-ci check: the commit message is passed via an env var, never interpolated into the script.
- ✅ Least-privilege permissions: workflow-level
contents: readwith per-job elevation only where needed. - ✅ Decommission removes long-lived stored credentials (CircleCI API token, GitHub PAT, CI SSH deploy key pair) and replaces ~150 lines of bespoke CI shell with thin task-triggering workflows.
- ✅ Non-obvious decisions are commented at the point of use (concurrency queueing, release-time pull, Slack failure routing), and plan-mandated ordering/naming conventions are followed exactly.
- ✅ Full releases stay gated behind the
releaseenvironment with team reviewers; Slack steps arecontinue-on-errorso notification outages cannot block the pipeline.
General Findings
- 🔵 Correctness:
slack:notify[${{ job.status }}]can receivecancelled, which falls through to the failure catch-all — cancelled runs post as failures. Plan-snippet behaviour (plan concern); consider an explicit cancelled rule post-migration. - 🔵 Correctness: Dependabot-triggered
pull_requestruns use the Dependabot secret store, soSLACK_BOT_TOKENis empty until issue 19 lands — plan-documented and accepted. - 🔵 Correctness:
skip-ci-checkduplicates GitHub's native[skip ci]handling — plan-documented belt-and-braces (D4). - 🔵 Safety: no in-repo recovery runbook for the
ENCRYPTION_PASSPHRASE→gpg.private.enc→ git-crypt → RubyGems credential chain; worth adding in the end-of-migration sweep. - 🔵 Security: mutable
@v1/@v4action tags in credential-bearing jobs — plan-documented (D6); SHA-pinning is a post-migration option. - 🔵 Code Quality: duplicated RubyGems-credential shell across prerelease/release, and raw Slack channel IDs — both are the plan's exact snippets (plan concerns).
- 🔵 Standards: catch-all routing rule's trailing
# buildscomment uses one space where the plan snippet and sibling rules use two — cosmetic drift.
Review generated by /accelerator:review-pr
| def self.standard(name, value, **) | ||
| Standard.new(name, value, **) |
There was a problem hiding this comment.
🔵 Standards + Code Quality (minor, high confidence)
The plan (§4.8) leaves all library code alone, yet this hunk rewrites **opts to anonymous ** forwarding. Verified: the refreshed lockfile's rubocop flags the old code under Style/ArgumentsForwarding (2 autocorrectable offences), and the plan's §5.1 gate requires ./go library:check to pass — so the change is justified. It is, however, mentioned nowhere in the PR description or commit message; please add it to the deliberate-decisions list so reviewers/bisectors can trace it.
| - name: Prerelease | ||
| run: ./go "version:bump[pre]" && ./go release | ||
| - name: Push release commit | ||
| run: git push && git push --tags |
There was a problem hiding this comment.
🔵 Correctness + Safety (plan concern)
If a human push lands on main between checkout and this push, the push fails non-fast-forward after the gem published; the next version:bump[pre] recomputes the same version and RubyGems rejects it, wedging the pipeline until a manual bump. The ordering is the plan's documented D5 parity hazard — recording the wedge consequence for the post-migration hardening list / runbook.
| env: | ||
| SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} | ||
|
|
||
| prerelease: |
There was a problem hiding this comment.
🔵 Security (plan concern)
Dependabot PRs auto-merge unreviewed, and every main push runs this job, which unlocks git-crypt and writes RubyGems credentials before ./go loads the just-merged dependency tree. Each link is plan-documented (D3, ungated-prerelease parity), but the composed path — malicious dev-dependency release → credential exfiltration → trojaned gem — deserves its own post-migration issue (update-type filter, scoped/MFA RubyGems key, or later credential injection).
| merge-pull-request: | ||
| needs: [check, test, build] | ||
| runs-on: ubuntu-latest | ||
| if: github.actor == 'dependabot[bot]' |
There was a problem hiding this comment.
🔵 Security (plan concern)
github.actor is whoever triggered the latest event, not whose commits merge. This is the plan's exact YAML (D3), so noting for post-migration hardening: github.event.pull_request.user.login == 'dependabot[bot]' (or dependabot/fetch-metadata) verifies the property that actually matters.
| permissions: | ||
| contents: write | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔵 Correctness (plan concern, low confidence)
actions/checkout@v4 defaults to fetch-depth: 1 with no tags; the old CircleCI checkout was a full clone. If gem-release consults existing tags/history when bumping/tagging, behaviour could differ. The plan's YAML uses the default, so this is a verify-on-first-run item rather than a diff defect — worth watching on the first main build.
| } | ||
| # Operator's ambient auth — the stored PAT is gone (see the cutover PR's | ||
| # deliberate-decisions list). | ||
| t.access_token = ENV.fetch('GITHUB_TOKEN') { `gh auth token`.strip } |
There was a problem hiding this comment.
🔵 Code Quality (plan concern)
If gh is missing or unauthenticated, `gh auth token`.strip yields an empty string and provisioning fails later with an opaque 401 instead of a clear message. This is the plan's exact D7 snippet — flagging for a fleet-wide follow-up (fail fast when the resolved token is empty) rather than as a defect of this diff.
| ```bash | ||
| openssl aes-256-cbc \ | ||
| -e \ | ||
| -md sha1 \ |
There was a problem hiding this comment.
🔵 Security (plan concern)
openssl aes-256-cbc -md sha1 without -pbkdf2/-iter is single-iteration EVP_BytesToKey; the encrypted CI GPG key sits in a public repo, so its safety rests entirely on passphrase strength against fast offline brute force. Pre-existing, and the plan says to leave the openssl commands untouched — recording for fleet-wide post-migration hardening.
There was a problem hiding this comment.
Pull request overview
This PR completes the repo’s CI migration by removing the legacy CircleCI pipeline/scripts and introducing equivalent GitHub Actions workflows, along with updated Rake-based provisioning for GitHub secrets/environments and Slack notifications.
Changes:
- Add GitHub Actions workflows for PR and
main(check/test/build, prerelease, release environment gate, dependabot merge). - Replace CircleCI-era Rake provisioning (CircleCI project, deploy keys/SSH) with GitHub secrets/environments + Slack notification tasks.
- Remove CircleCI configuration and CI helper scripts; update docs and dependencies to match the new pipeline.
Reviewed changes
Copilot reviewed 20 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| .github/workflows/pr.yaml | New PR workflow (check/test/build + dependabot auto-merge). |
| .github/workflows/main.yaml | New main workflow (skip-ci guard, prerelease + gated release). |
| .github/gpg.private.enc | Adds encrypted GPG key blob for CI unlock flow. |
| .circleci/config.yml | Removes CircleCI pipeline configuration. |
| scripts/ci/steps/test.sh | Removes CircleCI test step script. |
| scripts/ci/steps/build.sh | Removes CircleCI build step script. |
| scripts/ci/steps/prerelease.sh | Removes CircleCI prerelease script. |
| scripts/ci/steps/release.sh | Removes CircleCI release script. |
| scripts/ci/steps/merge-pull-request.sh | Removes CircleCI dependabot merge script. |
| scripts/ci/common/install-slack-deps.sh | Removes CircleCI runner setup for Slack deps. |
| scripts/ci/common/install-gpg-key.sh | Removes CircleCI-specific GPG key install script. |
| scripts/ci/common/install-git-crypt.sh | Removes CircleCI secrets tooling install script. |
| scripts/ci/common/install-asdf.sh | Removes CircleCI asdf install script. |
| scripts/ci/common/install-asdf-dependencies.sh | Removes CircleCI asdf dependency install script. |
| scripts/ci/common/configure-rubygems.sh | Removes CircleCI RubyGems credential config script. |
| scripts/ci/common/configure-git.sh | Removes CircleCI git identity config script. |
| scripts/ci/common/configure-asdf.sh | Removes CircleCI asdf plugin config script. |
| Rakefile | Updates provisioning to GitHub secrets/environments; adds Slack notifications + CI git author task; adds library:build. |
| ruby_terraform.gemspec | Drops CircleCI/SSH rake deps; adds rake_slack. |
| Gemfile.lock | Dependency resolution updates to match the new gem set. |
| README.md | Updates CI key management docs for GitHub Actions location. |
| lib/ruby_terraform/options/types.rb | Refactors kwarg handling for Types.standard. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def self.standard(name, value, **) | ||
| Standard.new(name, value, **) |
| concurrency: | ||
| group: main | ||
| cancel-in-progress: false | ||
| queue: max |
| } | ||
| # Operator's ambient auth — the stored PAT is gone (see the cutover PR's | ||
| # deliberate-decisions list). | ||
| t.access_token = ENV.fetch('GITHUB_TOKEN') { `gh auth token`.strip } |
Cutover to GitHub Actions per the Variant A family plan (gem pilot).
Includes decommission — merging this PR completes the repo's migration.
releaseenvironment gate.github/rake_slack; dependabot auto-merge jobrake_githubsecrets/environments;rake_circle_cidropped.circleci/,scripts/ci/, the CI SSH deploykey pair and its
keys:deploy/deploy_keysprovisioning, and the storedCircleCI/GitHub API credentials (
config/secrets/{circle_ci,github}/)Deliberate decisions (not defects)
This cutover reproduces the CircleCI pipeline's behaviour, warts included;
fixing inherited hazards is post-migration work. In particular:
./go releasepublishes to RubyGems before the version-bump commit ispushed — pre-existing ordering inside the untouched release logic.
mainwith no approval gate; onlyfull releases are gated (
environment: release).merge does not trigger a release build — on CircleCI the merge commit
carried
[skip ci], so this matches. Updates ship with the nexthuman-triggered release.
releasejob pullsmainat approval time, so a delayed approvalpublishes main as it stands then, not the SHA this run tested — parity with
the old
release.sh(which also pulled;prerelease.shdid not, so theprerelease job has no pull).
asdf_install@v1is our own action (infrablocks/github-actions); we arehappy tracking its major version tag.
build system (
./go/rake) and CI stays lean — it just triggers tasks andsupplies secrets/context.
Gemfile.lockcarries transitive major bumps — the unavoidable resolutionof the targeted
bundle lock --update, not scope creep.autocorrects existing code (e.g.
Style/ArgumentsForwarding) — requiredby the
library:checkverification gate, not drive-by refactoring.pipeline:prepare) authenticates with the operator's ambientghlogin (GITHUB_TOKENfallback) instead of a stored PAT — a deliberateparity deviation; the stored token in
config/secrets/github/config.yamlis deleted with the rest of the CircleCI-era credentials.
Do not merge manually — the pipeline merges once checks are green.
Disabling the CircleCI project and deleting the
CircleCIdeploy key aredeferred to the end-of-migration sweep.