From 52f6edf1ab0eac133542eeb4a6cfe164565a6da7 Mon Sep 17 00:00:00 2001 From: Rory Hunter Date: Thu, 18 Aug 2022 19:44:35 +0100 Subject: [PATCH] Updates to changelog processing after docs redesign (#89463) --- .../release/ValidateChangelogEntryTask.java | 76 ++++++-- .../templates/release-highlights.asciidoc | 8 +- .../ReleaseHighlightsGeneratorTest.java | 8 +- .../ValidateChangelogEntryTaskTest.java | 179 ++++++++++++++++++ ...hlightsGeneratorTest.generateFile.asciidoc | 24 ++- docs/changelog/83345.yaml | 5 +- 6 files changed, 265 insertions(+), 35 deletions(-) create mode 100644 build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/release/ValidateChangelogEntryTaskTest.java diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/release/ValidateChangelogEntryTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/release/ValidateChangelogEntryTask.java index 14114314ad4de..acbd79fe28194 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/release/ValidateChangelogEntryTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/release/ValidateChangelogEntryTask.java @@ -8,6 +8,8 @@ package org.elasticsearch.gradle.internal.release; +import com.google.common.annotations.VisibleForTesting; + import org.gradle.api.DefaultTask; import org.gradle.api.GradleException; import org.gradle.api.file.ConfigurableFileCollection; @@ -30,6 +32,21 @@ public class ValidateChangelogEntryTask extends DefaultTask { private final ConfigurableFileCollection changelogs; private final ProjectLayout projectLayout; + public static final String TRIPLE_BACKTICK = "```"; + private static final String CODE_BLOCK_ERROR = """ + [%s] uses a triple-backtick in the [%s] section, but it must be + formatted as a Asciidoc code block. For example: + + [source,yaml] + ---- + { + "metrics.time" : 10, + "metrics.time.min" : 1, + "metrics.time.max" : 500 + } + ---- + """; + @Inject public ValidateChangelogEntryTask(ObjectFactory objectFactory, ProjectLayout projectLayout) { this.changelogs = objectFactory.fileCollection(); @@ -43,37 +60,60 @@ public void executeTask() { .stream() .collect(Collectors.toMap(file -> rootDir.relativize(file.toURI()).toString(), ChangelogEntry::parse)); + changelogs.forEach(ValidateChangelogEntryTask::validate); + } + + @VisibleForTesting + static void validate(String path, ChangelogEntry entry) { // We don't try to find all such errors, because we expect them to be rare e.g. only // when a new file is added. - changelogs.forEach((path, entry) -> { - final String type = entry.getType(); - - if (type.equals("known-issue") == false && type.equals("security") == false) { - if (entry.getPr() == null) { - throw new GradleException( - "[" + path + "] must provide a [pr] number (only 'known-issue' and " + "'security' entries can omit this" - ); - } - - if (entry.getArea() == null) { - throw new GradleException( - "[" + path + "] must provide an [area] (only 'known-issue' and " + "'security' entries can omit this" - ); - } + final String type = entry.getType(); + + if (type.equals("known-issue") == false && type.equals("security") == false) { + if (entry.getPr() == null) { + throw new GradleException( + "[" + path + "] must provide a [pr] number (only 'known-issue' and 'security' entries can omit this" + ); } - if ((type.equals("breaking") || type.equals("breaking-java")) && entry.getBreaking() == null) { + if (entry.getArea() == null) { + throw new GradleException("[" + path + "] must provide an [area] (only 'known-issue' and 'security' entries can omit this"); + } + } + + if (type.equals("breaking") || type.equals("breaking-java")) { + if (entry.getBreaking() == null) { throw new GradleException( "[" + path + "] has type [" + type + "] and must supply a [breaking] section with further information" ); } - if (type.equals("deprecation") && entry.getDeprecation() == null) { + if (entry.getBreaking().getDetails().contains(TRIPLE_BACKTICK)) { + throw new GradleException(CODE_BLOCK_ERROR.formatted(path, "breaking.details")); + } + if (entry.getBreaking().getImpact().contains(TRIPLE_BACKTICK)) { + throw new GradleException(CODE_BLOCK_ERROR.formatted(path, "breaking.impact")); + } + } + + if (type.equals("deprecation")) { + if (entry.getDeprecation() == null) { throw new GradleException( "[" + path + "] has type [deprecation] and must supply a [deprecation] section with further information" ); } - }); + + if (entry.getDeprecation().getDetails().contains(TRIPLE_BACKTICK)) { + throw new GradleException(CODE_BLOCK_ERROR.formatted(path, "deprecation.details")); + } + if (entry.getDeprecation().getImpact().contains(TRIPLE_BACKTICK)) { + throw new GradleException(CODE_BLOCK_ERROR.formatted(path, "deprecation.impact")); + } + } + + if (entry.getHighlight() != null && entry.getHighlight().getBody().contains(TRIPLE_BACKTICK)) { + throw new GradleException(CODE_BLOCK_ERROR.formatted(path, "highlight.body")); + } } @InputFiles diff --git a/build-tools-internal/src/main/resources/templates/release-highlights.asciidoc b/build-tools-internal/src/main/resources/templates/release-highlights.asciidoc index f07ba9c5d4db3..bd8ef8602530b 100644 --- a/build-tools-internal/src/main/resources/templates/release-highlights.asciidoc +++ b/build-tools-internal/src/main/resources/templates/release-highlights.asciidoc @@ -32,14 +32,18 @@ if (notableHighlights.isEmpty()) { %> <% for (highlight in notableHighlights) { %> [discrete] [[${ highlight.anchor }]] -=== {es-pull}${highlight.pr}[${highlight.title}] +=== ${highlight.title} ${highlight.body.trim()} + +{es-pull}${highlight.pr}[#${highlight.pr}] <% } %> // end::notable-highlights[] <% } %> <% for (highlight in nonNotableHighlights) { %> [discrete] [[${ highlight.anchor }]] -=== {es-pull}${highlight.pr}[${highlight.title}] +=== ${highlight.title} ${highlight.body.trim()} + +{es-pull}${highlight.pr}[#${highlight.pr}] <% } %> diff --git a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/release/ReleaseHighlightsGeneratorTest.java b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/release/ReleaseHighlightsGeneratorTest.java index 7f510bef22661..db39c6eea7e86 100644 --- a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/release/ReleaseHighlightsGeneratorTest.java +++ b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/release/ReleaseHighlightsGeneratorTest.java @@ -60,11 +60,11 @@ public void generateFile_rendersCorrectMarkup() throws Exception { } private List getEntries() { - ChangelogEntry entry1 = makeChangelogEntry(1, true); - ChangelogEntry entry2 = makeChangelogEntry(2, true); - ChangelogEntry entry3 = makeChangelogEntry(3, false); + ChangelogEntry entry123 = makeChangelogEntry(123, true); + ChangelogEntry entry456 = makeChangelogEntry(456, true); + ChangelogEntry entry789 = makeChangelogEntry(789, false); // Return unordered list, to test correct re-ordering - return List.of(entry2, entry1, entry3); + return List.of(entry456, entry123, entry789); } private ChangelogEntry makeChangelogEntry(int pr, boolean notable) { diff --git a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/release/ValidateChangelogEntryTaskTest.java b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/release/ValidateChangelogEntryTaskTest.java new file mode 100644 index 0000000000000..ec7b47b057a97 --- /dev/null +++ b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/release/ValidateChangelogEntryTaskTest.java @@ -0,0 +1,179 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License + * 2.0 and the Server Side Public License, v 1; you may not use this file except + * in compliance with, at your election, the Elastic License 2.0 or the Server + * Side Public License, v 1. + */ + +package org.elasticsearch.gradle.internal.release; + +import org.gradle.api.GradleException; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.Test; + +import java.util.stream.Stream; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.endsWith; + +class ValidateChangelogEntryTaskTest { + + @Test + void test_prNumber_isRequired() { + ChangelogEntry changelog = new ChangelogEntry(); + changelog.setType("enhancement"); + + final String message = doValidate(changelog); + + assertThat(message, endsWith("must provide a [pr] number (only 'known-issue' and 'security' entries can omit this")); + } + + @Test + void test_prNumber_notRequired() { + Stream.of("known-issue", "security").forEach(type -> { + ChangelogEntry changelog = new ChangelogEntry(); + changelog.setType(type); + + // Should not throw an exception! + ValidateChangelogEntryTask.validate("", changelog); + }); + } + + @Test + void test_area_isRequired() { + final ChangelogEntry changelog = new ChangelogEntry(); + changelog.setType("enhancement"); + changelog.setPr(123); + + final String message = doValidate(changelog); + + assertThat(message, endsWith("must provide an [area] (only 'known-issue' and 'security' entries can omit this")); + } + + @Test + void test_breaking_requiresBreakingSection() { + Stream.of("breaking", "breaking-java").forEach(type -> { + final ChangelogEntry changelog = buildChangelog(type); + + final String message = doValidate(changelog); + + assertThat(message, endsWith("has type [" + type + "] and must supply a [breaking] section with further information")); + }); + } + + @Test + void test_breaking_rejectsTripleBackticksInDetails() { + Stream.of("breaking", "breaking-java").forEach(type -> { + final ChangelogEntry.Breaking breaking = new ChangelogEntry.Breaking(); + breaking.setDetails(""" + Some waffle. + ``` + I AM CODE! + ``` + """); + + final ChangelogEntry changelog = buildChangelog(type); + changelog.setBreaking(breaking); + + final String message = doValidate(changelog); + + assertThat(message, containsString("uses a triple-backtick in the [breaking.details] section")); + }); + } + + @Test + void test_breaking_rejectsTripleBackticksInImpact() { + Stream.of("breaking", "breaking-java").forEach(type -> { + final ChangelogEntry.Breaking breaking = new ChangelogEntry.Breaking(); + breaking.setDetails("Waffle waffle"); + breaking.setImpact(""" + More waffle. + ``` + THERE ARE WEASEL RAKING THROUGH MY GARBAGE! + ``` + """); + + final ChangelogEntry changelog = buildChangelog(type); + changelog.setBreaking(breaking); + + final String message = doValidate(changelog); + + assertThat(message, containsString("uses a triple-backtick in the [breaking.impact] section")); + }); + } + + @Test + void test_deprecation_rejectsTripleBackticksInImpact() { + final ChangelogEntry.Deprecation deprecation = new ChangelogEntry.Deprecation(); + deprecation.setDetails("Waffle waffle"); + deprecation.setImpact(""" + More waffle. + ``` + THERE ARE WEASEL RAKING THROUGH MY GARBAGE! + ``` + """); + + final ChangelogEntry changelog = buildChangelog("deprecation"); + changelog.setDeprecation(deprecation); + + final String message = doValidate(changelog); + + assertThat(message, containsString("uses a triple-backtick in the [deprecation.impact] section")); + } + + @Test + void test_deprecation_rejectsTripleBackticksInDetails() { + final ChangelogEntry.Deprecation deprecation = new ChangelogEntry.Deprecation(); + deprecation.setDetails(""" + Some waffle. + ``` + I AM CODE! + ``` + """); + + final ChangelogEntry changelog = buildChangelog("deprecation"); + changelog.setDeprecation(deprecation); + + final String message = doValidate(changelog); + + assertThat(message, containsString("uses a triple-backtick in the [deprecation.details] section")); + } + + @Test + void test_highlight_rejectsTripleBackticksInBody() { + final ChangelogEntry.Highlight highlight = new ChangelogEntry.Highlight(); + highlight.setBody(""" + Some waffle. + ``` + I AM CODE! + ``` + """); + + final ChangelogEntry changelog = buildChangelog("enhancement"); + changelog.setHighlight(highlight); + + final String message = doValidate(changelog); + + assertThat(message, containsString("uses a triple-backtick in the [highlight.body] section")); + } + + private static ChangelogEntry buildChangelog(String type) { + final ChangelogEntry changelog = new ChangelogEntry(); + changelog.setType(type); + changelog.setPr(123); + changelog.setArea("Infra/Core"); + return changelog; + } + + private String doValidate(ChangelogEntry entry) { + try { + ValidateChangelogEntryTask.validate("docs/123.yaml", entry); + throw new AssertionError("No exception thrown!"); + } catch (Exception e) { + assertThat(e, Matchers.instanceOf(GradleException.class)); + return e.getMessage(); + } + } +} diff --git a/build-tools-internal/src/test/resources/org/elasticsearch/gradle/internal/release/ReleaseHighlightsGeneratorTest.generateFile.asciidoc b/build-tools-internal/src/test/resources/org/elasticsearch/gradle/internal/release/ReleaseHighlightsGeneratorTest.generateFile.asciidoc index a55a590a8bca5..19c713042a42b 100644 --- a/build-tools-internal/src/test/resources/org/elasticsearch/gradle/internal/release/ReleaseHighlightsGeneratorTest.generateFile.asciidoc +++ b/build-tools-internal/src/test/resources/org/elasticsearch/gradle/internal/release/ReleaseHighlightsGeneratorTest.generateFile.asciidoc @@ -20,20 +20,26 @@ Other versions: // tag::notable-highlights[] [discrete] -[[notable_release_highlight_number_1]] -=== {es-pull}1[Notable release highlight number 1] -Notable release body number 1 +[[notable_release_highlight_number_123]] +=== Notable release highlight number 123 +Notable release body number 123 + +{es-pull}123[#123] [discrete] -[[notable_release_highlight_number_2]] -=== {es-pull}2[Notable release highlight number 2] -Notable release body number 2 +[[notable_release_highlight_number_456]] +=== Notable release highlight number 456 +Notable release body number 456 + +{es-pull}456[#456] // end::notable-highlights[] [discrete] -[[notable_release_highlight_number_3]] -=== {es-pull}3[Notable release highlight number 3] -Notable release body number 3 +[[notable_release_highlight_number_789]] +=== Notable release highlight number 789 +Notable release body number 789 + +{es-pull}789[#789] diff --git a/docs/changelog/83345.yaml b/docs/changelog/83345.yaml index 25e49cd882719..955e31a3c1929 100644 --- a/docs/changelog/83345.yaml +++ b/docs/changelog/83345.yaml @@ -14,7 +14,8 @@ highlight: As an example, the following ILM policy would roll an index over if it is at least 7 days old or at least 100 gigabytes, but only as long as the index is not empty. - ``` + [source,console] + ---- PUT _ilm/policy/my_policy { "policy": { @@ -31,5 +32,5 @@ highlight: } } } - ``` + ---- notable: true