Skip to content

Conversation

@joelanford
Copy link
Member

@joelanford joelanford commented Apr 1, 2020

Description of the change:
Add template, examples, and prototype generator program to auto-generate CHANGELOG and migration guide.

Motivation for the change:
Too often, we run into merge conflicts because multiple PRs are updating the CHANGELOG in the same area at the same time.

With this PR (or something like it), we would add separate fragment files for each PR that would be much less likely to conflict. Then we would generate the CHANGELOG and migration guide from these files.

Another benefit of this approach is that we can standardize formatting and policy (e.g. breaking changes require a migration guide section)

Tasks:

  • Sanity check: On every PR, validate every fragment without making filesystem changes
  • Sanity check: Forbid CHANGELOG and migration guide changes if version.Version ends with "+git"?
  • Link check: On every PR, run generator and verify links are valid with latest fragments

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 1, 2020
@jmrodri
Copy link
Member

jmrodri commented Apr 1, 2020

from a users POV, is the idea we copy the template to a new file and that becomes our changelog entry?

@jmrodri
Copy link
Member

jmrodri commented Apr 1, 2020

depending on what has to be done, I'm lazy enough to add a Makefile target: make changelog that will do the boring stuff and print out the file location for me to fill in.

@jmrodri
Copy link
Member

jmrodri commented Apr 1, 2020

depending on what has to be done, I'm lazy enough to add a Makefile target: make changelog that will do the boring stuff and print out the file location for me to fill in. I assume the generate tool is to run in travis right?

@joelanford
Copy link
Member Author

The idea is that we run generate.go which will:

  1. read the fragment files to build a new section of the CHANGELOG and migration guide for the entire release
  2. read in the existing CHANGELOG
  3. Write the new section, followed by the existing section to construct the new CHANGELOG

It would be a similar process for the migration guide, but that isn't in the generate.go program yet.

@joelanford
Copy link
Member Author

Yeah, we would want to integrate it into the release process somehow. As is, it requires a -title flag to be passed, which would generally be the tag we're about to create/release.

So we would have to do TITLE=v0.18.0 make changelog or something.

@joelanford
Copy link
Member Author

Also, from a contributor's point of view, there would no longer be updates directly to CHANGELOG.md. Instead, every PR with a user-facing change would create a new file in /changelog/fragments (based on 00-template.yaml) just for that PR.

This is the change that would help avoid the CHANGELOG.md merge conflicts that we see pretty often. Every time we have to resolve these conflicts requires another run of CI, so this would be a big time-saver for contributors and would help PRs merge faster.

@jmrodri
Copy link
Member

jmrodri commented Apr 1, 2020

@joelanford today we have marker checking all of the docs including the CHANGELOG. Presumably with the fragments this might no longer work. Do we still need that type of functionality? Would that just be another part of the release? Or a longer running job that runs nightly and files github issues with broken links. I prefer this option honestly :)

@joelanford
Copy link
Member Author

joelanford commented Apr 1, 2020

@joelanford today we have marker checking all of the docs including the CHANGELOG. Presumably with the fragments this might no longer work. Do we still need that type of functionality? Would that just be another part of the release? Or a longer running job that runs nightly and files github issues with broken links. I prefer this option honestly :)

I thought about that too. I think as part of the sanity test in every PR, we would re-generate the CHANGELOG and migration guides without consuming the fragments and without committing the regenerated files, and then run marker after that.

That way we can allow markdown in the fragments AND still test that the links are valid.

Comment on lines 153 to 178
for _, e := range c.Entries {
if e.Migration != nil {
haveMigrations = true
bb.WriteString(fmt.Sprintf("### %s\n\n", e.Migration.Header))
bb.WriteString(fmt.Sprintf("%s\n\n", strings.Trim(e.Migration.Body, "\n")))
bb.WriteString(fmt.Sprintf("See %s for more details.\n\n", e.PullRequestLink()))
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Do we want to arrange these under headers based on kind like we do in the CHANGELOG? Or just flatten the migration entries out and list them one-by-one in the order they're read from the files.

On a separate but related note, do we want to do any sorting of CHANGELOG and migration guide entries?

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 do any sorting of the migration guide now? I've just always added stuff at the end anyway. Though we may want to consider putting the migration guide into version specific upgrade guides when we build, since I think it's starting to get a bit unwieldy. Might be out of scope although if we do want to go that way may be better to do it now than have to rework the tooling later

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we do any sorting of the migration guide now?

No, but depending on how we name the fragment files, entries will show up semi-randomly, just based on a lexicographical sort of the filenames.

Though we may want to consider putting the migration guide into version specific upgrade guides when we build, since I think it's starting to get a bit unwieldy.

Yeah I had the same thought. Scrolling to the bottom of a very long migration guide is not the best UX. We could at least reverse the order so that new migration steps are always at the top.

Copy link
Member

Choose a reason for hiding this comment

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

yeah, but if you're two or three versions behind then you need to go to the middle of the document somewhere to find your upgrade instructions, which I think is still just a worse experience than seeing migration_guide-v0.16.md or whatever naming scheme we'd use

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm good with this change. And it actually makes the generation simpler since there's no need to deal with an existing file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Migration guides will now be generated in a separate file for each release. In a follow-up, we can split the existing migration guide into separate files per release.

joelanford added a commit to joelanford/operator-sdk that referenced this pull request Apr 2, 2020
@joelanford joelanford force-pushed the autogen-changelog branch 2 times, most recently from aaaf7fd to c4ad017 Compare April 2, 2020 22:51
joelanford added a commit to joelanford/operator-sdk that referenced this pull request Apr 6, 2020
@joelanford joelanford changed the title WIP: Auto-generating the CHANGELOG and Migration Guide Auto-generating the CHANGELOG and Migration Guide Apr 6, 2020
@joelanford joelanford changed the title Auto-generating the CHANGELOG and Migration Guide [WIP] Auto-generating the CHANGELOG and Migration Guide Apr 7, 2020
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2020
@joelanford joelanford marked this pull request as ready for review April 8, 2020 20:14
@joelanford joelanford changed the title [WIP] Auto-generating the CHANGELOG and Migration Guide Auto-generating the CHANGELOG and Migration Guide Apr 8, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 8, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2020
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

A few nits, otherwise LGTM.

Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2020
@openshift-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 14, 2020
Copy link
Member

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

no additional comments from me. Looking forward to this.

@joelanford joelanford merged commit 19d0e1e into operator-framework:master Apr 16, 2020
@joelanford joelanford deleted the autogen-changelog branch April 16, 2020 01:55
@joelanford
Copy link
Member Author

/cherry-pick v0.17.x

@openshift-cherrypick-robot

@joelanford: new pull request created: #2849

Details

In response to this:

/cherry-pick v0.17.x

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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.

8 participants