contribution guidelines: change to say updating changelog is required #5586
Conversation
TravisEz13
changed the title
|
If I could kindly request that the PR checklist include the the correct level of tests have been run and passed (i.e. feature has been run on feature affecting changes). |
|
Added, we didn't have any such language in the contribution guideline. I've added language to the template. If my language needs improvement but is adequate. I'd suggest filing an issue and we can fix it with a subsequent PR. |
.github/CONTRIBUTING.md
Outdated
|
|
||
| #### Pull request - Change log | ||
|
|
||
| * All PRs must either update the [changelog](../CHANGELOG.MD) in your pull request. |
There was a problem hiding this comment.
either is either inappropriate or part of this sentence is missing
| #### Pull request - Change log | ||
|
|
||
| * All PRs must either update the [changelog](../CHANGELOG.MD) in your pull request. | ||
| New changes always go into the **Unreleased** section at the top of the changelog. |
There was a problem hiding this comment.
What about calling out Breaking Changes? Is categorization of the changes handled as part of the PR update or continue to be done by maintainers? I think ideally, the PR author should make a call and PS-Maintainers and PS-Committee should review before publishing.
.github/PULL_REQUEST_TEMPLATE.md
Outdated
| - [ ] Use the present tense and imperative mood when describing your changes | ||
| - [ ] [Summarized changes](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission) | ||
| - [ ] [Update the changelog](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---changelog) | ||
| - [ ] User facing [Documentation needed](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission) (remove if not needed) |
There was a problem hiding this comment.
Can we say 'insert NA' if not needed rather than removing? Easier to catch if there's a disagreement.
| @@ -1,8 +1,11 @@ | |||
| <!-- | |||
| ## PR Checklist | |||
There was a problem hiding this comment.
Is this right place? I believe the check list is for maintainers only.
I'd prefer the template here:
### Problem description
- The problem investigation results. Often we don't have understanding that we really fix - so we should describe/repo the problem here.
- Link to Issue in simple cases.
### Fix description
- Description what and how we fix.
### Additional considerations
- Which is not part of this change.
- More information to best understanding this change.
There was a problem hiding this comment.
The Problem investigation and results should be in the issue.
This checklist is all items that by our defined process is supposed to be done by the contributor except for the one new item this PR specifically adds and the one @markekraus asked me to add.
Change Summary is supposed to be what you call the "fix description" and we the link in the checklist describes how to produce this. Change Summary is more appropriate because not all PRs are fixes.
There was a problem hiding this comment.
The Problem investigation and results should be in the issue.
I agree, but in fact, we see another. The initial description in a Issue is almost always different from what we need to do in PR. Moreover, in most cases Issues do not contain a final opinion.
+1 for Change Summary.
Non-blocking comment.
|
|
||
| #### Pull request - Change log | ||
|
|
||
| * All PRs must update the [changelog](../CHANGELOG.MD) in your pull request. |
There was a problem hiding this comment.
Based on previous experience I don't like the requirement.
- We reject formatting changes to get git commit that only contain significant changes. With this requirement, we will have trash in git commit. There is now a need to move commits to other branches. There will be more of these jobs over time. It's harder to do with this trash.
- This will slow down the performance of community users and irritate them.
Finally, we can easily automate it. I previously sent @daxian-dbw a sample script. If we'll to keep the PR headers neat, we'll get a neat log of the automatic way.
There was a problem hiding this comment.
We reject changes which contain both formatting and functional changes to the same file. This is not the same thing at all.
Unless we say, maintainers are not part of the community, I don't think we can say this. The PowerShell DSC community and other communities I've participated in have this requirement without issue.
There was a problem hiding this comment.
There was a problem hiding this comment.
Finally, I'm not sure it's easy to keep the PR headers clean and therefore automate it. By header, I assume you mean the message, and the message is often too limited to use for as the changelog. If you mean the notes, since they are unstructured, they suffer other problems.
If the author refuses to do this, the maintainer can always update the changelog for them, but in the DSC community, this was rarely an issue.
There was a problem hiding this comment.
DSC repository has less than 100 PR in the last two years. We have so much in two months. We will actually have a conflict in every PR with this file.
Previously, the team required to update the file and then forget it. Why? Probably because it's not a functional change that everyone forgets and ignores.
I'm sorry, I don't understand the usefulness of this file while there is an open GitHub - any consumer can at any time not only look at the history of changes, but also the history of the code.
This is a secondary element to me. If this element requires so much effort, I immediately think that it should be removed or automated. If we can't use a PR header/description, we could think about using a commit header/description.
There was a problem hiding this comment.
We will actually have a conflict in every PR with this file.
There is an attribute on the file to use a Union merge, which is supposed to prevent merge conflicts. This may cause some issues in the file, but should produce a usable file. Many other projects use this method.
DSC repository has less than 100 PR in the last two years. We have so much in two months.
When you were looking at the DSC Repository, did you count all the submodules? I asked the DSC team and DSC had 43 PRs in November across all the repos.
I'm sorry, I don't understand the usefulness of this file while there is an open GitHub ...
As to the purpose of the file, most people that just want to consume the project would like a simplified way of viewing what has been changed in the project from their perspective. This is not what the PR or the commit log is about and this needs to be reviewed in context to make sure it's being expressed clearly. For example, #2253 suggest we include a link to the changelog in the What's New section of the documentation.
... I immediately think that it should be removed or automated.
Even if it were automated, translating from the contributor focused messaged of the PR to the PowerShell user-focused message in the changelog is what is taking the time. Automating gathering the PR descriptions and links would only solve at best half the problem.
There was a problem hiding this comment.
When you were looking at the DSC Repository, did you count all the submodules? I asked the DSC team and DSC had 43 PRs in November across all the repos.
No, I follow your link. I wonder do they have one changelog for all repos? If no they never have merge conflicts as we had previously with the file.
I'm not the first person who wants this "kill" feature:
https://pypi.python.org/pypi/gitchangelog#changelog
https://www.npmjs.com/package/auto-changelog
https://github.com/skywinder/github-changelog-generator
Also I found this project very interesting (and links there too).
My comments isn't blocking.
There was a problem hiding this comment.
I think this one is the most interesting: https://github.com/skywinder/github-changelog-generator But, it needs a LOT of work to be usable. This issue needs to be fixed, and we would like to be able to group by tags.
This issue needs to be fixed:
github-changelog-generator/github-changelog-generator#493
|
@SteveL-MSFT , @iSazonov said his comments are not blocking. Can you update your feedback? I think we should proceed with trying this and see how it goes. |
SteveL-MSFT
left a comment
SteveL-MSFT
left a comment
There was a problem hiding this comment.
LGTM - Everyone should understand that we are always open to feedback and nothing is set in stone. Based on a team discussion, one of the biggest costs for a release is creating the ChangeLog hence updating this document to push it to the contributor most aware of why the changes are being made and ideally how it's useful to end users.
…PowerShell#5586) - Update contribution guidelines to make updating the changelog required - add a checklist to PR template Comment by @SteveL-MSFT : > Everyone should understand that we are always open to feedback and nothing is set in stone. Based on a team discussion, one of the biggest costs for a release is creating the ChangeLog hence updating this document to push it to the contributor most aware of why the changes are being made and ideally how it's useful to end users.
…#5586) - Update contribution guidelines to make updating the changelog required - add a checklist to PR template Comment by @SteveL-MSFT : > Everyone should understand that we are always open to feedback and nothing is set in stone. Based on a team discussion, one of the biggest costs for a release is creating the ChangeLog hence updating this document to push it to the contributor most aware of why the changes are being made and ideally how it's useful to end users.
4 participants
PR Checklist
Note: Please mark anything not applicable to this PR
NA.[feature]if the change is significant or affectes feature testsPR Summary