Use PR base branch when checking transport version modifications (elastic#136220) (elastic#136405)

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.

Author: rjernst

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
+    }
 }

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesPlugin.java

@@ -14,18 +14,23 @@
 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;
 
 import java.util.Map;
 import java.util.Properties;
 
+import static org.elasticsearch.gradle.internal.util.ParamsUtils.loadBuildParams;
+
 public class TransportVersionResourcesPlugin implements Plugin<Project> {
 
     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<BuildParameterExtension> 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));
 

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<String> 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

build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/transport/ValidateTransportVersionResourcesTask.java

@@ -66,6 +66,9 @@ public Path getResourcesDir() {
     @Input
     public abstract Property<String> getCurrentUpperBoundName();
 
+    @Input
+    public abstract Property<Boolean> getCI();
+
     private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+");
 
     @ServiceReference("transportVersionResources")
@@ -82,13 +85,14 @@ public void validateTransportVersions() throws IOException {
         Map<String, TransportVersionUpperBound> 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<String, TransportVersionDefinition> collectAllDefinitions(
         return allDefinitions;
     }
 
-    private void validateNamedDefinition(TransportVersionDefinition definition, Set<String> referencedNames) {
+    private void validateNamedDefinition(
+        TransportVersionDefinition definition,
+        Set<String> 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<String, TransportVersionDefinition> definitions,
-        Map<Integer, List<IdAndDefinition>> idsByBase
+        Map<Integer, List<IdAndDefinition>> 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()
+                    );
+                }
             }
         }
     }