From d23df289ed29d3269a446ece90cf5a0a8fa5e019 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 8 Oct 2025 11:29:15 -0700 Subject: [PATCH 1/4] Use PR base branch when checking transport version modifications Transport version validation relies on comparing the local state to the "base" version of each file. The base can be calculcated in several different context-dependent ways, but by default it compares to upstream main to determine the merge base. In release branches this is incorrect since it will find the point at with the release branch was created. This commit adjusts the base ref to be relative to the PR base branch when runnign in CI. When running locally on release branches these modification checks are disabled since there is nothing to compare to. --- .../TransportVersionValidationFuncTest.groovy | 39 +++++++++ .../TransportVersionResourcesPlugin.java | 9 +++ .../TransportVersionResourcesService.java | 11 ++- ...ValidateTransportVersionResourcesTask.java | 81 +++++++++++-------- 4 files changed, 102 insertions(+), 38 deletions(-) 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 712ac849222ad..9ff4c7525c32d 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 @@ -311,4 +311,43 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes then: result.task(":myserver:validateTransportVersionResources").outcome == TaskOutcome.SUCCESS } + + def "modification checks use PR base branch in CI"() { + given: + // create 9.1 branch + execute("git checkout -b 9.1") + // setup 9.1 upper bound as if 9.2 was just branched, so no patches yet + unreferableTransportVersion("initial_9.1.0", "8013000") + transportVersionUpperBound("9.1", "initial_9.1.0", "8013000") + execute("git add .") + execute("git commit -m initial") + + // advance main branch + execute("git checkout main") + unreferableTransportVersion("initial_9.1.0", "8013000") + referableAndReferencedTransportVersion("new_tv", "8124000,8013001") + transportVersionUpperBound("9.2", "new_tv", "8124000") + transportVersionUpperBound("9.1", "new_tv", "8013001") + execute("git add .") + execute("git commit -m update") + + // create a 9.1 backport + execute("git checkout 9.1") + execute("git checkout -b test_9.1") + referableAndReferencedTransportVersion("new_tv", "8124000,8013001") + transportVersionUpperBound("9.2", "new_tv", "8124000") + transportVersionUpperBound("9.1", "new_tv", "8013001") + file("myserver/build.gradle") << """ + tasks.named('validateTransportVersionResources') { + currentUpperBoundName = '9.1' + } + """ + + when: + def result = gradleRunner("validateTransportVersionResources") + .withEnvironment(Map.of("BUILDKITE_PULL_REQUEST_BASE_BRANCH", "9.1")).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 1e3c2ad2e3f82..733bf3be23cc5 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,11 +14,14 @@ 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.elasticsearch.gradle.internal.info.BuildParameterExtension; +import org.elasticsearch.gradle.internal.info.GlobalBuildInfoPlugin; import org.gradle.api.Action; import org.gradle.api.Plugin; import org.gradle.api.Project; import org.gradle.api.file.Directory; import org.gradle.api.plugins.JavaPlugin; +import org.gradle.api.provider.Property; import org.gradle.api.provider.Provider; import org.gradle.api.tasks.Copy; import org.gradle.language.base.plugins.LifecycleBasePlugin; @@ -26,6 +29,8 @@ import java.util.Map; import java.util.Properties; +import static org.elasticsearch.gradle.internal.util.ParamsUtils.loadBuildParams; + public class TransportVersionResourcesPlugin implements Plugin { public static final String TRANSPORT_REFERENCES_TOPIC = "transportReferences"; @@ -37,6 +42,9 @@ public void apply(Project project) { project.getPluginManager().apply(PrecommitTaskPlugin.class); var psService = project.getPlugins().apply(ProjectSubscribeServicePlugin.class).getService(); + project.getRootProject().getPlugins().apply(GlobalBuildInfoPlugin.class); + Property buildParams = loadBuildParams(project); + Properties versions = (Properties) project.getExtensions().getByName(VersionPropertiesPlugin.VERSIONS_EXT); Version currentVersion = Version.fromString(versions.getProperty("elasticsearch")); @@ -76,6 +84,7 @@ public void apply(Project project) { t.getShouldValidateDensity().convention(true); t.getShouldValidatePrimaryIdNotPatch().convention(true); t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor()); + t.getCI().set(buildParams.get().getCi()); }); project.getTasks().named(PrecommitPlugin.PRECOMMIT_TASK_NAME).configure(t -> t.dependsOn(validateTask)); diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java index 208d73b0a8a6f..c32a36362cf3a 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java @@ -284,6 +284,11 @@ private String findUpstreamRef() { logger.warn("No remotes found. Using 'main' branch as upstream ref for transport version resources"); return "main"; } + // default the branch name to look at to that which a PR in CI is targeting + String branchName = System.getenv("BUILDKITE_PULL_REQUEST_BASE_BRANCH"); + if (branchName == null) { + branchName = "main"; + } List remoteNames = List.of(remotesOutput.split("\n")); if (remoteNames.contains(UPSTREAM_REMOTE_NAME) == false) { // our special remote doesn't exist yet, so create it @@ -299,14 +304,14 @@ private String findUpstreamRef() { gitCommand("remote", "add", UPSTREAM_REMOTE_NAME, upstreamUrl); } else { logger.warn("No elastic github remotes found to copy. Using 'main' branch as upstream ref for transport version resources"); - return "main"; + return branchName; } } // make sure the remote main ref is up to date - gitCommand("fetch", UPSTREAM_REMOTE_NAME, "main"); + gitCommand("fetch", UPSTREAM_REMOTE_NAME, branchName); - return UPSTREAM_REMOTE_NAME + "/main"; + return UPSTREAM_REMOTE_NAME + "/" + branchName; } // Return the transport version resources paths that exist in upstream 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 a344741d7ef88..6d2a78b68e275 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 @@ -66,6 +66,9 @@ public Path getResourcesDir() { @Input public abstract Property getCurrentUpperBoundName(); + @Input + public abstract Property getCI(); + private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+"); @ServiceReference("transportVersionResources") @@ -82,13 +85,14 @@ public void validateTransportVersions() throws IOException { Map upperBounds = resources.getUpperBounds(); TransportVersionUpperBound currentUpperBound = upperBounds.get(getCurrentUpperBoundName().get()); boolean onReleaseBranch = checkIfDefinitelyOnReleaseBranch(upperBounds); + boolean validateModifications = onReleaseBranch == false || getCI().get(); for (var definition : referableDefinitions.values()) { - validateNamedDefinition(definition, referencedNames); + validateNamedDefinition(definition, referencedNames, validateModifications); } for (var definition : unreferableDefinitions.values()) { - validateUnreferableDefinition(definition); + validateUnreferableDefinition(definition, validateModifications); } for (var entry : idsByBase.entrySet()) { @@ -101,10 +105,10 @@ public void validateTransportVersions() throws IOException { if (onReleaseBranch) { // on release branches we only check the current upper bound, others may be inaccurate - validateUpperBound(currentUpperBound, allDefinitions, idsByBase); + validateUpperBound(currentUpperBound, allDefinitions, idsByBase, validateModifications); } else { for (var upperBound : upperBounds.values()) { - validateUpperBound(upperBound, allDefinitions, idsByBase); + validateUpperBound(upperBound, allDefinitions, idsByBase, validateModifications); } validatePrimaryIds(resources, upperBounds, allDefinitions); @@ -126,7 +130,7 @@ private Map collectAllDefinitions( return allDefinitions; } - private void validateNamedDefinition(TransportVersionDefinition definition, Set referencedNames) { + private void validateNamedDefinition(TransportVersionDefinition definition, Set referencedNames, boolean validateModifications) { if (referencedNames.contains(definition.name()) == false) { throwDefinitionFailure(definition, "is not referenced"); } @@ -152,35 +156,39 @@ private void validateNamedDefinition(TransportVersionDefinition definition, Set< } } } - // validate any modifications - TransportVersionDefinition originalDefinition = getResources().get().getReferableDefinitionFromGitBase(definition.name()); - if (originalDefinition != null) { - validateIdenticalPrimaryId(definition, originalDefinition); - for (int i = 1; i < originalDefinition.ids().size(); ++i) { - TransportVersionId originalId = originalDefinition.ids().get(i); - - // we have a very small number of ids in a definition, so just search linearly - boolean found = false; - for (int j = 1; j < definition.ids().size(); ++j) { - TransportVersionId id = definition.ids().get(j); - if (id.base() == originalId.base()) { - found = true; - if (id.complete() != originalId.complete()) { - throwDefinitionFailure(definition, "has modified patch id from " + originalId + " to " + id); + + if (validateModifications) { + TransportVersionDefinition originalDefinition = getResources().get().getReferableDefinitionFromGitBase(definition.name()); + if (originalDefinition != null) { + validateIdenticalPrimaryId(definition, originalDefinition); + for (int i = 1; i < originalDefinition.ids().size(); ++i) { + TransportVersionId originalId = originalDefinition.ids().get(i); + + // we have a very small number of ids in a definition, so just search linearly + boolean found = false; + for (int j = 1; j < definition.ids().size(); ++j) { + TransportVersionId id = definition.ids().get(j); + if (id.base() == originalId.base()) { + found = true; + if (id.complete() != originalId.complete()) { + throwDefinitionFailure(definition, "has modified patch id from " + originalId + " to " + id); + } } } - } - if (found == false) { - throwDefinitionFailure(definition, "has removed id " + originalId); + if (found == false) { + throwDefinitionFailure(definition, "has removed id " + originalId); + } } } } } - private void validateUnreferableDefinition(TransportVersionDefinition definition) { - TransportVersionDefinition originalDefinition = getResources().get().getUnreferableDefinitionFromGitBase(definition.name()); - if (originalDefinition != null) { - validateIdenticalPrimaryId(definition, originalDefinition); + private void validateUnreferableDefinition(TransportVersionDefinition definition, boolean validateModifications) { + if (validateModifications) { + TransportVersionDefinition originalDefinition = getResources().get().getUnreferableDefinitionFromGitBase(definition.name()); + if (originalDefinition != null) { + validateIdenticalPrimaryId(definition, originalDefinition); + } } if (definition.ids().isEmpty()) { throwDefinitionFailure(definition, "does not contain any ids"); @@ -204,7 +212,8 @@ private void validateIdenticalPrimaryId(TransportVersionDefinition definition, T private void validateUpperBound( TransportVersionUpperBound upperBound, Map definitions, - Map> idsByBase + Map> idsByBase, + boolean validateModifications ) { TransportVersionDefinition upperBoundDefinition = definitions.get(upperBound.definitionName()); if (upperBoundDefinition == null) { @@ -240,13 +249,15 @@ private void validateUpperBound( ); } - TransportVersionUpperBound existingUpperBound = getResources().get().getUpperBoundFromGitBase(upperBound.name()); - if (existingUpperBound != null && getShouldValidatePrimaryIdNotPatch().get()) { - if (upperBound.definitionId().patch() != 0 && upperBound.definitionId().base() != existingUpperBound.definitionId().base()) { - throwUpperBoundFailure( - upperBound, - "modifies base id from " + existingUpperBound.definitionId().base() + " to " + upperBound.definitionId().base() - ); + if (validateModifications) { + TransportVersionUpperBound existingUpperBound = getResources().get().getUpperBoundFromGitBase(upperBound.name()); + if (existingUpperBound != null && getShouldValidatePrimaryIdNotPatch().get()) { + if (upperBound.definitionId().patch() != 0 && upperBound.definitionId().base() != existingUpperBound.definitionId().base()) { + throwUpperBoundFailure( + upperBound, + "modifies base id from " + existingUpperBound.definitionId().base() + " to " + upperBound.definitionId().base() + ); + } } } } From 9627fbfe86e4dde06a45252cd9c86e9ae3bcb8a9 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Wed, 8 Oct 2025 18:42:11 +0000 Subject: [PATCH 2/4] [CI] Auto commit changes from spotless --- .../transport/ValidateTransportVersionResourcesTask.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) 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 6d2a78b68e275..3420ab0cbb639 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 @@ -130,7 +130,11 @@ private Map collectAllDefinitions( return allDefinitions; } - private void validateNamedDefinition(TransportVersionDefinition definition, Set referencedNames, boolean validateModifications) { + private void validateNamedDefinition( + TransportVersionDefinition definition, + Set referencedNames, + boolean validateModifications + ) { if (referencedNames.contains(definition.name()) == false) { throwDefinitionFailure(definition, "is not referenced"); } @@ -252,7 +256,8 @@ private void validateUpperBound( if (validateModifications) { TransportVersionUpperBound existingUpperBound = getResources().get().getUpperBoundFromGitBase(upperBound.name()); if (existingUpperBound != null && getShouldValidatePrimaryIdNotPatch().get()) { - if (upperBound.definitionId().patch() != 0 && upperBound.definitionId().base() != existingUpperBound.definitionId().base()) { + if (upperBound.definitionId().patch() != 0 + && upperBound.definitionId().base() != existingUpperBound.definitionId().base()) { throwUpperBoundFailure( upperBound, "modifies base id from " + existingUpperBound.definitionId().base() + " to " + upperBound.definitionId().base() From ebb692aa5a05fe8867166ebee17fb725a9f6f690 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Thu, 9 Oct 2025 12:25:40 -0700 Subject: [PATCH 3/4] ignore empty PR branch --- .../internal/transport/TransportVersionResourcesService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java index c32a36362cf3a..4304168e4c5db 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java @@ -286,7 +286,7 @@ private String findUpstreamRef() { } // default the branch name to look at to that which a PR in CI is targeting String branchName = System.getenv("BUILDKITE_PULL_REQUEST_BASE_BRANCH"); - if (branchName == null) { + if (branchName == null || branchName.strip().isEmpty()) { branchName = "main"; } List remoteNames = List.of(remotesOutput.split("\n")); From 9ac9071264c769aa06b7a3038489b46977489290 Mon Sep 17 00:00:00 2001 From: elasticsearchmachine Date: Thu, 9 Oct 2025 22:48:59 +0000 Subject: [PATCH 4/4] [CI] Auto commit changes from spotless --- .../xpack/core/security/authc/AuthenticationTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java index 3462663266e9a..447140f2f9571 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java @@ -51,7 +51,6 @@ import static org.hamcrest.Matchers.hasKey; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.sameInstance;