-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Automate creation of Issue for major release PHP synchronisation #57890
Automate creation of Issue for major release PHP synchronisation #57890
Conversation
const content = generateIssueContent( result ); | ||
|
||
// Write the Markdown content to a file | ||
fs.writeFileSync( nodePath.join( __dirname, 'issueContent.md' ), content ); |
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.
Currently writes to a file. We could have it automatically create the Issue on Github.
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 not sure about this, since the generated content will almost certainly need a bit of editing. Might just be easier to write to file and the person generating it can copy/paste/edit before publishing.
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.
Sounds fine to me. It's a very manual script anyway. The overhead of auto-creating an Issue probably isn't worth the effort.
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 agree we shouldn't create an issue but we could actually output the content in the console. So the commands become something like yourcommand > issueContent.md
if you really want to put the content in a file. That seems more inline with how tools like that work.
bin/generate-php-sync-issue.mjs
Outdated
// The paginate method will fetch all pages of results from the API. | ||
// We limit the total results because we only fetch commits | ||
// since a certain date (via the "since" param). | ||
commits = await octokit.paginate( 'GET /repos/{owner}/{repo}/commits', { | ||
owner, | ||
repo, | ||
since, | ||
per_page: 100, | ||
path, | ||
} ); |
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 can generate a fair number of requests
bin/generate-php-sync-issue.mjs
Outdated
owner, | ||
repo, | ||
since, | ||
path |
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.
Not using path
will result in a tonne of commits that have little or no value. So actually it's better to only fetch commits against the paths we know will have PHP files that need to be synced.
? `https://github.com/WordPress/gutenberg/pull/${ prId }` | ||
: `[Commit](${ commit.html_url })`; |
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.
If we can't build the PR URL from the commit message, then we'll fall back to the commit URL itself and let GH UI show the link to the PR there.
I believe @ramonjd was working on something similar last year. |
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.
Thanks for the PR, this is working pretty well for me!
Regarding the generated content:
- the
compat/wordpress-6.4
folder can be removed, since whatever is in there is already in core. It exists to provide compatibility for WP 6.3, because we have to support the current and the previous WP versions. Not sure if there's a simple way to automate that as the folder numbers will vary per release - might be easier to just delete it manually. load.php
is plugin-specific so we probably don't need to look at it either.- not sure if this is possible: could we remove some PRs from the list if they have a certain label? Anything with
Backport from WordPress Core
can safely be ignored. - Would it be worth checking the
Needs PHP backport
label and including any PRs from there that aren't in the list?
One thing to bear in mind is that we should include all PRs since the last plugin release to be fully included in Beta 1, because anytime after that new features could have been merged that would still need syncing. See my comment on validateSince
below 😄
Other than that I think this is looking pretty good! I'm no node expert though so you might want a second opinion on the code itself.
const content = generateIssueContent( result ); | ||
|
||
// Write the Markdown content to a file | ||
fs.writeFileSync( nodePath.join( __dirname, 'issueContent.md' ), content ); |
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 not sure about this, since the generated content will almost certainly need a bit of editing. Might just be easier to write to file and the person generating it can copy/paste/edit before publishing.
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 great to me. Some valid remarks above from @tellthemachines but otherwise it's a perfect head start to the backporting process. Nice work everyone.
Thanks for review @tellthemachines 👍
Currently it will remove the folder for the current stable release but I think we need to make it a whitelist to retain only the folder for the next release and remove all others.
✅ Done
Apparently this label gets removed once the PR has been backported as a way of indicating it's completion status. So instead @youknowriad and I agreed that we would flag any PRs made between Beta 1 and final RC with a special warning.
Yes that would be a nice addition.
|
Given that this is not production code we could probably look to merge and iterate on this. |
@getdave "from WordPress Core" is another label and these can be safely ignored regardless of date... |
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.
LGTM! Small non-blocking typo and comment below. I think we can merge and iterate.
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.
Excellent work, Dave.
Maybe, as a follow-up, we can document the usage of this script. Here is an example of cherry-picking automation docs - https://developer.wordpress.org/block-editor/contributors/code/release/auto-cherry-picking/.
✅ I've set up an ignore list for any labels. This can be added to in followups. |
Just noting I used this script to generate #57959 |
What?
Automates the creation of the contents for the Issue that tracks PHP changes (example) that may need to be synchronised to WP Core for a major release.
Why?
For a major release, the Editor Tech Lead needs to create an Issue like this one which details all the PHP changes that may need backport to Core.
This requires manually going through all the key directories in the project that may contain PHP changes and then checking all the PRs that have been merged since the final RC of the previous release. If any PRs contain PHP changes then the PR needs to be added to the Issue in a very specific format.
It also requires organising headings for each seciton and doing a lot of manual formatting.
All this is:
This is why I think it's wide open for automation.
How?
This PR has a script which can be run locally which will
trunk
in a number of key directories (including/lib
,/block-library
and/phpunit
) since a date you provide.I've made some subjective choices about how things are group and displayed:
PRs may be listed only once in the Issue - the exception is PRs which modify PHP Unit tests as I wanted to display a full list of changes that affect theActually PRs are allowed more than once as long as it's across different sections/phpunit
directory.e2e-tests
andexperiments-page.php
which are never synchronised - please feel free to suggest additions to that list.Testing Instructions
node bin/generate-php-sync-issue.mjs --token={YOUR_TOKEN_HERE} --since=2023-11-01
issueContent.md
and copy all contents.Preview
the Issue - DO NOT SUBMIT IT!Testing Instructions for Keyboard
Screenshots or screencast