-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Post-branching changes for the 6.9 branch #10512
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
base: 6.9
Are you sure you want to change the base?
Conversation
This runs the `post-branching` Grunt task from WordPress#10511 to test that PR.
Also updates the `cli` image version.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
peterwilsoncc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me with a couple of questions inline.
The changes in the other files match what I am seeing on other branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unclear why this file is being dropped, shouldn't these be run on PRs for all branches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mistakenly thought that workflow_run events would use the version of the workflow file in the primary branch, but that seems to not be the case.
The documentation also does not explicitly clarify this. It does say "This event will only trigger a workflow run if the workflow file exists on the default branch." But that doesn't mean the actual workflow file in the default branch will be used, just that it has to be there to run at all.
This is not great because it means we lose the benefits of the reusable pattern by including a workflow with business logic in old branches.
I'm thinking that the added maintenance burden of including this workflow in older branches likely outweighs the benefits. Especially given that:
- almost every change committed to non-default branches should be first committed to
trunkand opening a PR withtrunkas a base branch will run the workflow - After the next version of WordPress is released, pull requests are almost never opened against older branches.
- The Test Build Process workflow already runs nearly every build task that creates built files before checking for any uncommitted changes to files under version control. This will at least flag things that were missed without automatically fixing, committing, and pushing the updates back.
If we decide to remove this in older branches, then the check-built-files.yml workflow should also be deleted because there's no point running it if this one will not be triggered on workflow_run with the completed type.
@johnbillion do you happen to have any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is retained in the 6.8 branch. Is the error here in in the 6.8 branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could have sworn there was a discussion somewhere proposing to create a standardized rule around this for old branches and apply it consistently. There is Core-62416, but that is just about switching to remote references instead of local ones to eliminate the majority of the maintenance burden.
I'll try to see if I can recall what I am thinking of in relation to this. But if I'm dreaming that up, this and #10511 can serve as a way to create those guidelines and we can apply them consistently in all old branches.
This builds off the patch attached to the core ticket and tests the changes in #10511 by running the new Grunt task being proposed there.
Trac ticket: Core-64235.
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.