From 21af248622c995ec5be3ef1e62ff691216074693 Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Fri, 10 Oct 2025 07:35:23 -0700 Subject: [PATCH] Use PR base branch when checking transport version modifications (#136220) 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 | 86 +++++++++++-------- 4 files changed, 107 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..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 @@ -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.strip().isEmpty()) { + 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 82b52e7895918..a7f1a18cb4165 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,11 @@ 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 +160,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 +216,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 +253,16 @@ 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() + ); + } } } }