-
Notifications
You must be signed in to change notification settings - Fork 148
Add Github CHERRY_PICK pull request template #300
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
Conversation
Combined the template from https://forums.swift.org/t/swift-5-7-release-process/56316#pull-requests-for-release-branch-7 and #248. |
@swift-ci please test |
Co-authored-by: Franklin Schrans <[email protected]>
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.
Thank you @Kyle-Ye!
@swift-ci please test and merge |
@swift-ci please test |
@Kyle-Ye Hmm I'm not seeing the template options when creating a PR, any ideas? |
|
Unfortunately there is not a GUI list like Github issue for us to choose when create a Pull Request on Github website. |
Ooh I see, that's too bad. I think in that case I prefer the previous behaviour where you get just the default template; the point was that you have something to fill in if you're filing a new PR. What do you think? Should we revert the change? |
I think there should be something that we can configure the default template file like |
Ok, if that'll take some time, should we revert the change for now and if the config file works, re-introduce the templates with the extra file? |
There are many people encountering the same problem. Here is a solution https://github.community/t/is-there-a-pull-request-template-selector-similar-to-issues/171838/9 If you accept, we can move DEFAULT.md to its original location and name. Then the default behavior will come back |
|
I think let's revert to the original single template, but add a commented out section at the bottom with the template with the cherry-pick instructions: ## Checklist
…
<!--
Note: If this PR cherry-picks a change for a release branch, use the following template instead:
- **Explanation**: _A description of the issue being fixed or enhancement being made. This can be brief, but it should be clear._
- **Scope**: _An assessment of the impact/importance of the change. For example, is the change a source-breaking language change, etc._
- **GitHub Issue**: _The issue number if the change fixes/implements an issue/enhancement on [GitHub Issues](https://github.com/apple/swift-docc/issues)._
- **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 the change. Technical review can be delegated by a code owner or otherwise requested as deemed appropriate or useful._
-->
What do you think? That seems to be the cleanest solution. |
Revert the PR now is totally OK. But I disagree with the approach you describe. Github has multi PR template feature supported. If we have a need for it, we should follow the Github's official spec instead of using textual form to workaround which seems error-prone for me.
If we want to revert to the original behevior, we can move the DEFAULT.md to its original location. Then when creating a PR on Github's website, it will use it as the default template. And also I think we should still put the extra template on PULL_REQUEST_TEMPLATES folder for the following reasons:
|
We still lose discoverability of the cherry-pick template though, at least on the web, and for now. Maybe we can do something in the middle as well, where the comment I'm proposing to add at the bottom can instead refer to the other template:
|
Update #301, could you help review it? |
By the way, there is a PR #232 to update the issue template to the latest yaml form so that we can fill in issues more easily(In a GUI friendly way).
|
Bug/issue #, if applicable:
Summary