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

New release notes generator tasks #71125

Merged
merged 31 commits into from
Jul 28, 2021

Conversation

pugnascotia
Copy link
Contributor

@pugnascotia pugnascotia commented Mar 31, 2021

Continuation of #68463, but targeting master instead. Part of #67335.

This PR adds tasks for generating release notes, using information stored in files in the repository:

  • generateReleaseNotes - generates new release notes, release highlights and breaking changes
  • validateChangelogs - validates that all the changelog YAML files are well-formed (confirm to schema, have required fields depending on the type value)

Notes:

  • I changed Version to allow a v prefix in relaxed mode
  • I generated some YAML files using a script (not included) that pulls data from GitHub. I'll remove these files before merging, but they allow the tasks to be run.

@pugnascotia pugnascotia added >feature :Delivery/Build Build or test infrastructure v8.0.0 labels Mar 31, 2021
@pugnascotia pugnascotia requested a review from mark-vieira March 31, 2021 14:53
@elasticmachine elasticmachine added the Team:Delivery Meta label for Delivery team label Mar 31, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-delivery (Team:Delivery)

@mark-vieira
Copy link
Contributor

Sorry, I haven't had a chance to look at this in great detail yet but what do we intend to do with the stuff in elasticsearch-infra/release-tools? Is there a reason we've baked this into the actual build vs putting it in the release-tools project?

@geekpete
Copy link
Member

geekpete commented Apr 2, 2021

Will this have the option to also output to machine readable formats (eg. json) for indexing for searchable/filterable release notes?

Copy link
Contributor

@mark-vieira mark-vieira left a comment

Choose a reason for hiding this comment

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

