Conversation
There was a problem hiding this comment.
Pull request overview
This PR prevents GitHub auto-merge from merging pull requests before all CI workflows have finished by ensuring each workflow’s “gate” job produces a unique check name (instead of multiple workflows emitting a shared complete check context).
Changes:
- Renames the gate job in
lint.ymlfromcompletetolint-complete. - Renames the gate job in
build.ymlfromcompletetobuild-complete. - Renames gate jobs in non-PR workflows (
release.yml,publish.yml) for consistency (<workflow>-complete).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| .github/workflows/lint.yml | Renames the gate job to lint-complete so it can be required independently. |
| .github/workflows/build.yml | Renames the gate job to build-complete so it can be required independently. |
| .github/workflows/release.yml | Renames the gate job to release-complete for consistent naming (not PR-gating). |
| .github/workflows/publish.yml | Renames the gate job to publish-complete for consistent naming (not PR-gating). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19ec901486
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
To keep the Terraform configuration simple and so that you don't have to keep maintaining it each time a new workflow is added I recommend making lint and build reusable workflows, that are called from a single orchestrator ci.yml where the complete lives. We use this pattern on the quickstart repo and it's allowed that workflow to evolve without needing any coordination with anything outside the repo.
e.g.:
# .github/workflows/ci.yml
name: ci
on:
push:
pull_request:
jobs:
complete:
if: always()
name: complete
needs: [lint, build]
runs-on: ubuntu-slim
steps:
- if: contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled')
run: exit 1
lint:
uses: ./.github/workflows/lint.yml
build:
uses: ./.github/workflows/build.yml
leighmcculloch
left a comment
There was a problem hiding this comment.
Approving because I see the terraform change was already made in:
I'd still recommend what I suggest in #25 (review), but defer to you.
|
@leighmcculloch i decided against this so we can't bypass checks, ever, given our desire to have a more strict repo. Happy to undo the terraform and go with this if you think it doesn't bring much. |
|
Either should work to do that, defer to you. |
Enabling auto-merge could merge a PR before all CI finished. Both
lint.ymlandbuild.ymldefined a gate job namedcomplete, and branch protection required a single status check namedcomplete. GitHub matches required checks by context name and tracks only the latest run for a name, so two concurrentcompletechecks made the gate ambiguous — as soon as the fast lint gate reported success, the requirement looked satisfied and auto-merge could fire before the slower build finished.This introduces a single
ci.ymlorchestrator as the only PR gate:ci.ymlruns onpull_request(and pushes tomain), callslint.ymlandbuild.ymlas reusable workflows, and rolls them into onecompletejob thatneeds: [lint, build]. It owns thepermissionsandconcurrencyconfig.lint.ymlandbuild.ymlbecomeon: workflow_call; their own aggregator jobs are removed since the orchestrator'sneedsalready captures every nested job's result.publish.ymlandrelease.ymlkeep theircompleteaggregators; they fire only on release/dispatch events and don't gate merges tomain.Because
completeneedsboth lint and build, the single required check can't report success until all of CI finishes — so auto-merge waits on everything through one check.RELEASE.md's branch-protection section is updated to match.