Make WordPress Core

Opened 2 years ago

Last modified 8 weeks ago

#57556 new enhancement

Configure feature branches on GitHub to trigger workflows to support development on forks

Reported by: flixos90's profile flixos90 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Build/Test Tools Keywords: has-patch needs-docs
Focuses: Cc:

Description

When contributing to WordPress core through GitHub, the workflow is to create new branch in a fork, and open a pull request from there to WordPress core.

This works reasonably well for smaller pieces of work, but when it comes to larger features it is not a great development experience having to work in a single pull request for the entire effort. A simplified workflow can be to work in individual issues and pull requests within the fork, until a fully functional version of the feature or larger enhancement is ready for an actual WordPress core pull request.

This workflow is already possible today, however one caveat is that as long as the development happens on the fork, the GitHub workflows are not triggered unless the work happens against the trunk branch. That however is not a good idea in itself as the trunk branch is the main development branch and not intended for work on specific features. It would make it impossible to work on multiple features in the same fork if they had to be developed against trunk.

This ticket therefore proposes to add a certain branch pattern to the CI configuration of WordPress core's GitHub workflows, e.g. to ensure that they run for any branches satisfying the feature/* pattern.

Of course in theory any fork could implement that individually, but that would eventually not be a great workflow as then every PR against the main WordPress/wordpress-develop repository would include that change, which it should not.

Since no actual development happens within the WordPress/wordpress-develop repository directly, I'd argue there is no harm in adding this extra branch rule. What it would help though is to support the above workflow to develop greater features iteratively in a fork.

Change History (11)

This ticket was mentioned in PR #3908 on WordPress/wordpress-develop by @flixos90.


2 years ago
#1

  • Keywords has-patch added

#2 @joemcgill
2 years ago

I think this is a great suggestion and would greatly help contributors review smaller changes as part of larger feature projects.

#3 @peterwilsoncc
2 years ago

Preventing actions from running on forks was an intentional decision in [49781].

Allowing actions to run on forks was using minutes on contributors accounts, and potentially costing them money.

An exception exists for pull requests against the forks, so in the use case you describe a pull request can be opened against the fork and the action will run if actions are enabled on the repository.

See this pull request against my fork to demonstrate running actions.

#4 @flixos90
2 years ago

@peterwilsoncc Thank you for the additional context, that's really helpful.

I believe the exception you're referring to only works when opening a fork PR against trunk though. My proposal here would be to expand that to allow for the same to happen when working against feature/* branches. As outlined in the ticket description, working against trunk in a fork is not really feasible when working on a bigger feature that consists of several PRs. It's more appropriate to work against feature specific branches. Also when working against trunk it's not possible to work on multiple decoupled features within the same fork.

#5 @jorbin
2 years ago

  • Keywords needs-docs added

I think the reasons that @peterwilsoncc mentions are valid, but I think we can get around them with some documentation and making it specific to a naming convention. I worry that feature/* is too generic though. Maybe we make it something WP specific? wp-pipeline/* perhaps to indicate that this is change that should trigger WordPress pipeline actions?

Adding the needs docs keyword since this is something that we will want to communicate in both a short-term sense on make/core but also in a long-term sense by making either a handbook update or perhaps also a CONTRIBUTING.md update.

#6 @peterwilsoncc
2 years ago

It it's possible to verify intent, then I am happy to allow them to run.

An alternative I was thinking of was to require the branch include a file .run-gh-pipelines-and-accept-charges-against-fork -- if such a thing is possible with the action config files.

#7 follow-up: @flixos90
2 years ago

@jorbin @peterwilsoncc Given the additional context you provided, I agree we would want to verify intent to run the CI workflows. I think a branch name prefix like wp-pipeline/* works well for that purpose. I think including a specific file does not work so well as it would need to be created per branch, and then it would also inevitably show up in the eventual PRs against WordPress/wordpress-develop - unless it was deleted, but then also CI workflows would stop running for PRs in the fork. So I think a specific branch name prefix works better.

Adding the needs docs keyword since this is something that we will want to communicate in both a short-term sense on make/core but also in a long-term sense by making either a handbook update or perhaps also a CONTRIBUTING.md update.

I am happy to draft some documentation for that. Related question though, is there any documentation on how this currently works? Where is it documented that fork PRs against trunk trigger GitHub workflows for example?

#8 in reply to: ↑ 7 @peterwilsoncc
2 years ago

Replying to flixos90:

I am happy to draft some documentation for that. Related question though, is there any documentation on how this currently works? Where is it documented that fork PRs against trunk trigger GitHub workflows for example?

I'm not sure that it is. I only realised when I was hunting down the historical commit and found it documented there.

You're right, a branch prefix makes more sense than a requirement. A lot less error prone. :)

#9 @peterwilsoncc
3 months ago

Some time later...

How about using an environment variable to allow folks to turn it on in their fork? ACTIONS_APPROVED_FOR_FORK or some such.

#10 @dd32
3 months ago

Or, what about running a highly limited subset of the core tasks by default?

Realistically, contributors don't need the full PHPUnit PHP/MySQL matrix of tests, a single test run of PHP $latest would likely meet their needs. Likewise, the vast majority probably don't need the Performance tests or Installation-testing tests. The full kitchen sink could be configured to run either on-demand or left until PR time.

Initially my thinking was that could be a separate workflow that was something like contributor-run-tests that ran on pushes to any non-core-like branches (to reduce the minutes used when syncing forks)

#11 @peterwilsoncc
8 weeks ago

I did a test using an environment variable to allow running tests against feature/ branches with a PR against my own repo. As it's a POC, I only tested with the phpunit test suite.

See peterwilsoncc/wordpress-develop#83

If the variable ALLOW_FEATURE_BRANCHES == 'true' the actions run, otherwise they are skipped.

Note: See TracTickets for help on using tickets.