From 8f2f6dadd55c7d8f2b34203f00a44f5dc46d5645 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 19 Sep 2025 07:20:05 -0700 Subject: [PATCH] Only validate primary ids on release branches (#135044) Primary ids are only incremented on the main branch. For release branches only patch ids will be created. The primary id validation won't always work on release branches because only some of the primary ids will be backported. This commit skips primary id validation if we are sure we are on a release branch. We can tell this by looking at the current upper bound compared to other upper bounds. If there is a newer one, we know we are on a release branch. If there isn't a newer one, we _might_ be on a release branch, or on main, but in this case doing primary id validation is ok because there's effectively no difference, we are looking at the latest upper bound. --- .../AbstractTransportVersionFuncTest.groovy | 3 +++ .../TransportVersionValidationFuncTest.groovy | 17 +++++++++++++ .../TransportVersionResourcesPlugin.java | 24 +++++++++++++++---- ...ValidateTransportVersionResourcesTask.java | 21 +++++++++++++++- 4 files changed, 59 insertions(+), 6 deletions(-) diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy index 72c52a524dc33..18ebfb6a314d7 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/AbstractTransportVersionFuncTest.groovy @@ -125,6 +125,9 @@ class AbstractTransportVersionFuncTest extends AbstractGradleFuncTest { tasks.named('generateTransportVersion') { currentUpperBoundName = '9.2' } + tasks.named('validateTransportVersionResources') { + currentUpperBoundName = '9.2' + } """ referableAndReferencedTransportVersion("existing_91", "8012000") referableAndReferencedTransportVersion("older_92", "8122000") diff --git a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy index 0ef9bc35de631..861b7a11b8f1a 100644 --- a/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy +++ b/build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionValidationFuncTest.groovy @@ -277,4 +277,21 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes "[myserver/src/main/resources/transport/definitions/referable/existing_92.csv]" ) } + + def "primary id checks skipped on release branch"() { + given: + file("myserver/build.gradle") << """ + tasks.named('validateTransportVersionResources') { + currentUpperBoundName = '9.1' + } + """ + referableAndReferencedTransportVersion("some_tv", "8125000") + transportVersionUpperBound("9.2", "some_tv", "8125000") + + when: + def result = gradleRunner("validateTransportVersionResources").build() + + then: + result.task(":myserver:validateTransportVersionResources").outcome == TaskOutcome.SUCCESS + } } diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesPlugin.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesPlugin.java index 433bba0c058ba..44a00281dabb3 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesPlugin.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesPlugin.java @@ -14,6 +14,7 @@ import org.elasticsearch.gradle.internal.conventions.VersionPropertiesPlugin; import org.elasticsearch.gradle.internal.conventions.precommit.PrecommitPlugin; import org.elasticsearch.gradle.internal.conventions.precommit.PrecommitTaskPlugin; +import org.gradle.api.Action; import org.gradle.api.Plugin; import org.gradle.api.Project; import org.gradle.api.file.Directory; @@ -69,6 +70,7 @@ public void apply(Project project) { t.getReferencesFiles().setFrom(tvReferencesConfig); t.getShouldValidateDensity().convention(true); t.getShouldValidatePrimaryIdNotPatch().convention(true); + t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor()); }); project.getTasks().named(PrecommitPlugin.PRECOMMIT_TASK_NAME).configure(t -> t.dependsOn(validateTask)); @@ -79,19 +81,31 @@ public void apply(Project project) { t.getManifestFile().set(project.getLayout().getBuildDirectory().file("generated-resources/manifest.txt")); }); project.getTasks().named(JavaPlugin.PROCESS_RESOURCES_TASK_NAME, Copy.class).configure(t -> { - t.into("transport/definitions", c -> c.from(generateManifestTask)); + t.into(resourceRoot + "/definitions", c -> c.from(generateManifestTask)); }); + Action generationConfiguration = t -> { + t.setGroup(taskGroup); + t.getReferencesFiles().setFrom(tvReferencesConfig); + t.getIncrement().convention(1000); + t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor()); + }; + var generateDefinitionsTask = project.getTasks() .register("generateTransportVersion", GenerateTransportVersionDefinitionTask.class, t -> { - t.setGroup(taskGroup); t.setDescription("(Re)generates a transport version definition file"); - t.getReferencesFiles().setFrom(tvReferencesConfig); - t.getIncrement().convention(1000); - t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor()); }); + generateDefinitionsTask.configure(generationConfiguration); validateTask.configure(t -> t.mustRunAfter(generateDefinitionsTask)); + var resolveConflictTask = project.getTasks() + .register("resolveTransportVersionConflict", GenerateTransportVersionDefinitionTask.class, t -> { + t.setDescription("Resolve merge conflicts in transport version internal state files"); + t.getResolveConflict().set(true); + }); + resolveConflictTask.configure(generationConfiguration); + validateTask.configure(t -> t.mustRunAfter(resolveConflictTask)); + var generateInitialTask = project.getTasks() .register("generateInitialTransportVersion", GenerateInitialTransportVersionTask.class, t -> { t.setGroup(taskGroup); diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java index 829ce577891df..c796f1e905214 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java @@ -60,6 +60,13 @@ public Path getResourcesDir() { @Input public abstract Property getShouldValidatePrimaryIdNotPatch(); + /** + * The name of the upper bounds file which will be used at runtime on the current branch. Normally + * this equates to VersionProperties.getElasticsearchVersion(). + */ + @Input + public abstract Property getCurrentUpperBoundName(); + private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {} private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+"); @@ -76,6 +83,7 @@ public void validateTransportVersions() throws IOException { Map allDefinitions = collectAllDefinitions(referableDefinitions, unreferableDefinitions); Map> idsByBase = collectIdsByBase(allDefinitions.values()); Map upperBounds = resources.getUpperBounds(); + boolean onReleaseBranch = checkIfDefinitelyOnReleaseBranch(upperBounds); for (var definition : referableDefinitions.values()) { validateNamedDefinition(definition, referencedNames); @@ -93,7 +101,9 @@ public void validateTransportVersions() throws IOException { validateUpperBound(upperBound, allDefinitions, idsByBase); } - validatePrimaryIds(resources, upperBounds, allDefinitions); + if (onReleaseBranch == false) { + validatePrimaryIds(resources, upperBounds, allDefinitions); + } } private Map collectAllDefinitions( @@ -318,6 +328,15 @@ private void validatePrimaryIds( ); } + private boolean checkIfDefinitelyOnReleaseBranch(Map upperBounds) { + // only want to look at definitions <= the current upper bound. + // TODO: we should filter all of the upper bounds/definitions that are validated by this, not just in this method + String currentUpperBoundName = getCurrentUpperBoundName().get(); + TransportVersionUpperBound currentUpperBound = upperBounds.get(currentUpperBoundName); + + return upperBounds.values().stream().anyMatch(u -> u.definitionId().complete() > currentUpperBound.definitionId().complete()); + } + private void throwDefinitionFailure(TransportVersionDefinition definition, String message) { Path relativePath = getResources().get().getDefinitionPath(definition); throw new VerificationException("Transport version definition file [" + relativePath + "] " + message);