Skip to content
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

Create a system to enforce that PR authors are responsible for updating the changelog #5670

Closed
lucaswoj opened this issue Nov 13, 2017 · 16 comments

Comments

@lucaswoj
Copy link
Contributor

The pace of development and complexity of GL JS makes it time consuming and error prone for the engineer doing the release to write the whole change log by hand at release time.

We could consider following the lead of other teams at Mapbox and require that PR authors update the change log if their changes affect end users or explicitly opt out from updating the change log if the changes do not affect end users.

@andrewharvey
Copy link
Collaborator

andrewharvey commented Nov 14, 2017

Do you mean that each PR would include an entry in the CHANGELOG under a master heading (if worthy enough to have an entry) and then at release time the master heading is changed over to the version?

I'm 👍 for this. I've always preferred to update the CHANGELOG as part of each PR, but for my PR's I haven't simply because it wasn't being done that way.

Added bonus is people can see what changes are coming soon in the next release.

@jfirebaugh
Copy link
Contributor

Downside of this is it triggers a merge conflict in almost every PR.

@mollymerp
Copy link
Contributor

there are a fair number of PRs that would not warrant an update in the changelog (changes to testing patterns or internal refactors that don't affect end users). I'd prefer a checkbox on the PR template to a hard requirement 🤷‍♀️

@anandthakker
Copy link
Contributor

The underlying problem here is that manually building the changelog from the git history each time we do a release can be quite difficult and time consuming.

An alternative solution to requiring a changelog update in each PR might be to use some sort of system for commit comments, a la https://commitizen.github.io/cz-cli/, to make it easier to build the changelog.

@lucaswoj lucaswoj changed the title Create a test to ensure that every PR updates the changelog Create a system to enforce that PR authors are responsible for updating the changelog Nov 14, 2017
@lucaswoj
Copy link
Contributor Author

I'm loving all these ideas! Just edited the issue title to increase the design space a little.

@1ec5
Copy link
Contributor

1ec5 commented Nov 14, 2017

Good release notes require some manual curation, whether it’s at release time or periodically during the release cycle. Here are some examples I’ve seen from gardening the iOS SDK’s release notes:

  • Developers reading the release notes don’t need to know about fixes for regressions that were introduced within the same release cycle.
  • Breaking changes should be highlighted and ideally listed up-front.
  • Changes should be sorted in order of developer impact.
  • Long release notes can be broken up by topic area (each topic sorted by developer impact) to make the notes more scannable. Sometimes the topics aren’t known in advance. Dividing the notes by topic can also reduce the occurrence of merge conflicts.
  • Whereas the commit message needs to summarize the code-level changes (“added missing calls to foo()”), the developer impact may be distinct (“fixed bug preventing redraws when pressing bar”).

Commitzen can certainly make it easier to perform that curation, but the biggest improvement would come from treating CHANGELOG.md as a draft release notes and curating it multiple times during a release cycle, while the context is still fresh.

@1ec5
Copy link
Contributor

1ec5 commented Nov 14, 2017

As for a system of enforcement, I’d recommend Danger, which works by commenting on the PR instead of updating webhooks. Danger can leave a reminder as a warning without failing the build.

@jfirebaugh
Copy link
Contributor

jfirebaugh commented May 4, 2018

@anandthakker
Copy link
Contributor

anandthakker commented May 4, 2018

Considering the merge conflict issue @jfirebaugh mentions above, and @1ec5 's point about manual curation, I propose that we attempt to streamline--but not fully automate--this process by doing the following:

  1. Create a script that reads the git log since the last release and transforms it into a list of PR/issue entries like: [Number and title of PR] [github link] [list of associated issues, scraped from ‘Fixes #…’ PR / commit comments. With this list serving as a rough draft of the changelog entry for a release, we'd eliminate the tedium of chasing down PRs and issues for each commit and instead be able to focus on the part that's actually important: communicating clearly the changes in this release.

  2. Add a Danger bot that helps us to enforce the standard that every PR description contains either: Changelog: n/a (reason) or Changelog: breaking|feature|fix [description of change, if different from PR title]

1 is more important than 2, but I think 2 is a good idea because it essentially asks the the author of a change to handle thinking about how best to communicate it, thus avoiding the situation where the person running the release doesn't have enough context to do so and has to chase down the author to figure out what to say.

@1ec5
Copy link
Contributor

1ec5 commented May 4, 2018

Edit: I just realized this ticket is in the gl-js repository rather than the gl-native repository. Never mind me. 😅

Manual curation is one of the benefits of the changelog workflow that the iOS team practices (and tries to help the core team practice). The other benefit is having a running changelog in one place in source control. If we rely on the script to aggregate these release notes, then either we need a page on mapbox.github.io that gets updated regularly with that script’s output, or we need to do a manual weekly roundup that checks in these notes. Ultimately, the release notes do need to work their way into CHANGELOG.md, since that’s what we use to generate the cover page of the jazzy docset.

If you haven’t already, please take a look at the iOS and macOS release note template and the iOS changelog. Note how we categorize the changes by topic and sort them by developer or user impact rather than chronologically – that’s been our approach to cutting down on merge conflicts.

These release notes apply much of Mapbox’s documentation style guide, including the use of complete sentences. Hopefully we can maintain that high quality standard as we streamline the workflow.

@anandthakker
Copy link
Contributor

The other benefit is having a running changelog in one place in source control.

@1ec5 my intent in #5670 (comment) was that we'd continue to track CHANGELOG.md in source control and update it at the time of each release (usually in the vX.Y.Z commit). The script output would simply be used to give the person making that update a head start in the manual curation work.

or we need to do a manual weekly roundup that checks in these notes

Why weekly, rather than just with each release?

Note how we categorize the changes by topic and sort them by developer or user impact rather than chronologically – that’s been our approach to cutting down on merge conflicts

👍 to categorization and sorting by impact. Good point about cutting down merge conflicts -- but there's still likely to be conflict on the 2nd and 3rd change in each category, right?

@1ec5
Copy link
Contributor

1ec5 commented May 4, 2018

I posted #5670 (comment) before realizing this isn’t the gl-native repository, where we already maintain a running changelog on each merge but sometimes miss stuff that happens in mbgl. Sorry for the confusion.

@anandthakker
Copy link
Contributor

Quick first cut at this changelog-draft script here: https://github.com/mapbox/github-release-tools

Here's an example of what it currently generates: https://gist.github.com/anandthakker/a913b632c719f57a4bf6e8356943f868

@andrewharvey
Copy link
Collaborator

Quick first cut at this changelog-draft script here: https://github.com/mapbox/github-release-tools

Looks good.

I noticed that the hat tipping logic is based on the PR coming from a fork, I'm not sure how many external contributors you have with commit access, but right now it won't automatically hat tip their PRs. It's not an issue for my contributions, but something to keep in mind for others...

@andrewharvey
Copy link
Collaborator

#9023 proposes an option to tag the changeset category with a github label and use the PR title or <changelog> as the changelog entry

@andrewharvey
Copy link
Collaborator

I'll close this issue since #9023 is in place allowing PR authors to set the changelog entry and label for category (which I believe then get's auto populated into the changelog at release time).

If this still doesn't address the request, feel free to re-open.

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

No branches or pull requests

6 participants