Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move CI jobs for draft PRs to contributor's personal forks of Mill #3543

Draft
wants to merge 135 commits into
base: main
Choose a base branch
from

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Sep 13, 2024

This PR aims to reduce the bottleneck of contributors waiting for CI jobs on the shared Mill repo by moving CI for draft/WIP pull requests to each contributor's own fork, each of which has a separate concurrency budget (~20 concurrent workers) from the com-lihaoyi organization.

Once a PR is marked ready-for-review, com-lihaoyi CI begins, so the author effectively has a choice of where they want to run their CI workloads. Mill's main PR validation CI jobs are portable, do not need any special secrets or environment, and can run just fine in contributor's GH Actions environments

To do this:

  • Made the upstream build-linux/build-windows/test-docs CI jobs filter on github.event.pull_request.draft == false, and

  • Removed the branches: - main filter on the run-tests jobs so they begin running in contributor GH Actions when commits are pushed to a branch

  • Add a new Draft CI workflow that pushes a github status linking to the PR's branch in the contributor's fork, as an easy way to see the CI history for the PR there. This needs to run on pull_request_target rather than just pull_request to have the suitable permissions to update statuses

  • Some subtleties around handling ready_for_review events such that when a contributor marks a PR as ready for review, CI is re-triggered and handled appropriately without needing to subsequently push a new commit.

  • Substituted github.head_ref with github.ref_name in the concurrency.group key since the former only works on PR events while the latter also works on push events

This is a bit of an unusual setup AFAIK for Github Actions, but I've tested it manually a bunch and it seems to work, and should mitigate the bottleneck of multiple people updating their WIP PRs and having everyone blocked waiting on everyone else. Worst come to worst, if you mark a PR as ready_for_review it falls back to the old behavior, but this setup gives you an option to run draft PR validation on your own fork's GH Actions where it won't get blocked by other people's PRs. This is something you can actually already do today, but it requires some tedious fiddling that PR makes seamless

Tested on #3540 via git commit -am . --allow-empty && git commit -am . --allow-empty && git push origin head && git push upstream head~1:test-non-draft && git commit -am . --allow-empty && git push origin head, and targeting this PR against the test-non-draft branch in the Mill repo

@lihaoyi lihaoyi requested a review from lefou September 13, 2024 09:07
@lefou
Copy link
Member

lefou commented Sep 13, 2024

So IIUC, PR opened against Mill repository automatically run on Mill CI or the PR creator CI account, depending on the draft status of the PR? Do PRs need to be branches in the users fork or in the Mill repo or are both options supported? Also, if PRs are branches in the users forked repo, don't they already run in their CI?

@lihaoyi
Copy link
Member Author

lihaoyi commented Sep 13, 2024

Currently all PRs run in the com-lihaoyi/mill github actions. We previously disabled all branch-push triggers except for main, and PRs are typically from other named branches in the forked repo. PR validation runs on pull_request triggers, which run in Mill's own environment. This change removes the branch-push filter and filters the pull_request trigger to only apply to non-draft PRs

The current proposed change should work the same for both external PRs as wel as PRs from com-lihaoyi/mill's own branches; in both cases, the choice of trigger depends on whether the PR is a draft or not. Except for internal PRs they both end up running in the same place

@lefou
Copy link
Member

lefou commented Sep 13, 2024

So, this change is all about not running draft PR events but run all push events in any branch instead.

It looks to me, that we now run all commits to non-draft PRs, pushed to com-lihaoyi/mill, twice? One push event and one pull request event.

@lefou
Copy link
Member

lefou commented Sep 13, 2024

I started to open PRs from the main repository, since this allows scenarios like opening PRs against another PR branch, which gets autoamtically rebased to main, once the dependency got merged to main. To avoid duplicate runs, we now should always open PRs from forks, which is not the end of the world, but slightly less convenient.

@lihaoyi
Copy link
Member Author

lihaoyi commented Sep 13, 2024

Yes I believe you are correct that as of now this change means non-draft PRs from the main repository will run CI twice. Let me see if I can tweak the github actions conditionals to mitigate that

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.

2 participants