CI - Stop triggering two CI events when PR commits are pushed #110
Conversation
.github/workflows/dotnet.yml
Outdated
| pull_request: | ||
| push: | ||
| branches: | ||
| - master |
There was a problem hiding this comment.
this matches the behavior in SpacetimeDB (https://github.com/clockworklabs/SpacetimeDB/blob/master/.github/workflows/ci.yml)
There was a problem hiding this comment.
I think SpacetimeDB has somewhat different constraints because it's so large / expensive to build, whereas this repo takes less than a minute.
I love being able to run CI on branches even before I make a PR. Could we preserve that?
There was a problem hiding this comment.
updated to run on push events but not synchronize events
bfops
changed the title
push events only trigger on masterpush events only trigger on pushes to master
bfops
changed the title
push events only trigger on pushes to masterpull_request events don't also trigger on push
| on: | ||
| push: | ||
| pull_request: | ||
| types: [opened, reopened] |
There was a problem hiding this comment.
The default (i.e. if types isn't specified) is [opened, reopened, synchronized].
The synchronized event happens whenever commits are pushed to a PR, which was redundant with us also triggering on all pushes.
See also https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request
There was a problem hiding this comment.
The
synchronizedevent happens whenever commits are pushed to a PR, which was redundant with us also triggering on allpushes.
IIRC they do that intentionally because the commit on the branch itself might be different from a merge commit when PR is out of date (not rebased), so the test result might be different as well? Admittedly, it's an edge case, but sometimes it might potentially catch issues early.
There was a problem hiding this comment.
Good point.. but it seems like this repo requires PRs to be up to date with master before merging, so I think it might not apply here?
There was a problem hiding this comment.
actually.. based on https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=synchronize#pull_request, it looks like synchronize is probably only triggered when the head branch is pushed.
I think there might be a different option/event type for testing the merge commits? IIRC they're pushed to some other ref that gets CI tested.
There was a problem hiding this comment.
Oh huh. I probably confused the event types. But yeah, either way the base branch requirement takes care of that.
@RReverser Added a hopefully-clarifying comment here: https://github.com/clockworklabs/spacetimedb-csharp-sdk/pull/110/files#r1693270679 |
bfops
changed the title
pull_request events don't also trigger on push
Description of Changes
Currently, a push to a PR triggers our
dotnetworkflows twice: once for thepushevent, and once for thepull_requestevent.This PR sets the
pull_requestevents to only beopenedandreopened, so that a push to the branch only triggers thepushevent and not the PRsynchronizeevent as well.Testing