TRU-342: Migrate CI/CD from Azure Pipelines to GitHub Actions - #34
Conversation
zhianchow
left a comment
There was a problem hiding this comment.
Approving — checked the new ci.yaml against quantum-sec/actions@v1.4.0 and against what the deleted azure-pipelines.yml actually did. No functional gaps; notes below are non-blocking.
Verified
- Interfaces line up:
terraform-module-validation.yamltakes no secrets and defaults to.terraform-version;semantic-release.yamlrequiresGIT_TOKEN_BASICand runsnpm ci(hence the lockfile);update-source-reference.yamlrequirespackage_nameplus exactly the four secrets passed here. devDependenciesare identical to the package-pki set that already released successfully, all three are on public npm, andpackage.jsonkeeps"private": true— no accidental publish path.- Existing tags here are bare semver (latest
1.7.3), which satisfies the strict-semver guard inupdate-source-reference.yaml. ?ref=pins forpackage-log-analyticsdo exist in the default target (infrastructure-modules, default branchmaster), in the?ref=X.Y.Z"form the sed expects — so the auto-bump job has something to rewrite rather than failing on an empty commit. Chain proven upstream: package-pki 1.1.0 →Update package-pki references to 1.1.0 (minor)(dd14fde).- Parity with the old pipeline: pre-commit ✅, Checkov soft-fail ✅, semantic-release gated to master ✅, infrastructure-modules auto-bump ✅. Terratest is not a loss — the old definition set
runFunctionalTests: false. No.gitmoduleshere, so dropping the submodule-init step is fine. Validating PRs is a net gain over the oldpr: none.
Non-blocking
- The description says
@v1.3.0, but the file pins@v1for validation and@v1.4.0for the two secret-bearing workflows. The floating major tag is the deliberate split-pin policy from quantum-sec/actions#6 — worth stating that in the body rather than describing it as "version-pinned … @v1.3.0", since the floating pin is the part a reviewer would want to weigh. push: feature/*, fix/*combined withpull_request: master, mainmeans every commit on a branch with an open PR validates twice. This PR has done exactly that — all three commits produced apushrun and apull_requestrun of the full suite. The old definition avoided it withpr: none. Droppingfeature/*/fix/*from the push trigger reclaims the duplicate half.- No
concurrency:stanza, where the old trigger hadbatch: true. Two pushes to master in quick succession could run twosemantic-releasejobs at once. Low risk at this repo's traffic, but it's a one-stanza guard.
zhianchow
left a comment
There was a problem hiding this comment.
Re-approving after the CI hygiene commit. Both non-blocking notes from my earlier review are addressed, and I've confirmed the effect rather than just the diff: this PR's head now produces a single pull_request run instead of the push/pull_request pair, with all checks green.
- Feature-branch push triggers dropped — validation runs once per commit via
pull_request, and pushes to master/main are unchanged, so the release and auto-bump gating is untouched. concurrencyper workflow+ref added withcancel-in-progress: false, thebatch: trueequivalent.
One carry-over nit, no need to hold the merge: the comment's "superseded runs are not cancelled" isn't what GitHub does — cancel-in-progress: false protects the in-progress run, but a previously pending run in the group is cancelled when a newer one queues. Three quick master pushes run the first, drop the second, run the third. No practical consequence here (semantic-release is cumulative, so the surviving run covers the dropped commit), but the sentence now appears in eleven files across the fleet and reads as a guarantee that no run is ever dropped.
zhianchow
left a comment
There was a problem hiding this comment.
Re-approving after the concurrency-comment correction. Comment-only — one file, 5+/3-, no change to any YAML key — and the new wording is accurate: cancel-in-progress: false protects the in-progress run, and a superseded pending run is replaced rather than queued.
I also checked that the stated rationale actually holds for this repo, rather than taking it as boilerplate: validation here runs pre-commit run --all-files over the full tree, and releases are cumulative, so a surviving run genuinely does cover a replaced one's content. Checks green on the new head.
Track A of TRU-342: replaces the Azure Pipelines definition with consumers of quantum-sec/actions under the split pin policy — validation (secret-free) rides the floating
@v1major tag; the release and auto-bump workflows (secret-bearing) are exact-pinned@v1.4.0.Changes
ci.yaml: validation (terraform-module-validation@v1), release (semantic-release@v1.4.0), auto-bump (update-source-reference@v1.4.0).releaserc.json+package.json/lockfile:@quantum-sec→@cncscrelease stack (semantic-release 21)azure-pipelines.ymldeleted in the same commit (the AzDO agent's global npm-link of the old@quantum-secsemantic-release v19 conflicts with the local v21 stack — they cannot coexist)