Skip to content

ci: run checks on every pull request, not just those targeting master - #24

Open
piotr-roslaniec wants to merge 1 commit into
devfrom
ci-cover-all-pull-requests
Open

piotr-roslaniec wants to merge 1 commit into
devfrom
ci-cover-all-pull-requests

Conversation

@piotr-roslaniec

Copy link
Copy Markdown

Problem

test.yml and gofmt.yml both filter pull_request events to base branch master:

pull_request:
  branches:
  - master

This fork stacks PRs on each other's branches, and the whole in-flight batch targets dev. For a pull_request event GitHub uses the workflow from the base branch, so with this filter any PR not targeting master gets zero checks.

Observable right now: #17 and #23 both report no checks reported on the ... branch. That is a trigger gap, not queue lag — #11 and #9 look green only because their runs were triggered back when they targeted master (verified: those runs' headSha still match the current heads, so the results are valid, just not reproducible for new pushes).

Net effect: the constant-time stack is about to be merged on locally-run tests as the only evidence.

Change

  • Drop the pull_request branch filter entirely, so every PR gets CI whatever it targets. A fixed allow-list would need updating for each new stacked base branch and would silently fail the same way again.
  • Add dev to the push trigger so the integration branch is covered on merge.

Notes

  • Targets dev because that is where the gap bites, and because pull_request resolves workflows from the base branch — landing it on dev is what gives the in-flight batch coverage. master picks it up whenever dev merges forward.
  • This PR itself will not show checks: dev's copy of the workflow still has the old filter until this merges. Expect checks to start appearing on subsequent PRs.
  • No job logic changed, only triggers.

Both workflows filtered pull_request events to base branch master. This fork
stacks pull requests on each other's branches, so every stacked PR and every
PR targeting dev ran with no checks at all: #17 and #23 currently report no
checks, and the whole in-flight batch targeting dev is covered only by
locally-run tests.

Drop the pull_request branch filter so any PR gets CI regardless of base, and
add dev to the push trigger so the integration branch is covered on merge.
@piotr-roslaniec

Copy link
Copy Markdown
Author

Worth flagging a limit of this PR that isn't obvious: merging it does not by itself give #17 and #23 CI.

Because GitHub resolves a pull_request workflow from the PR's base branch, this change only applies to PRs whose base already contains it. Checked both stacked bases — constant-time-hardening-backport and codex/ct-defaults-and-bounds still carry pull_request: branches: [master]. So once this lands on dev: #11 (base dev) gets CI, #17 and #23 still get none.

Suggested merge order, which needs no branch surgery:

  1. merge this PR into dev
  2. merge fix(crypto): rebase and verify constant-time hardening backport onto current master #11 into dev — now CI-covered
  3. retarget Enable bounded bigmod operations by default #17 to dev (it picks up dev's workflow, and therefore CI), merge
  4. retarget fix(crypto): extend constant-time coverage to Schnorr proofs and signing rounds 3-5 #23 to dev likewise, merge

Retargeting is content-neutral here because the stack is a linear, fast-forwardable chain: dev#11#17#23, each base an ancestor of the next (verified with git merge-base --is-ancestor). Collapsing each layer onto dev as the one below it lands changes nothing about the diffs.

Alternative if you'd rather not retarget: merge dev forward into each stacked base branch after this lands. Same effect, more moving parts.

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.

1 participant