-
Notifications
You must be signed in to change notification settings - Fork 95
Create pull_request_template.md #112
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
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 making this change @goodylili, it looks great! I have just a couple of small comments/suggestions
.github/pull_request_template.md
Outdated
| Provide the desired commit message below: | ||
|
|
||
| Example: | ||
| [Category] Brief description of changes made |
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.
Since this repository only contains docs (and not code) it's probably not needed to use conventional commits, but if we do use them we should link to something that will help contributors understand what the available categories are. If you've got different categories in mind (since all the changes are docs changes), I think it would be good to list them here.
.github/pull_request_template.md
Outdated
|
|
||
| - [ ] I have read and followed the [Ubuntu Server contributing guide](https://documentation.ubuntu.com/server/contributing/). | ||
| - [ ] I have signed the [Contributor License Agreement (CLA)](https://ubuntu.com/legal/contributors). | ||
| - [ ] My changes are well-documented, and I have updated the documentation as needed. |
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 think we probably won't need this one, since all the changes in this repo relate to documentation :)
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.
yes meta - contributions here are documentation by definition - so this line could probably go away indeed
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.
Great already, some suggestions to make it even better :-)
.github/pull_request_template.md
Outdated
|
|
||
| - [ ] I have read and followed the [Ubuntu Server contributing guide](https://documentation.ubuntu.com/server/contributing/). | ||
| - [ ] I have signed the [Contributor License Agreement (CLA)](https://ubuntu.com/legal/contributors). | ||
| - [ ] My changes are well-documented, and I have updated the documentation as needed. |
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.
yes meta - contributions here are documentation by definition - so this line could probably go away indeed
.github/pull_request_template.md
Outdated
| - [ ] My changes are well-documented, and I have updated the documentation as needed. | ||
| - [ ] My pull request is linked to an existing issue (if applicable). | ||
| - [ ] I have tested my changes, and they work as expected. | ||
| - [ ] New and existing unit tests pass locally with my changes. |
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.
Unit tests in contributions to this repository likely never have unit-tests as it is not code.
Some time in the future we most likely re-enable some spell checks and other linting and once we do so we'd add a better matching line here then. For now I'd suggest to remove it.
|
Hi @goodylili, I just wanted to follow up to see how this is going. I noticed that you've marked all the review comments as resolved. Once you've pushed your changes, please feel free to request re-review to let us know it's ready :) |
|
Apologies for the delay @s-makin It's ready for review |
No apology needed :) I took another look but can't find the commits associated with your changes - are you sure you have pushed the commits? |
|
Oh yes, I resolved the suggestions |
Hi @goodylili I just wanted to check in with you to see how things are going as I'd love to get these changes merged. I noticed that you resolved the comments, but I still don't see the commits that are associated with the requested changes. If you did this work on the command line, don't forget to commit your changes and push them to your branch so that they show up here for us to see :) if you need any help with anything, please let me know! |
|
Hi @s-makin I've actually committed the changes and I did so from GitHub right here Do you mind explaining the problem further? |
Resolving suggestions so that we can merge the PR
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've taken the liberty of addressing the feedback so that we can resolve and close out this contribution. Thanks again @goodylili for your help in making the Server docs better :)
Hi @goodylili, sure thing! Your changes weren't showing up, which (if you're working on the command line) usually means the changes haven't been committed and/or pushed to the branch to update it. However, if you were working on the GitHub web interface, it works slightly differently but is the same idea - to get the changes to show up in the pull request via the GitHub web interface, you need to go to your fork and click on the "sync changes" button. I didn't realise you were using the web interface, I think I might need to clarify our instructions for contributors to describe these parts of the workflow as well. |
Description
The problem:
We are currently lacking a nicely-formatted Pull Request template.
What is needed:
It should include:
a link to the Contributor License Agreement
a reminder to sign the CLA, if the contributor has not already
Fixes: #1234 as a reminder to include the corresponding issue number, if the PR fixes an open issue
a space for a commit message that the author wants to be applied to a squash merge
Suggestions:
We currently don't have a PR template at all. There is some helpful guidance in the GitHub documentation about creating PR templates.
Related Issue
Checklist