New release notes generator tasks #68463
Conversation
pugnascotia
added
>feature
v8.0.0
v7.12.0
:Delivery/Tooling
|
Pinging @elastic/es-delivery (Team:Delivery) |
| public class GenerateReleaseNotesTask extends DefaultTask { | ||
| private static final Logger LOGGER = Logging.getLogger(GenerateReleaseNotesTask.class); | ||
|
|
||
| private final ConfigurableFileCollection changelogs = getProject().getObjects().fileCollection(); |
There was a problem hiding this comment.
we should avoid relying on getProject() here. (Mostly because this will likely bite us when moving closer to supporting configuration on demand where referencing project can be an issue)
Instead we should use inject ObjectFactory via constructor and create those properties in the constructor.
| public GenerateReleaseNotesTask() { | ||
| final Version version = VersionProperties.getElasticsearchVersion(); | ||
|
|
||
| this.changelogs.setFrom( |
There was a problem hiding this comment.
I think this shouldn't be done in the task itself but in the plugin. The changelogs property should be a simple property that is configured from the plugin.
| .getFiles() | ||
| ); | ||
|
|
||
| this.releaseNotesFile.set( |
There was a problem hiding this comment.
same as above with changelogs should be set from the plugin imo
| .file(String.format("docs/reference/release-notes/%d.%d.asciidoc", version.getMajor(), version.getMinor())) | ||
| ); | ||
|
|
||
| this.releaseHighlightsFile.set( |
There was a problem hiding this comment.
same as above with changelogs should be set from the plugin imo
| getProject().getLayout().getProjectDirectory().file("docs/reference/release-notes/highlights.asciidoc") | ||
| ); | ||
|
|
||
| this.breakingChangesFile.set( |
There was a problem hiding this comment.
same as above with changelogs should be set from the plugin imo
| ); | ||
|
|
||
| this.breakingChangesFile.set( | ||
| getProject().getLayout() |
There was a problem hiding this comment.
In case you need ProjectLayout in a task please inject it via constructor like described above with ObjectFactory
| this.breakingChangesFile.set( | ||
| getProject().getLayout() | ||
| .getProjectDirectory() | ||
| .file(String.format("docs/reference/migration/migrate_%d_%d.asciidoc", version.getMajor(), version.getMinor())) |
There was a problem hiding this comment.
I think in general I think we want to have the task not knowing anything of these conventional locations and keep those in the plugin
| List.of( | ||
| "[[breaking-changes-" + majorDotMinor + "]]", | ||
| "== Breaking changes in " + majorDotMinor, | ||
| "++++", | ||
| "<titleabbrev>" + majorDotMinor + "</titleabbrev>", | ||
| "++++", | ||
| "", | ||
| "This section discusses the changes that you need to be aware of when migrating", | ||
| "your application to {es} " + majorDotMinor + ".", | ||
| "", | ||
| "See also <<release-highlights>> and <<es-release-notes>>." | ||
| ).forEach(out::println); | ||
|
|
||
| if (VersionProperties.isElasticsearchSnapshot()) { | ||
| out.println(); | ||
| out.println("coming[" + version + "]"); | ||
| } | ||
|
|
||
| out.println(); | ||
| out.println("//NOTE: The notable-breaking-changes tagged regions are re-used in the"); | ||
| out.println("//Installation and Upgrade Guide"); |
There was a problem hiding this comment.
It doesn't look like #66569 has been applied to 7.x or master yet, but we've rebranded our breaking changes as a migration guide:
https://www.elastic.co/guide/en/elasticsearch/reference/7.11/migrating-7.10.html
I think we'll need to update this file to separate breaking changes and deprecations. We'll also need to update the boilerplate text here.
There was a problem hiding this comment.
@debadair authored the migration guide changes. I'll add her as a reviewer to get her feedback.
There was a problem hiding this comment.
Thanks for the ping. I've merged the rest of the migration guide changes. It introduces separate sections for the Breaking Changes and Deprecations, so the heading levels need to be bumped down.
There was a problem hiding this comment.
Thanks for opening this @pugnascotia! Two bigger pieces of feedback:
-
With [DOCS] Rebrand breaking changes as the Migration guide #66569, we rebranded our breaking changes page as a migration guide. This change separates breaking changes and deprecations in the
migrate_<version>.asciidocfiles. It also updates our boilerplate copy. We'll need to update the breaking change generator for these changes. I've added @debadair, who authored the migration guide changes, so she can provide feedback as well. -
I ran
./gradlew GenerateReleaseNote. However, it looks like the generator created the appropriate pages but, aside from the release notes, didn't pull in the YAML data. Here's my local diff after running the command:
diff --git a/docs/reference/migration/migrate_7_12.asciidoc b/docs/reference/migration/migrate_7_12.asciidoc
index d2ddcc55166..471b4c44b0d 100644
--- a/docs/reference/migration/migrate_7_12.asciidoc
+++ b/docs/reference/migration/migrate_7_12.asciidoc
@@ -9,25 +9,7 @@ your application to {es} 7.12.
See also <<release-highlights>> and <<es-release-notes>>.
-* <<breaking_712_engine_changes>>
+coming[7.12.0]
//NOTE: The notable-breaking-changes tagged regions are re-used in the
//Installation and Upgrade Guide
-
-//tag::notable-breaking-changes[]
-
-[discrete]
-[[breaking_712_engine_changes]]
-=== Engine changes
-
-[[breaking_712_engine_forcemerge_change]]
-.Force-merges on frozen and searchable snapshot indices will fail if merging is required.
-[%collapsible]
-====
-*Details* +
-In earlier versions a force-merge on a frozen index or a searchable snapshot
-index would incorrectly yield a successful response without performing the
-requested merge. This bug is fixed in version 7.12: from this version onwards a
-force-merge on these immutable indices will fail if the requested merge is not
-a no-op.
-====
diff --git a/docs/reference/release-notes/highlights.asciidoc b/docs/reference/release-notes/highlights.asciidoc
index 43857de0cc9..8af8c5bfb94 100644
--- a/docs/reference/release-notes/highlights.asciidoc
+++ b/docs/reference/release-notes/highlights.asciidoc
@@ -12,7 +12,8 @@ endif::[]
// Add previous release to the list
Other versions:
-{ref-bare}/7.11/release-highlights.html[7.11]
+{ref-bare}/7.12/release-highlights.html[7.12]
+| {ref-bare}/7.11/release-highlights.html[7.11]
| {ref-bare}/7.10/release-highlights.html[7.10]
| {ref-bare}/7.9/release-highlights.html[7.9]
| {ref-bare}/7.8/release-highlights.html[7.8]
@@ -26,18 +27,3 @@ Other versions:
| {ref-bare}/7.0/release-highlights-7.0.0.html[7.0]
-
-// Use the notable-highlights tag to mark entries that
-// should be featured in the Stack Installation and Upgrade Guide:
-// tag::notable-highlights[]
-// [discrete]
-// === Heading
-//
-// Description.
-// end::notable-highlights[]
-
-// Omit the notable highlights tag for entries that only need to appear in the ES ref:
-// [discrete]
-// === Heading
-//
-// Description.
|
|
||
| private void generateGroupHeader(Version version, String type) { | ||
| out.println("[[" + type + "-" + version + "]]"); | ||
| out.println("[float]"); |
There was a problem hiding this comment.
Style nit: We try to use [discrete] now per Asciidoctor preferences: https://asciidoctor.org/docs/asciidoc-asciidoctor-diffs/#blocks
Shouldn't make a difference to the actual output.
| out.println("[float]"); | |
| out.println("[discrete]"); |
| out.println("[[release-notes-" + version + "]]"); | ||
| out.println("== {es} version " + version); |
There was a problem hiding this comment.
It'd be nifty to auto-include and xref this file in release-notes.asciidoc: https://github.com/pugnascotia/elasticsearch/blob/67335-new-changelog-process-7x/docs/reference/release-notes.asciidoc
There was a problem hiding this comment.
I've added code to update the release notes index file 👍
There was a problem hiding this comment.
What are the rules for actually including these files in release notes? I ran this locally, and it seemed like the vast majority of those yaml files weren't added to any generated files.
Do we have plans for automating the creation of those changelog files in any way? If we expect every PR to have one that's going to be quite tedious.
|
|
||
| import java.io.File; | ||
|
|
||
| public class ValidateChangelogsTask extends DefaultTask { |
There was a problem hiding this comment.
I'm wondering if it might make sense to use the existing ValidateJsonAgainstSchemaTask we use for the REST specs and create a schema for this. I find the error messages produced there pretty good we could move all the logic in ChangelogEntry.validate() into something more declarative.
| String majorDotMinor = version.getMajor() + "." + version.getMinor(); | ||
| String majorMinor = String.valueOf(version.getMajor()) + version.getMinor(); | ||
|
|
||
| List.of( |
There was a problem hiding this comment.
I wonder if it might make sense to use some kind of templating framework here (groovy, mustache) to make updating the formatting here a bit more approachable.
| action.setDescription("Generates release notes from changelog files held in this checkout"); | ||
| }); | ||
|
|
||
| project.getTasks().register("validateChangelogs", ValidateChangelogsTask.class).configure(action -> { |
There was a problem hiding this comment.
This should be wired up to the check task so we do this validation during CI checks.
|
Apologies all - there was a bug in the logic for omitting YAML files that don't apply to the current version. I've fixed the bug (and started addressing the other feedback). |
| List.of( | ||
| "[[breaking-changes-" + majorDotMinor + "]]", | ||
| "== Breaking changes in " + majorDotMinor, | ||
| "++++", | ||
| "<titleabbrev>" + majorDotMinor + "</titleabbrev>", | ||
| "++++", | ||
| "", | ||
| "This section discusses the changes that you need to be aware of when migrating", | ||
| "your application to {es} " + majorDotMinor + ".", | ||
| "", | ||
| "See also <<release-highlights>> and <<es-release-notes>>." | ||
| ).forEach(out::println); | ||
|
|
||
| if (VersionProperties.isElasticsearchSnapshot()) { | ||
| out.println(); | ||
| out.println("coming[" + version + "]"); | ||
| } | ||
|
|
||
| out.println(); | ||
| out.println("//NOTE: The notable-breaking-changes tagged regions are re-used in the"); | ||
| out.println("//Installation and Upgrade Guide"); |
There was a problem hiding this comment.
Thanks for the ping. I've merged the rest of the migration guide changes. It introduces separate sections for the Breaking Changes and Deprecations, so the heading levels need to be bumped down.
| out.println("===="); | ||
| out.println("*Details* +"); | ||
| out.println(breaking.getBody().trim()); | ||
| out.println("===="); |
There was a problem hiding this comment.
For some changes, there is also an Impact section that describes what action you need to take.
|
@mark-vieira I don't yet have plans to automate the generation of the YAML files on an ongoing basis. Since we'll want some kind of bot support anyway, we could have a bot prepare a basic YAML file if there isn't one in the PR. Of course, when you first open a PR you won't know the PR number, which makes it tricky to populate the So maybe the flow could be:
We could get fancy and leave some kind of placeholder text in the stub PR, and keep the PR labelled with e.g. "requires-changelog" unless the stub text is removed. This flow could be a good candidate for a GitHub action, since it won't require any heavy lifting with Gradle - unless, of course, we choose to implement it with Gradle 🤷♂️ |
Indeed, all the more reason to automate this. In fact, I almost consider having some automation in place here a prerequisite to migrating to such a method of handling release notes. Otherwise this is just far too cumbersome on developers IMO, but perhaps this requires a larger conversation, given this is going to have a substantial impact on development flow.
Yep, that all sounds good to me. I don't have strong feelings on how it's implemented so long as it's easy to maintain. There are certainly details to work out, I'll leave it up to you on the best way to discuss/iterate on the design details (gh issue, google doc, email thread, etc). |
| task.setDescription("Validates that all the changelog YAML files are well-formed"); | ||
|
|
||
| task.setInputFiles( | ||
| project.getLayout() |
There was a problem hiding this comment.
Instead of relying on project.getLayout() we can inject ProjectLayout via the constructor here. This reads cleaner imo and also helps us later on when we wanna migrate to --configuration-cache were referecing the mutable project instance is discouraged from several places.
| action.setDescription("Generates release notes from changelog files held in this checkout"); | ||
|
|
||
| action.setChangelogs( | ||
| project.getLayout() |
There was a problem hiding this comment.
use injected ProjectLayout instance
| action.setReleaseNotesIndexFile(project.getLayout().getProjectDirectory().file("docs/reference/release-notes.asciidoc")); | ||
|
|
||
| action.setReleaseNotesFile( | ||
| project.getLayout() |
There was a problem hiding this comment.
use injected ProjectLayout instance
| ); | ||
|
|
||
| action.setReleaseHighlightsFile( | ||
| project.getLayout().getProjectDirectory().file("docs/reference/release-notes/highlights.asciidoc") |
There was a problem hiding this comment.
use injected ProjectLayout instance
| ); | ||
|
|
||
| action.setBreakingChangesFile( | ||
| project.getLayout() |
There was a problem hiding this comment.
use injected ProjectLayout instance
| .getFiles() | ||
| ); | ||
|
|
||
| action.setReleaseNotesIndexFile(project.getLayout().getProjectDirectory().file("docs/reference/release-notes.asciidoc")); |
There was a problem hiding this comment.
use injected ProjectLayout instance
|
Closed in favour of #71125. |
|
Closed in favour of #71125 |
Part of #67335.
cc @jrodewig for your thoughts on usability and output formatting.
This is an initial draft of a new process for generating release notes, using information stored in files in the repository. It adds two new Gradle tasks:
generateReleaseNotes- generates new release notes, release highlights and breaking changesvalidateChangelogs- validates that all the changelog YAML files are well-formedThis PR is targeted at 7.12 because that's where it will first be used, and during development it seems easier to work there than in
master.This work isn't complete in itself, because we will need some kind of branch validation to ensure that PRs that ought to include YAML files do so.
Notes:
Versionto allow avprefix, and to capture anything after the patch version number as "labels".