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

.github: Add a pull request template for release branch PRs #72722

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

AnthonyLatsis
Copy link
Collaborator

Unfortunately, this change does not make the template automatically pop up when a pull request targets a release branch — the only way to achieve a similar effect involves GitHub Actions — but it at least provides a template one can tap into without having to dig up the form on the forums, and a way to propose changes to the form! Release process announcements may also start linking out to this template for both the form and usage instructions instead of copying it.

I deliberately chose a template name that is easy to remember with query parameters in mind.


This solution is still limited in that each applicable repository needs its own copy of the template in order for it to work as a query parameter for PRs against said repository. We can solve this with organization-level community health files (a pull request template is a kind of community health file).

* Risk: What is the (specific) risk to the release for taking this change?
* Testing: What specific testing has been done or needs to be done to further
validate any impact of this change?
* Reviewer: One or more code owners for the impacted components should review
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, I thought this was to mention the reviewer(s) who approved main branch PR as well. Should we mention in here? Or is that an useful information worth mentioning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is sketchy, so I just left it as is for now. Not every main branch PR is merged with (approving) reviews from code owners, and not every release branch PR is an atomic, full cherry-pick. The only reasonable thing this can mean that comes to mind is "A code owner or entitled delegate that has either already approved the main branch PR if the release branch PR is identical, or otherwise agreed to review both PRs". I do not see a point in having this entry unless the reviewer must be known at the time of opening the release branch PR.

@shahmishal What is this actual meaning of "Reviewer"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In practice this often means who reviewed the change in the main branch. As the approval on release branches in practice is often just the branch manager and the original reviewer doesn't necessarily review again. Tho preferably they would approve as well

Copy link
Collaborator Author

@AnthonyLatsis AnthonyLatsis Apr 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That much is clear 🙂 What I am wondering is if the authors of this form had other cases in mind. Like, who should it be if you merged the original PR in your own right or if there is no original PR? Logically, that would be someone who has agreed to review the possible back-port beforehand, but, if so, was this interpretation anticipated? Supposing it was, I think a regular review request is more sensible in this case because you don’t have to bother getting in touch with someone and arranging a review just to submit the PR. If that makes sense, then why have this entry at all when review info is derivable from the original PR in the other, predominant, case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

who should it be if you merged the original PR in your own right or if there is no original PR

The branch manager will request an additional review from somebody if they do not feel the review on the main branch PR was sufficient, if the change looks particularly risky and there was no review, etc. If there was no reviewer on the main branch PR, you can just write N/A or None and say why there was no reviewer.

@AnthonyLatsis
Copy link
Collaborator Author

@xedin ping

@AnthonyLatsis AnthonyLatsis force-pushed the release-branch-pr-tmpl branch from 5413ef2 to 0417080 Compare April 22, 2024 21:52
@AnthonyLatsis
Copy link
Collaborator Author

@swift-ci please smoke test

@AnthonyLatsis AnthonyLatsis enabled auto-merge April 22, 2024 21:55
@AnthonyLatsis AnthonyLatsis merged commit f5b1e49 into swiftlang:main Apr 23, 2024
3 checks passed
@AnthonyLatsis AnthonyLatsis deleted the release-branch-pr-tmpl branch April 23, 2024 01:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants