chore(ci): avoid duplicate CI runs for pushes to non-fork PRs #11519
Conversation
WalkthroughThe GitHub Actions "test" workflow configuration was updated to change its trigger conditions. Now, it runs on pushes only to the Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (21)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@sgarner probably contributors should be using forks instead, so the workflows run in their own forks :) I think it's okay that the test workflows run on every branch - then the contributors (whether they have write access to this repo or not) will see the test results in their own forked repository and they will not need to wait for workflow approval from one of us. I am tempted to decline this PR. |
That's a fair point - although I doubt that many contributors work that way. Workflows are disabled on forks by default, so they would have to specifically go in and enable the option to do this. I've observed that most people only run tests on their local machine (if at all) before submitting a PR. If we want to support that, additional conditions could be used to adapt the workflow depending on whether the branch is on a fork or not. Other than working around this issue, what would be the benefit of forcing maintainers to work on forks? |
Hm, it didn't request me to enable anything when I forked the repo, though it was very long ago. Maybe we should update docs to tell people how to enable CI/CD on their fork? |
Possible it was different if you forked before typeorm was using GitHub Actions. Here's what appears now: 
Of course if you do this with the current trigger settings it causes the same duplicate CI runs issue to occur in your fork. Every push to a pull request will then trigger jobs in both your fork and the upstream (though this will be less visible since the jobs in the fork won't appear as checks on the PR). |
|
I think I had to enable it, but it was one time only so I vaguely remember. Either way, I like that I can run the workflows in my own fork before I open the PR. |
|
@sgarner I think I might know how to solve this, by checking the repository owner: jobs:
database-tests:
if: ${{ (github.repository_owner != 'typeorm') || (github.ref_name == 'master' && <something to check it's a not PR> ) }}
|
|
Actually I'm wondering, why don't we keep the Later edit: nevermind, I understand why :) |
Description of change
Previously, pushes to branches in this repository with pull requests would trigger duplicate CI runs:
This is because our workflow uses both the
pushandpull_requestevents, and GitHub Actions doesn't do any de-duplication. So a push to atypeorm/typeormbranch with a PR would trigger both events (producing 86 test jobs instead of 43).This didn't happen very often because most PRs are from outside contributors on branches in their own forks, which would only trigger the
pull_requestevent. However it would occur when maintainers with write access created PRs using non-fork branches intypeorm/typeorm.We can fix this by changing the trigger branch criteria:
Filter the
pushevent so it only triggers for pushes to themasterbranch. We have (I believe?) branch protection rules to ensure that commits can't be made directly tomaster, but this ensures we re-validatemasterafter a PR is merged. Since all changes require a PR, CI doesn't need to run on other branches unless a PR is opened.I've also widened the filter on the
pull_requestevent so it triggers for PRs based on all branches. I don't see any need to restrict this, and it ensures that tests can run in chained PRs.Summary by CodeRabbit