Skip to content

Migrate pipeline from CircleCI to GitHub Actions#268

Closed
phelma wants to merge 1 commit into
mainfrom
gha-migration
Closed

Migrate pipeline from CircleCI to GitHub Actions#268
phelma wants to merge 1 commit into
mainfrom
gha-migration

Conversation

@phelma

@phelma phelma commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Cutover to GitHub Actions per the Variant A family plan (gem pilot).
Includes decommission — merging this PR completes the repo's migration.

  • main + pr workflows: loop-guard, check/test, prerelease, release environment gate
  • git-crypt unlock on the runner; encrypted CI GPG key moved to .github/
  • Slack notifications via rake_slack; dependabot auto-merge job
  • Rakefile provisioning swapped to rake_github secrets/environments; rake_circle_ci dropped
  • CircleCI pipeline removed: .circleci/, scripts/ci/, the CI SSH deploy
    key pair and its keys:deploy/deploy_keys provisioning

Deliberate decisions (not defects)

This cutover reproduces the CircleCI pipeline's behaviour, warts included;
fixing inherited hazards is post-migration work. In particular:

  • ./go release publishes to RubyGems before the version-bump commit is
    pushed — pre-existing ordering inside the untouched release logic.
  • Prerelease publishes on every push to main with no approval gate; only
    full releases are gated (environment: release).
  • Dependabot auto-merge accepts any update type that passes checks, and the
    merge does not trigger a release build — on CircleCI the merge commit
    carried [skip ci], so this matches. Updates ship with the next
    human-triggered release.
  • asdf_install@v1 is our own action (infrablocks/github-actions); we are
    happy tracking its major version tag.
  • Job scaffolding is repeated flat per job by design: the logic lives in the
    build system (./go/rake) and CI stays lean — it just triggers tasks and
    supplies secrets/context.
  • Gemfile.lock carries transitive major bumps — the unavoidable resolution
    of the targeted bundle lock --update, not scope creep.

Do not merge manually — cutover is: merge this PR, disable the CircleCI
project, delete the CircleCI deploy key from repo settings.

@phelma
phelma marked this pull request as ready for review July 9, 2026 14:18
Copilot AI review requested due to automatic review settings July 9, 2026 14:18

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

Comment thread .github/workflows/pr.yaml
Comment thread .github/workflows/pr.yaml
Comment thread .github/workflows/pr.yaml
Comment thread .github/workflows/main.yaml
Comment thread .github/workflows/main.yaml
Comment thread .github/workflows/main.yaml
Comment thread .github/workflows/main.yaml
Comment thread .github/workflows/main.yaml
phelma

This comment was marked as outdated.

@phelma

This comment was marked as outdated.

phelma

This comment was marked as outdated.

@phelma phelma left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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 pull steps in prerelease and release mean the published artefact can include commits this run never checked/tested, and the release environment approval effectively approves "main at approval time" rather than a specific SHA. Combined with the main concurrency 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 the infrablocks.io domain used elsewhere) and the --global scope of repository:set_ci_author come 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 than github.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: read with per-job elevation, concurrency serialising publishes, continue-on-error notify steps, and the deliberate plain gh 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_PASSPHRASE or RubyGems credentials — a genuine trust-boundary improvement over CircleCI.
  • ✅ Complete decommission: CircleCI config, all nine scripts/ci wrappers, the SSH deploy key pair and its provisioning, and the rake_circle_ci/rake_ssh dependencies 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 release environment with maintainers reviewers, provisioned via rake_github.

General Findings

All of the following are plan concerns or post-migration follow-ups — none blocks this diff:

  • 🔵 Correctness: the skip-ci-check job guards a path GitHub Actions already handles natively ([skip ci] etc. suppress push workflows; GITHUB_TOKEN pushes 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 .yaml while dependabot.yml uses .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

Comment on lines +12 to +14
concurrency:
group: main
cancel-in-progress: false

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 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.

Comment on lines +75 to +76
- name: Pull latest main
run: git pull

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🟡 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.

Comment on lines +115 to +119
- uses: actions/checkout@v4
with:
ref: main
- name: Pull latest main
run: git pull

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 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.

Comment thread .github/workflows/pr.yaml
continue-on-error: true
run: ./go "slack:notify[${{ job.status }}]"
env:
SLACK_BOT_TOKEN: ${{ secrets.SLACK_BOT_TOKEN }}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 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.

Comment thread .github/workflows/pr.yaml
merge-pull-request:
needs: [check, test, build]
runs-on: ubuntu-latest
if: github.actor == 'dependabot[bot]'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 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]'.

Comment thread .github/workflows/pr.yaml
check:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 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.

Comment thread .github/workflows/pr.yaml
pull-requests: write
steps:
- name: Merge pull request
run: gh pr merge --merge "$PR_URL"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 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.

Comment thread README.md
-in ./config/secrets/ci/gpg.private \
-out ./.circleci/gpg.private.enc \
-out ./.github/gpg.private.enc \
-k "<passphrase>"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 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.

Comment thread Rakefile
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"'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 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.

Comment thread Rakefile
Comment on lines +158 to +160
]
t.environments = [
{ name: 'release',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

🔵 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.

@phelma phelma closed this Jul 9, 2026
@phelma
phelma deleted the gha-migration branch July 9, 2026 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants