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

Adopt go-changelog in Nomad #10825

Merged
merged 6 commits into from
Jul 6, 2021
Merged

Adopt go-changelog in Nomad #10825

merged 6 commits into from
Jul 6, 2021

Conversation

notnoop
Copy link
Contributor

@notnoop notnoop commented Jun 28, 2021

Adopts go-changelog for managing Nomad's changelog. go-changelog is becoming the HashiCorp defacto standard tool for managing changelog, e.g. Consul, Vault, Waypoint. Consul 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.

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 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.

@notnoop notnoop self-assigned this Jun 28, 2021
@@ -0,0 +1,132 @@
# How To Use
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in the contributing guide rather than being buried in a potentially-hidden folder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was conflicted about this and opted not to add it there. I can add a link from contributing/README.md to this document if that helps?

I think the details belong to a separate file from contributing/README.md, as it is missing much of the ceremony and conventions around PR management (e.g. changelog, labels, branch names).

I'm uncertain if the file is more discoverable here or in contributing directory. Though, I think it's more noticeable if someone navigates to this directory unexpectedly - seeing a long list of numbered txt files without README is jarring a bit.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, let's add a link from contributing/README.md then.

.changelog/README.md Outdated Show resolved Hide resolved
### Improvement

~~~
```release-note:improvement
Copy link
Member

Choose a reason for hiding this comment

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

The syntax here is confusing to me... are the .txt files supposed to have Markdown syntax? If so, we should say that explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

go-changelog just happened to use this markdown-like syntax but the tool doesn't expect or require markdown: https://github.com/hashicorp/go-changelog/blob/89e3f9b779494b52e013b9615f1a7d8937fca0a8/note.go#L15-L20 .

Copy link
Member

Choose a reason for hiding this comment

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

Wild. 😁 Maybe we can call out what we'd like folks to do for consistency anyways?

```
changelog-build -last-release v1.1.2 -this-release HEAD \
-entries-dir . -changelog-template changelog.tmpl -note-template note.tmpl
```
Copy link
Member

Choose a reason for hiding this comment

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

This needs more constraints on when and where you're supposed to run this. As far as I can tell HEAD should be the release branch for the release, not main. Can we describe how this works for backports too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - though, I'd say the detailed commands should belong in the nomad-releases scripts rather than the documentation here.

But even in backport, running make changelog on the backport branch should generate the backport. I've meant this line to highlight the full command options, not to guide how to use it. Maybe I can remove it from here if that causes confusion.

Copy link
Member

Choose a reason for hiding this comment

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

Yes - though, I'd say the detailed commands should belong in the nomad-releases scripts rather than the documentation here.

That totally makes sense, you're right.

@@ -25,6 +25,10 @@ endif
# tag corresponding to latest release we maintain backward compatibility with
PROTO_COMPARE_TAG ?= v1.0.3$(if $(findstring ent,$(GO_TAGS)),+ent,)

# LAST_RELEASE is the git sha of the latest release corresponding to this branch. main should have the latest
# published release, but backport branches should point to the parent tag (e.g. 1.0.8 in release-1.0.9 after 1.1.0 is cut).
LAST_RELEASE ?= v1.1.2
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to update the release process for this too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! We can have nomad-releases update the value "automatically" too.


~~~
```release-note:improvement
internal/server: Add new option for configs
Copy link
Member

Choose a reason for hiding this comment

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

Double checking - these lines shouldn't include PR/Issue #'s & links? (which will be derived from the filename automatically instead?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. PR link is automatic.

@@ -0,0 +1,56 @@
{{- if index .NotesByType "breaking-change" -}}
BREAKING CHANGES:
Copy link
Member

Choose a reason for hiding this comment

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

Is this the correct order? In the past we've always placed Features at the top, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reprioritized features to be on top. Other HashiCorp products have backward incompatibility changes above - but their features seem to be less significant than how we frame them in Nomad and seem more like improvements to me, e.g. https://github.com/hashicorp/consul/blob/master/CHANGELOG.md#1100-beta3-may-27-2021. Updated it to reflect Nomad practices and we can change as we want.

@notnoop
Copy link
Contributor Author

notnoop commented Jul 6, 2021

I've rebased against the latest main. Also, I've moved the changelog documentation to contributing dir - it having sample snippets was messing up with go-changelog processing.

Ready for review again.

Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

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

LGTM; the version would be nice but certainly not a blocker

@@ -109,6 +112,7 @@ deps: ## Install build and development dependencies
go install github.com/golang/protobuf/protoc-gen-go@v1.3.4
go install github.com/hashicorp/go-msgpack/codec/codecgen@v1.1.5
go install github.com/bufbuild/buf/cmd/buf@v0.36.0
go install github.com/hashicorp/go-changelog/cmd/changelog-build@latest
Copy link
Member

Choose a reason for hiding this comment

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

Ideally https://github.com/hashicorp/go-changelog/ should be tagged with a version we can pin to

Copy link
Contributor Author

@notnoop notnoop Jul 6, 2021

Choose a reason for hiding this comment

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

Agree. Will follow up. I'm also working to optimize the code a bit and we can cut a release tag afterwards.

@@ -0,0 +1,140 @@
Nomad Codebase Documentation
Copy link
Member

Choose a reason for hiding this comment

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

Is the README.md-e name some kind of workaround?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops - that's due to an sed invocation :(

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants