8000 Create pull_request_template.md by goodylili · Pull Request #112 · canonical/ubuntu-server-documentation · GitHub
[go: up one dir, main page]

Skip to content

Conversation

@goodylili
Copy link
Contributor

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

  • I have read and followed the Ubuntu Server contributing guide.
  • I have signed the Contributor License Agreement (CLA).
  • 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.

@goodylili goodylili marked this pull request as ready for review January 11, 2025 11:33
Copy link
Collaborator
@s-makin s-makin left a 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

Provide the desired commit message below:

Example:
[Category] Brief description of changes made
Copy link
Collaborator

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.


- [ ] 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.
Copy link
Collaborator

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 :)

Copy link
Contributor

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

Copy link
Contributor
@cpaelzer cpaelzer left a 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 :-)


- [ ] 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.
Copy link
Contributor

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

- [ ] 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.
Copy link
Contributor

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.

@goodylili goodylili closed this Jan 16, 2025
@goodylili goodylili deleted the patch-1 branch January 16, 2025 10:35
@goodylili goodylili restored the patch-1 branch January 16, 2025 10:45
@goodylili goodylili reopened this Jan 16, 2025
@s-makin
Copy link
Collaborator
s-makin commented Jan 21, 2025

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 :)

@goodylili
Copy link
Contributor Author

Apologies for the delay @s-makin

It's ready for review

@goodylili goodylili requested review from cpaelzer and s-makin January 23, 2025 13:45
@s-makin
Copy link
Collaborator
s-makin commented Jan 23, 2025

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?

@goodylili
Copy link
Contributor Author

Oh yes, I resolved the suggestions

@s-makin
Copy link
Collaborator
s-makin commented Feb 7, 2025

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!

@goodylili
Copy link
Contributor Author

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
Copy link
Collaborator
@s-makin s-makin left a 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 :)

@s-makin s-makin merged commit 6cf7883 into canonical:main Feb 12, 2025
1 check passed
@s-makin
Copy link
Collaborator
s-makin commented Feb 12, 2025

Hi @s-makin

I've actually committed the changes and I did so from GitHub right here

Do you mind explaining the problem further?

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Admin: Create a PR template

3 participants

0