GitHub Actions has overlapping triggers which causes tests to run twice when pushing to an open PR

[AzfaarQureshi]

What is the issue
There are overlapping triggers for the test workflow which causes it to run twice if you commit to an open PR

Why is this a problem
Well, it's not that deep of a problem but:

• you have to wait longer for CI to resolve
• uses up twice the GitHub Actions minutes
• slows down all other workflows as the 21 redundant runners increase queue times

it's more of a mild inconvenience but the fix is super quick (I can make a PR if the change is green-lit 😄)

We can solve this by only allowing pushes on master to trigger the workflow

# test.yml
name: Test
on:
  push:
   # branches-ignore:
   # - 'release/*'
    branches: master # proposed addition
  pull_request:

Steps to reproduce

1. Open a PR
   • GitHub Actions test workflow only runs once under the pull_request trigger (expected behavior)
   • 21 total checks
2. Push a commit to the open PR
   • test workflow runs twice, once under pull_request (expected) and again under push (unexpected)
   • 42 total checks
     
     

[NathanielRN]

I think you can also use the pull request types as defined on the documentation:

on:
  pull_request:
    types: [opened, reopened]

[AzfaarQureshi]

@NathanielRN yup that could work as well! My only concern is that if we restrict the pull_request trigger instead of push, we'll have the tests run whenever a commit is pushed upstream, even if the branch has no PR associated with it. That seems a bit unnecessary because it's probably not that useful to have CI to run while your work is WIP.

what do you think?

[NathanielRN]

@AzfaarQureshi I personally find it really helpful to see tests on my commits, then when I make the PR I can be sure that it is passing tests and can be merged.

Not to mention that people have the ability to make pushes to their branches after its been approved. Please let me me if I misunderstood, but if you restrict it to only pushes to master then people can push something broken to their PR and tests won't run to block it from being merged. Running them on push means what you give is what you get: the code you pushed always has tests ran against it.

[AzfaarQureshi]

@NathanielRN oh, the tests will still run whenever you push new commits to an open PR. For example https://github.com/open-o11y/opentelemetry-python/pull/7/commits you can see that beside every commit there is a link to a unique execution of the test workflow (you can confirm this by looking at the runner ID and see they're diff). Restricting push to master only means that the push itself won't trigger a new workflow but commits to a branch with an open PR will still result in CI re-triggering

[NathanielRN]

@AzfaarQureshi

Right that's because by default the pull request includes the type synchronize which provides the behavior you explained.

Like I said a push on every commit seems useful to me, especially since it allows developers to see if their code passes tests on remote (without having to open a PR to check this!) which is the end goal. I've found in the past that passing only locally can be misleading.

[AzfaarQureshi]

@NathanielRN

I've found in the past that passing only locally can be misleading.

Not sure what you mean by this exactly because the tests will still be run upstream when you open the PR

Please let me me if I misunderstood, but if you restrict it to only pushes to master then people can push something broken to their PR and tests won't run to block it from being merged.

right, as you mentioned, the synchronize type will prevent this from ever happening since any new push to a branch with an open pr re-triggers the CI. Am I misunderstanding your point? we can continue the convo in thread on gitter if you want.

It seems like @codeboten also had a chore listed to restrict the push to run on master only 9f5de45. Was there prior community discussion/consensus on this matter?

Either way, @NathanielRN's suggested change

on:
  pull_request:
    types: [opened, reopened]

also achieves the same result of removing redundant workflows so that change is fine by me too 😄

[github-actions]

This issue was marked stale due to lack of activity. It will be closed in 30 days.

[lzchen]

Closing, we only run CI on pull requests now (except pushes to main).