Hi,
I really like how Katello has template for new PRs, and I would like to propose doing the same in the Foreman core (and plugins). This is what they have:
#### What are the changes introduced in this pull request?
#### Considerations taken when implementing this change?
#### What are the testing steps for this pull request?
What do you think?
Not a huge fan of this, I don’t think a pull request is a right place for it. For the first two points, I’d say those should be included in the commit message. For the last one, we have an issue tracker (multiple ones in fact), which quite often already describe how to test/verify the proposed changes.
I’m also on the commit message fan train. It’s so useful when you use git log/blame to get a good context. Having to go back to a PR/issue tracker for context is tedious.
This should be nothing new, but I think How to Write a Git Commit Message is still one of the best resources out there.
What I typically do is write a commit message. In the PR I may provide some more context, like if it’s a draft then what remains to be done.
Another thing I’ve gotten used to is finding out where a bug was introduced and then add a Fixes: COMMIT. This is what the Linux Kernel does (Submitting patches: the essential guide to getting your code into the kernel — The Linux Kernel documentation). That’s very useful as a reviewer, but especially as a maintainer. Then you can use git tag --contains COMMIT to see which version introduced it. That doesn’t find cherry picks of the commit, but it’s very useful.
And now that I read the kernel process I see Submitting patches: the essential guide to getting your code into the kernel — The Linux Kernel documentation has many more good suggestions.
From the perspective of this Katello team member, I really like having the PR template. Good commit messages are great to have and should be encouraged. But in reality, most of our commit messages are single-line and don’t really explain much. One may think as the commit author that your 37-character commit message is self-explanatory, but tell that to the reader from a different team who’s trying to figure out what you did 2 years ago. Being able to look at a rich, detailed PR description can give really valuable context that wouldn’t otherwise be there.
Additionally, one of the biggest barriers I’ve found to reviewing others’ PRs is a lack of testing steps. If I have to go ask someone how to set up an environment to test their PR, there’s that much more chance of me just skipping the review. Also, putting the testing steps in the PR description allows the testing steps to change easily if the PR code changes. But who goes back and updates Redmines?
It does require some adjustment if you’re used to not doing it, but I think having a template is a net positive for everyone, especially those from a different time or team.
For the first two points, I’d say those should be included in the commit message.
I agree, but usually, they are not there.
For the last one, we have an issue tracker (multiple ones in fact), which quite often already describe how to test/verify the proposed changes.
Yeah but sometimes Redmine is just a headline with a category, bugzillas are filled with tons of comments (half of them private) and huge descriptions. And Jira is internal only.
What I typically do is write a commit message. In the PR I may provide some more context, like if it’s a draft then what remains to be done.
+1, and the “how to test” part could be just in the PR description
My point in this is to have a quick understanding of what is being posted. What, why and how to test are three crucial things I need to know for doing the review.