Grrr, I reviewed this yesterday but just didn't "submit" the comments. Sorry Rory.

},
"type": {
"type": "string",
"enum": [
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible a single PR might be multiple things? For example, introduce an enhacement/feature and deprecations or breaking changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not from the point-of-view of the docs, no.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have nearly 200 pull requests with both the enhancement and breaking labels. How would we deal with them? It doesn't seem right to make this mutually exclusive. Certainly we should support an enhancement PR that included breaking changes as part of this. We should consider these things unique axis I think. Perhaps @elastic/docs might have some insight here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider these things unique axis I think.

This feels right to me. The current manual process does not treat enhancement, release highlight, or breaking labels as mutually exclusive or even necessarily related.

@debadair has had more recent experience with cleaning up our release docs. I'll defer to her call on what the future state should look like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Strictly speaking, what happens at the moment is that any PR with multiple > labels has to be manually categorised by whoever is preparing the release notes. By "release notes", I mean like the list of changes for 7.12.0, where each change appears under a single heading, and "Breaking changes" and "Enhancements" are separate sections. That's why I said that from a docs point-of-view, you can only have a single type of change.

We could modify the logic, e.g. if you have a PR with both the >breaking and >enhancement labels, then you could file the PR under the "Enhancement" section in the release notes but also require the extra section in the yaml file for the breaking information. However, that would be inconsistent with a PR that is only labelled >breaking.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should support this. That is, for a PR with both labels, it would be listed twice in the changelog, once in each section, and with a unique description specific to the type of change.

This makes sense to me. That will also help users who are quickly scanning the RNs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jrodewig We actually talked this through in the Delivery team sync last week and I didn't update the thread here, apologies. In the meeting, I pointed out that the release documentation already has a place for information about breaking changes or deprecated functionality, separate to the release notes (list of PRs).

Regarding PRs labelled with both e.g. >enhancement and >breaking, the current process requires the release point person to decide which section each PR should go under, so although we have PRs in GitHub that are labelled this way, to date we've never included them in multiple sections in the release notes. Note that this has resulted in some interesting inconsistencies - #65584 was listed as an enhancement in the list of PRs for release 7.11.0, but had a mention as a breaking change in the migration doc.

In my opinion, this makes sense - an enhancement should be an incremental improvement in some existing functionality. For an enhancement to also be a breaking change seems to me to be an oxymoron.

So with the current code, there's nothing to stop a contributor labelling their PR as >enhancement, but still including breaking or deprecation information as well.

@mark-vieira does that agree with your understanding?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, this makes sense - an enhancement should be an incremental improvement in some existing functionality. For an enhancement to also be a breaking change seems to me to be an oxymoron.

That makes sense. It seems like we'd want to clarify this with the team and contributors. Adding a check to ensure a PR is not labelled as both an enhancement and breaking change would help enforce that.

Thanks for for explaining!

Copy link
Contributor

Choose a reason for hiding this comment

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

Diving into this late, but if I'm following this correctly, the Migration guide is based on things marked breaking in the changelog. So an enhancement (or even a bug fix) that is a breaking change is not included in either the breaking changes section in the RNs OR the Migration guide? Having inconsistent lists of breaking changes has caused confusion and pain in the past, but having consistent but incomplete lists doesn't feel like the answer.

We don't make breaking changes just for the heck of it, every one is a fix, an enhancement, or a new and better way of doing something. The breaking changes are really more of a filter on the changelog--which of the bug fixes, enhancements, and new features could require a user to update their configuration or code?

Which gets to @geekpete's point about searching and filtering the entries. We want to be able to show users what has changed between the version they are running and the version they want to upgrade to, and then filter that down to just those changes that might require action on their part. And, like we tried to do with the "notable changes", highlight those that are most likely to require action.

Rather than not allowing enhancement PRs to be tagged as breaking, it feels like we just want to ignore the breaking tag for the purposes of building the release notes, and just use it to generate the content for the migration guide. Which probably means the changelog entries for breaking changes need to an additional piece that provides the "Impact" info that tells users what they need to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@debadair Here's an annotated example:

pr: 69149
issues:
  - 55820
area: Packaging
# In the current scheme, this can only have one value. Breaking changes would always
# be `breaking` or `breaking-java`. 
type: breaking
summary: Remove support for `JAVA_HOME`
versions:
  - v8.0.0

# Supplement sections can also be added. For example:
breaking:
  # Controls whether this entry is included in the `//tag::notable-breaking-changes[]` region
  # in `highlights.asciidoc`. Possible the field name here is misleading.
  notable: false
  title: Remove support for `JAVA_HOME`
  area: Packaging

  # Required
  details: |-
    {es} no longer supports the `JAVA_HOME` environment variable. Note
    that {es} does not treat `JAVA_HOME` being set as a failure, as it is
    perfectly reasonable to have this configured at a system level.

  # Currently required, but I note that this isn't always present in our published
  # release notes where the `details` is short. We could make it optional.
  impact: |-
    If you rely on the `JAVA_HOME` environment variable to configure the
    JDK for {es}, you now need to use the `ES_JAVA_HOME` environment
    variable instead.

Where we generate a "starter" YAML for a PR in changes to our HOMER bot, we add a highlight: section for PRs labelled release highlight, a breaking: section for PRs labelled >breaking or >breaking-java, and a deprecation: section for PRs labelled >deprecation.

We don't yet enforce that e.g. breaking PRs have a breaking: section, but we probably should. Conversely, I wasn't planning on prohibiting breaking: or deprecation: sections where the type: is other than breaking or deprecation, leaving that up to a PR author's discretion.

Provided all the PRs in a release provide these YAML files and the contents are good, then we can generate the release documents under docs/reference/release-notes/ and docs/reference/migration (or at least a first pass that is 95% of the way there).

The restriction in this PR on having a single type value comes from our existing release notes process. Right now, any PR with multiple type labels (e.g. >breaking and >enhancement) has to be placed under a single heading by the release point person. They could pick === Breaking changes or they could pick === Enhancements. I wanted to push that decision to the PR author, not the release point person.

We could allow multiple values for type:, but to me that implies a change in the list of PRs page, and I was aiming to automate the process without significantly changing the output.

Note that there's nothing here to stop someone using multiple type labels on a PR, they'll just need to pick one in the changelog YAML.

@mark-vieira
Copy link
Contributor

Will this have the option to also output to machine readable formats (eg. json) for indexing for searchable/filterable release notes?

Is this a capability we have now or is this just something aspirational?

@geekpete
Copy link
Member

geekpete commented Apr 2, 2021

Aspirational, a nice to have.
I have a POC tool that scrapes the asciidoc release notes file and chops it into individual release note items making them searchable/filterable by category/type/etc, it's pretty ugly with lots of exception handling for malformed release note bits and pieces, so just having a validated schema will iron out many if not most of the exceptions but it will still be harder than just parsing a json file.

@mark-vieira
Copy link
Contributor

Roger that, we can certainly make that happen as a potential next step.

@pugnascotia
Copy link
Contributor Author

Sorry, I haven't had a chance to look at this in great detail yet but what do we intend to do with the stuff in elasticsearch-infra/release-tools? Is there a reason we've baked this into the actual build vs putting it in the release-tools project?

That might be a relic from when I was going to use GitHub actions for some of this stuff. On the other hand, we want to be able to validate the changelog yaml files in CI. We could split out the asciidoc generation? Do you think that's worth it? We'd have logic in 3 places then - homer, ES, and es-infra.

@mark-vieira
Copy link
Contributor

Do you think that's worth it? We'd have logic in 3 places then - homer, ES, and es-infra.

Yeah, that's my concern here. Right now in elasticsearch-infra we have changelog-generator, changlog-status and the generateReleaseNotes stuff that's part of release-tools. How much of this stuff can go away now? What about the migrateGithubIssues task in release-tools? Should we fold that into the ES repo as well?

@pugnascotia
Copy link
Contributor Author

Any pre-existing changelog code will be replaced as part of this effort. migrateGithubIssues is separate but we could easily move it into the main ES repo.

@pugnascotia
Copy link
Contributor Author

@mark-vieira I've refactored the file generation to use Groovy's SimpleTemplateEngine. I'd appreciate a quick once-over before I start writing tests, in order to validate the overall approach / structure.

@pugnascotia
Copy link
Contributor Author

@elasticmachine update branch

@pugnascotia
Copy link
Contributor Author

@mark-vieira what do you think about merging this (subject to removing the test YAML files)?

@mark-vieira
Copy link
Contributor

@mark-vieira what do you think about merging this (subject to removing the test YAML files)?

Seems fine to me.

@pugnascotia pugnascotia merged commit f34d9ad into elastic:master Jul 28, 2021
@pugnascotia pugnascotia deleted the 67335-new-changelog-process branch July 28, 2021 11:09
pugnascotia added a commit that referenced this pull request Jul 28, 2021
Part of #67335.

Add tasks for generating release notes, using information stored in files
in the repository:

   * `generateReleaseNotes` - generates new release notes, release
     highlights and breaking changes
   * `validateChangelogs` - validates that all the changelog YAML files are
     well-formed (confirm to schema, have required fields depending on the
     `type` value)

I also changed `Version` to allow a `v` prefix in relaxed mode
@pugnascotia
Copy link
Contributor Author

Backported to 7.x in a0a39ba. I also removed some yaml files from 7.x in 0d7feb1, which I appear to have committed accidentally at some point.

ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Jul 30, 2021
Part of elastic#67335.

Add tasks for generating release notes, using information stored in files
in the repository:

   * `generateReleaseNotes` - generates new release notes, release
     highlights and breaking changes
   * `validateChangelogs` - validates that all the changelog YAML files are
     well-formed (confirm to schema, have required fields depending on the
     `type` value)

I also changed `Version` to allow a `v` prefix in relaxed mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Delivery/Build Build or test infrastructure >feature Team:Delivery Meta label for Delivery team v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants