Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
# @TravisEz13 @PaulHigin

# Area: Documentation
# @joeyaiello @TravisEz13
.github/ @joeyaiello @TravisEz13

# Area: Test
# @JamesWTruher @TravisEz13 @adityapatwardhan
Expand Down
15 changes: 8 additions & 7 deletions .github/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -129,19 +129,20 @@ Additional references:
If the changes are related to an existing GitHub issue,
please reference the issue in PR description (e.g. ```Fix #11```).
See [this][closing-via-message] for more details.
* If the change warrants a note in the [changelog](../CHANGELOG.MD)
either update the changelog in your pull request or
add a comment in the PR description saying that the change may warrant a note in the changelog.
New changes always go into the **Unreleased** section.

#### Pull request - Change log

* All PRs must update the [changelog](../CHANGELOG.MD) in your pull request.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@TravisEz13 TravisEz13 Dec 1, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@iSazonov iSazonov Dec 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@TravisEz13 TravisEz13 Dec 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tracking Issue #5614

New changes always go into the **Unreleased** section at the top of the changelog.
Keeping the changelog up-to-date simplifies the release process for Maintainers.
An example (with an associated PR #):

```markdown
Unreleased
----------
## Unreleased

* `Update-Item` now supports `-FriendlyName` (#1234).
* `Update-Item` now supports `-FriendlyName` (#1234, @ExampleUser).
```
Note: Please add `**Breaking Change**` to the front of the entry in the changelog if the change is [breaking.](#making-breaking-changes)

* Please use the present tense and imperative mood when describing your changes:
* Instead of "Adding support for Windows Server 2012 R2", write "Add support for Windows Server 2012 R2".
Expand Down
18 changes: 12 additions & 6 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
<!--
## PR Checklist
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


If you are a PowerShell Team member, please make sure you choose the Reviewer(s) and Assignee for your PR.
If you are not from the PowerShell Team, you can leave the fields blank and the Maintainers will choose them for you. If you are familiar with the team, feel free to mention some Reviewers yourself.
Note: Please mark anything not applicable to this PR `NA`.
- [ ] [PR has a meaningful title](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#pull-request---submission)
- [ ] 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)
- [ ] Issue filed - Issue link:
- [ ] [Change is not breaking](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#making-breaking-changes)
- [ ] [Make sure you've added a new test, if existing tests do not effectively test the code changed](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md#before-submitting)
- [ ] [Add `[feature]` if the change is significant or affectes feature tests](https://github.com/PowerShell/PowerShell/blob/master/docs/testing-guidelines/testing-guidelines.md#requesting-additional-tests-for-a-pr)

For more information about the roles of Reviewer and Assignee, refer to [CONTRIBUTING.md](https://github.com/PowerShell/PowerShell/blob/master/.github/CONTRIBUTING.md).

-->
## PR Summary
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog

## Unreleased

- Update the contribution guideline to note that updating the changelog is required. (#5586)

## v6.0.0-rc - 2017-11-16

### Breaking changes
Expand Down