Migrate pipeline from CircleCI to GitHub Actions#268
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the project’s CI/CD pipeline from CircleCI to GitHub Actions, updating supporting rake tasks/secrets provisioning and adding Slack-based notifications needed for the new workflows.
Changes:
- Adds GitHub Actions workflows for PR and main branch pipelines (including prerelease + environment-gated release).
- Replaces CircleCI provisioning tasks with GitHub secrets/environments provisioning and introduces Slack notification tasks via
rake_slack. - Moves the encrypted CI GPG key into
.github/and updates documentation accordingly.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
ruby_terraform.gemspec |
Removes rake_circle_ci and adds rake_slack as a dev dependency. |
README.md |
Updates encrypted GPG key paths from .circleci/ to .github/. |
Rakefile |
Drops CircleCI task definitions; adds GitHub secrets/environments provisioning, Slack notification tasks, and a library:build task. |
Gemfile.lock |
Updates dependency graph (including adding rake_slack and removing rake_circle_ci, plus additional version upgrades). |
.github/workflows/pr.yaml |
Introduces PR workflow with check/test/build and Dependabot auto-merge job. |
.github/workflows/main.yaml |
Introduces main workflow with skip-ci guard, check/test, prerelease, and environment-gated release. |
.github/gpg.private.enc |
Adds encrypted private key material under .github/ for runner unlock flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as outdated.
This comment was marked as outdated.
phelma
left a comment
There was a problem hiding this comment.
Code Review: #268 - Migrate pipeline from CircleCI to GitHub Actions
Verdict: COMMENT
A disciplined, plan-conformant cutover: both workflows reproduce the Variant A family plan's authoritative YAML verbatim, the Rakefile/gemspec/lockfile changes match the prescribed snippets (rake_git_crypt 0.4.0, rake_github 0.17.0, rake_slack 0.3.0), the GPG key move is a pure rename, and decommission is complete with no residual CircleCI references outside docs/. Every substantive finding below challenges behaviour the family plan itself specifies — they are plan concerns for the family maintainers to weigh, not defects of this diff.
Cross-Cutting Themes
- Release-path commit drift (flagged by: correctness, safety) — the
git pullsteps inprereleaseandreleasemean the published artefact can include commits this run never checked/tested, and thereleaseenvironment approval effectively approves "main at approval time" rather than a specific SHA. Combined with themainconcurrency group, a long-pending release approval also queues (and GitHub cancels intermediate) subsequent main runs. All of this is in the plan's exact YAML — worth a family-plan revisit, not a change here. - Plan-propagated details (flagged by: standards, code-quality) —
ci@infrablocks.com(vs theinfrablocks.iodomain used elsewhere) and the--globalscope ofrepository:set_ci_authorcome verbatim from the plan/rake_slack template; if either is wrong, fix it at the plan so the fleet inherits the correction.
Tradeoff Analysis
- Parity vs hardening: security suggests SHA-pinning third-party actions, a stronger KDF for the committed encrypted GPG key,
gh pr merge --auto, and gating auto-merge on PR author rather thangithub.actor. The plan's parity principle deliberately defers hardening to post-migration fleet work — this review keeps those visible without blocking the diff.
Strengths
- ✅ Workflows match the plan's authoritative YAML exactly: top-level
permissions: contents: readwith per-job elevation,concurrencyserialising publishes,continue-on-errornotify steps, and the deliberate plaingh pr merge --merge. - ✅ Script-injection-safe commit-message handling (env var, not inline interpolation) in the skip-ci guard.
- ✅ PR workflow never unlocks git-crypt, so untrusted PR/dependabot runs cannot reach
ENCRYPTION_PASSPHRASEor RubyGems credentials — a genuine trust-boundary improvement over CircleCI. - ✅ Complete decommission: CircleCI config, all nine
scripts/ciwrappers, the SSH deploy key pair and its provisioning, and therake_circle_ci/rake_sshdependencies removed together;git grep -i circleci(excl. docs/lockfile) is clean. - ✅ Rakefile changes match the plan snippets verbatim with the mandated rationale comments; require and dev-dependency lists stay alphabetically ordered; commit message follows org convention.
- ✅ Full releases gated behind the
releaseenvironment withmaintainersreviewers, provisioned viarake_github.
General Findings
All of the following are plan concerns or post-migration follow-ups — none blocks this diff:
- 🔵 Correctness: the
skip-ci-checkjob guards a path GitHub Actions already handles natively ([skip ci]etc. suppress push workflows;GITHUB_TOKENpushes never trigger). Deliberate belt-and-braces per D4; candidate for post-migration cleanup. - 🔵 Security: the release path is a single-secret chain (passphrase → GPG key → git-crypt → long-lived RubyGems key). RubyGems Trusted Publishing (OIDC) would remove the long-lived credential — worth a family follow-up issue.
- 🔵 Safety: the documented publish-before-push hazard has no written recovery runbook (published-but-unpushed version after a partial prerelease failure). Cheap post-migration doc win.
- 🔵 Code Quality: Slack channel IDs are stamped as magic constants into every migrated repo's Rakefile; a channel change becomes a fleet-wide edit. Post-migration hardening candidate.
- 🔵 Standards: workflows use
.yamlwhiledependabot.ymluses.yml— plan-mandated and matches siblings; noted only so nobody "fixes" it.
False positive (verified, not counted): a correctness finding claimed dependabot-triggered runs cannot read SLACK_BOT_TOKEN; plan D2 records the token as provisioned org-wide in both the Actions and Dependabot stores, so builds-dependabot routing works.
Additional Findings
- 🔵
.github/workflows/main.yaml:88— Inline RubyGems credential shell duplicated across prerelease/release jobs; post-migration, fold into a rake task or shared action (code-quality, plan concern; deferred to keep within the inline-comment cap)
Review generated by /review-pr
| concurrency: | ||
| group: main | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
🟡 Correctness (plan concern — this is the plan's exact YAML)
A run waiting on the environment: release approval still occupies the main concurrency group. With cancel-in-progress: false, subsequent pushes to main queue behind it — and GitHub keeps only the latest pending run in a concurrency group, cancelling intermediate ones. While a release approval sits unactioned, intermediate merges to main get no prerelease at all, a behaviour change from CircleCI where the hold job did not block other pipelines.
Impact: An unapproved/forgotten release hold stalls all prerelease publishing; intermediate merges lose their prerelease runs permanently.
Suggestion: Either make prompt approve/reject of release holds an operational rule, or scope concurrency to the publishing jobs (job-level concurrency on prerelease/release). A family-plan decision, not a change for this PR.
| - name: Pull latest main | ||
| run: git pull |
There was a problem hiding this comment.
🟡 Correctness (plan concern — this is the plan's exact YAML)
The prerelease job pulls the latest main before releasing, but check/test in this run validated only github.sha. If a new commit lands on main mid-run, git pull folds it into the published gem untested — and if that push's own run was cancelled as an intermediate pending run in the concurrency group, its changes ship only via this untested path. (The old CircleCI prerelease.sh did not git pull; only release.sh did.)
Suggestion: At the plan level, consider dropping the pull from prerelease (the concurrency group already prevents non-fast-forward pushes) or failing if git rev-parse HEAD != github.sha.
| - uses: actions/checkout@v4 | ||
| with: | ||
| ref: main | ||
| - name: Pull latest main | ||
| run: git pull |
There was a problem hiding this comment.
🔵 Safety (plan concern)
The release job checks out ref: main then runs git pull, so an approval granted hours/days later publishes whatever main is at approval time — possibly including commits not validated by this run. This mirrors the old CircleCI release.sh, but it is not in the PR description's deliberate-decisions list.
Suggestion: Add it to the documented inherited hazards; post-migration, consider releasing the tested SHA.
| continue-on-error: true | ||
| run: ./go "slack:notify[${{ job.status }}]" | ||
| env: | ||
| SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }} |
There was a problem hiding this comment.
🔵 Security (plan concern — notify steps are plan-specified)
The check/test/build jobs run ./go (Rakefile/Gemfile code from the PR head) with SLACK_BOT_TOKEN in the notify step's environment. Fork PRs get no secrets, so exposure is limited to same-repo branches and dependabot — but any branch PR can modify slack:notify or its dependencies to exfiltrate the token. Blast radius is Slack posting only.
Suggestion: Post-migration: scope the bot token minimally (chat:write to specific channels), or drop Slack notifications from PR runs.
| 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 — the plan's YAML uses github.actor per D3)
github.actor is whoever triggered the event, not the PR author; the two can diverge (e.g. re-runs on a dependabot branch another user has interacted with). GitHub's guidance for dependabot auto-merge is to verify the PR author.
Suggestion: Fleet-wide follow-up: gate on github.event.pull_request.user.login == 'dependabot[bot]'.
| check: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 |
There was a problem hiding this comment.
🔵 Security (plan concern / post-migration hardening)
actions/checkout@v4 (and throughout both workflows) pins to a mutable tag rather than a commit SHA. The PR description covers the deliberate @v1 tracking of the org-owned action, but not third-party actions — and these jobs (in main.yaml) handle ENCRYPTION_PASSPHRASE and RubyGems credentials.
Suggestion: Post-migration, pin third-party actions to full commit SHAs with a version comment and let dependabot's github-actions ecosystem keep them fresh.
| pull-requests: write | ||
| steps: | ||
| - name: Merge pull request | ||
| run: gh pr merge --merge "$PR_URL" |
There was a problem hiding this comment.
🔵 Safety (plan concern — plain --merge is a documented D3 decision)
gh pr merge --merge merges the moment this workflow's needs are green; any required checks or branch-protection rules added later would be bypassed at merge time rather than awaited. Noted for when branch protection is introduced: --auto --merge queues behind all protection requirements.
| -in ./config/secrets/ci/gpg.private \ | ||
| -out ./.circleci/gpg.private.enc \ | ||
| -out ./.github/gpg.private.enc \ | ||
| -k "<passphrase>" |
There was a problem hiding this comment.
🔵 Security (plan concern — inherited unchanged; plan says leave the openssl commands untouched)
The committed encrypted GPG key uses aes-256-cbc -md sha1 — single-iteration SHA-1 key derivation, no -pbkdf2, protecting a private key in a public repo. The passphrase is the only barrier and the weak KDF makes offline brute-force of the ciphertext cheaper.
Suggestion: Post-migration hygiene: re-encrypt with -pbkdf2 -iter 600000 (or store the key as an Actions secret) and update the README recipe.
| desc 'Set the git author for CI' | ||
| task :set_ci_author do | ||
| sh 'git config --global user.name "InfraBlocks CI"' | ||
| sh 'git config --global user.email "ci@infrablocks.com"' |
There was a problem hiding this comment.
🔵 Standards (plan concern — matches the family plan and rake_slack verbatim)
CI author email is ci@infrablocks.com while every other org email here uses infrablocks.io (e.g. maintainers@infrablocks.io in this Rakefile; the old CircleCI author was circleci@infrablocks.io). If .com is a typo it is being propagated fleet-wide via the plan — fix it there, not in this PR.
| ] | ||
| t.environments = [ | ||
| { name: 'release', |
There was a problem hiding this comment.
🔵 Code Quality (plan concern — plan mandates this task "exactly as in rake_slack's Rakefile")
repository:set_ci_author runs git config --global, so a developer invoking it locally silently overwrites their global git identity. The workflows push from the checked-out repo, so --local would suffice.
Suggestion: Fleet-wide follow-up: switch to --local in the plan/template.
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_keysprovisioningDeliberate 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.
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.Do not merge manually — cutover is: merge this PR, disable the CircleCI
project, delete the
CircleCIdeploy key from repo settings.