-
-
Notifications
You must be signed in to change notification settings - Fork 3.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
Add GitHub Action to automatically create cherrypick PRs #10361
base: master
Are you sure you want to change the base?
Conversation
Uses peter-evans/create-pull-request and actions/checkout, both of which are already dependencies. When merging a PR with a cherrypick label, which only contains a single commit, attempt to cherrypick the single commit into a new branch, then create a PR. Fails if there are multiple commits (to catch squash and merge). Fails if the cherrypick does not apply cleanly. Does not attempt to merge the new PR. Does not attempt to update stable.
Yaaay! Exactly the direction I had in mind and never found time for.
Yes; but IMO it is fine to still sync
That'd be super nice, but not blocking IMO.
Nice to have but not critical to me.
Likewise not critical, as the absence of a cherrypick PR referencing it kinda already implies something went wrong.
Nice to have, but also harder to handle logically -- e.g., we still have many PRs labelled for cherrypicking to older branches that shouldn't be cherrypicked anymore now that current stable has moved on (a lot of enhancement PRs usually). So personally I'd rather keep this manual, actually. Also, now that the team is growing we can hopefully move more towards anticipatory work (e.g. document the upcoming Godot versions before/as they come out), instead of having to catch up a lot. As we now get smaller, but more frequent releases it is also not as critical if some stuff is not, say, in 4.1 docs but in 4.2/4.3. This was a large issue in the past with the larger, slower releases. So overall, we will probably be increasingly more conservative in what we cherrypick to old branches, and how far back, keeping that increasingly to important bug fixes (docs that are outright wrong or misleading, for example). The only blocking things here to me are:
Thank you for working on this, this will make both the docs team and users super happy :) |
Squash and merge is now handled. For the sake of code review, you may be interested in the GH docs for the ternary hack and for the pull request event:
|
I think all the nice-to-haves will require one or more separate actions/workflows, which trigger on the merging of the automatically generated PR. I believe there is also a complication where PRs generated by actions usually don't trigger new actions to avoid recursion, unless you manually set it up. So if this workflow of automatically generating but manually merging PRs is acceptable, let's defer the nice-to-haves for later. |
Uses peter-evans/create-pull-request and actions/checkout, both of which are already dependencies.
When merging a PR with a cherrypick label, attempt to cherrypick a single commit into a new branch, then create a PR.
Handles squash and merge, by checking the number of commits in the PR, and choosing the commit to cherrypick accordingly.
Fails if the cherrypick does not apply cleanly.
Does not attempt to merge the new PR.
Does not attempt to update stable.
This will likely need to wait until we stop using
stable
, since each auto-cherrypicked PR would need a separate PR to update stable. We can definitely automate that with another action, and I think we can even make the second action trigger on merge of any of these automatic cherrypick PRs... but that seems very messy. If we wait until stable is no longer used, then these automatic cherrypick PRs can simply be merged after giving them a human review, and then will trigger a rebuild of the docs.To test this I recommend making a new
fake-master
or similar branch in your fork, create a5.3
branch and acherrypick:5.3
label, and then change the relevant strings in the workflow file to refer to those branches instead. At least, that's how I tested.Potential future improvements:
Optional:
contains()
. So I couldn't figure this out in the initial implementation. There should be some way, though.