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

Introducing go-changelog #8387

Merged
merged 3 commits into from
Aug 6, 2020
Merged

Introducing go-changelog #8387

merged 3 commits into from
Aug 6, 2020

Conversation

hanshasselberg
Copy link
Member

@hanshasselberg hanshasselberg commented Jul 28, 2020

This PR introduces go-changelog for consul, with the goal to automatically generate our changelog in the future. This PR is more of an introduction though, it adds entries for the currently unreleased PRs and showcases how to generate the changelog for them.

There are two notable things:

  1. the templates for individual entries and the changelog as a whole: changelog.tmpl and note.tmpl
  2. a file for each PR, containing the changelog entry

When running changelog-gen, it looks like this:

$ changelog-build -last-release v1.8.0 -entries-dir .changelog/ -changelog-template changelog.tmpl -note-template note.tmpl -this-release 86b6b38faa7c69f26f1d4c71e271cd4285daadf9
FEATURES:

* acl: Added ACL Node Identities for easier creation of Consul Agent tokens. [[GH-7970](https://github.com/hashicorp/consul/pull/7970)]

IMPROVEMENTS:

* acl: allow auth methods created in the primary datacenter to optionally create global tokens. [[GH-7899](https://github.com/hashicorp/consul/pull/7899)]
* agent: Allow to restrict servers that can join a given Serf Consul cluster. [[GH-7628](https://github.com/hashicorp/consul/pull/7628)]
* connect: Append port number to expected ingress hosts. [[GH-8190](https://github.com/hashicorp/consul/pull/8190)]
* connect: support Envoy v1.14.4, v1.13.4, v1.12.6. [[GH-8216](https://github.com/hashicorp/consul/pull/8216)]
* connect: various changes to make namespaces for intentions work more like for other subsystems. [[GH-8194](https://github.com/hashicorp/consul/pull/8194)]
* dns: Improve RCODE of response when query targets a non-existent datacenter. [[GH-8102](https://github.com/hashicorp/consul/issues/8102)] [[GH-8218](https://github.com/hashicorp/consul/pull/8218)]
* version: The `version` CLI subcommand was altered to always show the git revision the binary was built from on the second line of output. Additionally the command gained a `-format` flag with the option now of outputting the version information in JSON form. **NOTE** This change has the potential to break any parsing done by users of the `version` commands output. In many cases nothing will need to be done but it is possible depending on how the output is parsed. [[GH-8268](https://github.com/hashicorp/consul/pull/8268)]

BUG FIXES:

* agent: Fixed a bug where Consul could crash when `verify_outgoing` was set to true but no client certificate was used. [[GH-8211](https://github.com/hashicorp/consul/pull/8211)]
* auto_encrypt: Fixed an issue that caused auto encrypt certificates to not be updated properly if the agents token was changed and the old token was deleted. [[GH-8311](https://github.com/hashicorp/consul/pull/8311)]
* auto_encrypt: Fixed an issue where auto encrypt certificate signing wasn't using the connect signing rate limiter. [[GH-8211](https://github.com/hashicorp/consul/pull/8211)]
* auto_encrypt: Fixed several issues around retrieving the first TLS certificate where it would have the wrong CN and SANs. This was being masked by a second bug (also fixed) causing that certificate to immediately be discarded with a second certificate request being made afterwards. [[GH-8211](https://github.com/hashicorp/consul/pull/8211)]
* connect: Fixed issue where specifying a prometheus bind address would cause ingress gateways to fail to start up. [[GH-8371](https://github.com/hashicorp/consul/pull/8371)]
* connect: fix crash that would result if a mesh or terminating gateway's upstream has a hostname as an address and no healthy service instances available. [[GH-8158](https://github.com/hashicorp/consul/pull/8158)]
* gossip: Avoid issue where two unique leave events for the same node could lead to infinite rebroadcast storms. [[GH-8343](https://github.com/hashicorp/consul/pull/8343)]
* xds: version sniff envoy and switch regular expressions from 'regex' to 'safe_regex' on newer envoy versions. [[GH-8222](https://github.com/hashicorp/consul/pull/8222)]

The output is pretty much identical to the one we currently have in our changelog. I added a couple of dots at the end and the link to the PR references pulls instead of issues, but they are pointing to the same things. Note how everything is sorted properly and the links at the end are generated for you.

@@ -0,0 +1,3 @@
```release-note:improvement
Copy link
Member Author

Choose a reason for hiding this comment

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

This is how you create an improvement

@@ -0,0 +1,3 @@
```release-note:feature
Copy link
Member Author

Choose a reason for hiding this comment

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

This is how you create a feature.

@@ -0,0 +1,3 @@
```release-note:bug
Copy link
Member Author

Choose a reason for hiding this comment

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

This is how you create a bug.


```release-note:bug
auto_encrypt: Fixed several issues around retrieving the first TLS certificate where it would have the wrong CN and SANs. This was being masked by a second bug (also fixed) causing that certificate to immediately be discarded with a second certificate request being made afterwards.
```
Copy link
Member Author

Choose a reason for hiding this comment

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

This is how you create multiple entries for a single PR.

Copy link
Member Author

@hanshasselberg hanshasselberg Jul 28, 2020

Choose a reason for hiding this comment

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

You have to look at the file to see the other entries.

@@ -0,0 +1,3 @@
```release-note:improvement
version: The `version` CLI subcommand was altered to always show the git revision the binary was built from on the second line of output. Additionally the command gained a `-format` flag with the option now of outputting the version information in JSON form. **NOTE** This change has the potential to break any parsing done by users of the `version` commands output. In many cases nothing will need to be done but it is possible depending on how the output is parsed.
```
Copy link
Member Author

Choose a reason for hiding this comment

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

Long description with markdown formatting.

Copy link
Contributor

@mikemorris mikemorris left a comment

Choose a reason for hiding this comment

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

This looks great! Is actually running changelog-gen something that should happen manually or could it eventually be integrated into CI? Should there be an (optional?) CI check/notice for validating that a PR adds a changelog entry file?

@mkeeler
Copy link
Member

mkeeler commented Jul 28, 2020

@mikemorris I don't think we can have a required check for the changelog entry for every PR for a couple of reasons.

  1. PRs from community members might not have the changelog entry and we want to be able to provide one post-merge for those.
  2. Many PRs don't require changelog entries: docs updates, refactoring etc.

As for a check being optional, it would make many PRs including most community PRs appear to be merged when failing checks.

@i0rek Does this mean that once this PR is merged we should start adding changelog entries in files?

@hanshasselberg
Copy link
Member Author

@mikemorris I think that running changelog-gen could be done by CI when we are comfortable with that. In terms of enforcing an entry, I am not sure how to do that best. I think it might be easier if we don't do it because of the reasons @mkeeler mentioned. We could however render the entry if it exists - that would help with the creation - I typoed them many times myself and they ended up not being included.

@mkeeler yes, as soon as this merges we can start with the files. I am starting to coordinate with the team thats doing the next releases because it would be on them to incorporate changelog-gen.

Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Nice! I think this is going to be great

changelog.tmpl Outdated Show resolved Hide resolved
@paddycarver
Copy link

In terms of enforcing an entry, I am not sure how to do that best. I think it might be easier if we don't do it because of the reasons @mkeeler mentioned.

Just to chime in here: for the other project that uses this, we use https://github.com/hashicorp/go-changelog/tree/a22ca465bf7706bf125eac0a4ec4f3f7aed2fa15/cmd/changelog-pr-body-check to ensure every PR either has a changelog or explicitly says there isn't a changelog. YMMV with community interaction stuff, just wanted to note that changelog: none worked reasonably well for us for things like docs/website/internal changes that don't require a changelog entry.

By requiring an explicit opt-out, we could identify things that didn't have a changelog entry on accident, which was helpful for making sure things didn't accidentally get left out.

Our contribution process was different in meaningful ways, however, so take all this with a grain of salt and in the spirit of breadcrumbs from previous travellers. :)

@dnephin
Copy link
Contributor

dnephin commented Jul 29, 2020

I like that idea for catching missing changelogs. Maybe we could use a github label (changelog-none-required) instead of the none file? I think that would make it easier for us to add to community contributions, and even for our own PRs that we know are incremental pieces and not changelog-ready yet.

@paddycarver
Copy link

We did that, but found that a none file was an easier workflow for us for reasons involving our contributing process. YMMV. :)

@hanshasselberg
Copy link
Member Author

Since we are already doing things with labels, I think we should try that! @alvin-huang Could we add a check that checks if a file was added in .changelog or changelog-none-required label is present?

@hanshasselberg hanshasselberg merged commit 586ee25 into master Aug 6, 2020
@hanshasselberg hanshasselberg deleted the changelog_automation branch August 6, 2020 21:15
hanshasselberg added a commit that referenced this pull request Aug 7, 2020
* add templates for changelog-gen
* add entry files for currently unreleased PRs on master
@hanshasselberg hanshasselberg changed the title Introducing changelog-gen Introducing go-changelog Sep 10, 2020
mikemorris pushed a commit that referenced this pull request Sep 10, 2020
* add templates for changelog-gen
* add entry files for currently unreleased PRs on master
mikemorris pushed a commit that referenced this pull request Sep 10, 2020
* add templates for changelog-gen
* add entry files for currently unreleased PRs on master
notnoop pushed a commit to hashicorp/nomad that referenced this pull request Jul 6, 2021
Adopts [`go-changelog`](https://github.com/hashicorp/go-changelog) for managing Nomad's changelog. `go-changelog` is becoming the HashiCorp defacto standard tool for managing changelog, e.g. [Consul](hashicorp/consul#8387), [Vault](hashicorp/vault#10363), [Waypoint](hashicorp/waypoint#1179). [Consul](hashicorp/consul#8387) seems to be the first product to adopt it, and its PR has the most context - though I've updated `.changelog/README.md` with the relevant info here.

## Changes to developers workflow

When opening PRs, developers should add a changelog entry in `.changelog/<PR#>.txt`. Check [`.changelog/README.md`](https://github.com/hashicorp/nomad/blob/docs-adopt-gochangelog/.changelog/README.md#developer-guide). 

For the WIP release, entries can be amended even after the PR merged, and new files may be added post-hoc (e.g. during transition period, missed accidentally, community PRs, etc).

### Transitioning

Pending PRs can start including the changelog entry files immediately.

For 1.1.3/1.0.9 cycle, the release coordinator should create the entries for any PR that gets merged without a changelog entry file. They should also move any 1.1.3 entry in CHANGELOG.md to a changelog entry file, as this PR done for GH-10818.

## Changes to release process

Before cutting a release, release coordinator should update the changelog by inserting the output of `make changelog` to CHANGELOG.md with appropriate headers. See [`.changelog/README.md`](https://github.com/hashicorp/nomad/blob/docs-adopt-gochangelog/.changelog/README.md#how-to-generate-changelog-entries-for-release) for more details.


## Details

go-changelog is a basic templating engine for maintaining changelog in HashiCorp environment.

It expects the changelog entries as files indexed by their PR number. The CLI generates the changelog section for a release by comparing two git references (e.g. `HEAD` and the latest release, e.g. `v1.1.2`), and still requires manual process for updating CHANGELOG.md and final formatting.

The approach has many nice advantages:
* Avoids changelog related merge conflicts: Each PR touches different file!
* Copes with amendments and post-PR updates: Just add or update a changelog entry file using the original PR numbers.
* Addresses the release backporting scenario: Cherry-picking PRs will cherry-pick the relevant changelog entry automatically!
* Only relies on data available through `git` - no reliance on GitHub metadata or require GitHub credentials

The approach has few downsides though:
* CHANGELOG.md going stale during development and must be updated manually before cutting the release
  * Repository watchers can no longer glance at the CHANGELOG.md to see upcoming changes
  * We can periodically update the file, but `go-changelog` tool does not aid with that
* `go-changelog` tool does not offer good error reporting. If an entry is has an invalid tag (e.g. uses `release-note:bugfix` instead of `release-note:bug`), the entry will be dropped silently
  * We should update go-changelog to warn against unexpected entry tags
  * TODO: Meanwhile, PR reviewers and release coordinators should watch out

## Potential follow ups

We should follow up with CI checks to ensure PR changes include a warning. I've opted not to include that now. We still make many non-changelog-worth PRs for website/docs, for large features that get merged in multiple small PRs. I did not want to include a check that fails often.

Also, we should follow up to have `go-changelog` emit better warnings on unexpected tag.
Copy link

@luci237 luci237 left a comment

Choose a reason for hiding this comment

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

38Dpj2d6wuLunwdvJQzg8D56j8qQyu7XJR

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.

None yet

6 participants