tests complete

Author: rjernst

build-tools-internal/src/integTest/groovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy

@@ -9,14 +9,10 @@
 
 package org.elasticsearch.gradle.internal.transport
 
-import org.elasticsearch.gradle.Version
-import org.elasticsearch.gradle.VersionProperties
-import org.elasticsearch.gradle.fixtures.WiremockFixture
+
 import org.gradle.testkit.runner.BuildResult
 import org.gradle.testkit.runner.TaskOutcome
 
-import java.nio.charset.StandardCharsets
-
 class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTest {
 
     def runGenerateAndValidateTask(String... additionalArgs) {
@@ -94,7 +90,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertUpperBound("9.2", "new_tv,8124000")
     }
 
-    def "invalid changes to a latest file should be reverted"() {
+    def "invalid changes to a upper bounds should be reverted"() {
         given:
         transportVersionUpperBound("9.2", "modification", "9000000")
 
@@ -106,7 +102,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertUpperBound("9.2", "existing_92,8123000")
     }
 
-    def "invalid changes to multiple latest files should be reverted"() {
+    def "invalid changes to multiple upper bounds should be reverted"() {
         given:
         transportVersionUpperBound("9.2", "modification", "9000000")
         transportVersionUpperBound("9.1", "modification", "9000000")
@@ -198,8 +194,9 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertUpperBound("9.2", "test_tv,8124000")
     }
 
-    def "a latest file can be regenerated"() {
+    def "an upper bound can be regenerated"() {
         given:
+        file("myserver/src/main/resources/transport/upper_bounds/9.2.csv").delete()
         referableAndReferencedTransportVersion("test_tv", "8124000")
 
         when:
@@ -310,7 +307,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertUpperBound("9.2", "test_tv,8124000")
     }
 
-    def "can add backport to an existing definition"() {
+    def "can add backport to latest upper bound"() {
         when:
         def result = runGenerateAndValidateTask("--name=existing_92", "--backport-branches=9.1,9.0").build()
 
@@ -322,17 +319,24 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         assertUpperBound("9.0", "existing_92,8000001")
     }
 
-    def "increment must be positive"() {
+    def "can add backport to older definition"() {
         given:
-        referencedTransportVersion("new_tv")
+        execute("git checkout main")
+        referableAndReferencedTransportVersion("latest_tv", "8124000,8012002")
+        transportVersionUpperBound("9.2", "latest_tv", "8124000")
+        transportVersionUpperBound("9.1", "latest_tv", "8012002")
+        execute("git commit -a -m added")
+        execute("git checkout -b mybranch")
 
         when:
-        def result = runGenerateTask("--increment=0").buildAndFail()
+        def result = runGenerateAndValidateTask("--name=existing_92", "--backport-branches=9.1,9.0").build()
 
         then:
-        assertGenerateFailure(result, "Invalid increment [0], must be a positive integer")
-        assertReferableDefinitionDoesNotExist("new_tv")
-        assertUpperBound("9.2", "existing_92,8123000")
+        assertGenerateAndValidateSuccess(result)
+        assertReferableDefinition("existing_92", "8123000,8012001,8000001")
+        assertUpperBound("9.2", "latest_tv,8124000")
+        assertUpperBound("9.1", "latest_tv,8012002")
+        assertUpperBound("9.0", "existing_92,8000001")
     }
 
     def "a different increment can be specified"() {
@@ -361,7 +365,7 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         def result = runGenerateTask("--increment=0").buildAndFail()
 
         then:
-        assertOutputContains(result.output, "Invalid increment: must be a positive integer > 0")
+        assertOutputContains(result.output, "Invalid increment 0, must be a positive integer")
     }
 
     def "a new definition exists and is in the latest file, but the version id is wrong and needs to be updated"(){

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

@@ -120,51 +120,36 @@ private List<TransportVersionId> updateUpperBoundsFiles(
         String mainReleaseBranch = getMainReleaseBranch().get();
         int primaryIncrement = getPrimaryIncrement().get();
         if (primaryIncrement <= 0) {
-            throw new IllegalArgumentException("Invalid increment [" + primaryIncrement + "], must be a positive integer");
+            throw new IllegalArgumentException("Invalid increment " + primaryIncrement + ", must be a positive integer");
         }
         List<TransportVersionId> ids = new ArrayList<>();
 
         TransportVersionDefinition existingDefinition = resources.getReferableDefinitionFromMain(name);
         for (TransportVersionUpperBound existingUpperBound : existingUpperBounds) {
 
-            TransportVersionUpperBound upperBoundToWrite = existingUpperBound;
+            String releaseBranch = existingUpperBound.releaseBranch();
+            boolean targetThisBranch = targetReleaseBranches.contains(existingUpperBound.releaseBranch());
 
-            // LOGIC
-            /*
-              - name may or may not already be set in the existing upper bound
-              - target release branches may or may not target the upper bound file
-             */
-
-            resources.writeUpperBound(upperBoundToWrite);
+            // Case: Targeting this upper bound file and it already points at the name we're generating
+            if (targetThisBranch && name.equals(existingUpperBound.name())) {
+                ids.add(existingUpperBound.id());
+                continue;
+            }
 
-            if (name.equals(existingUpperBound.name())) {
-                if (targetReleaseBranches.contains(existingUpperBound.releaseBranch()) == false) {
-                    // Here, we aren't targeting this latest file, but need to undo prior updates if the list of minor
-                    // versions has changed. We must regenerate this latest file to make this operation idempotent.
-                    resources.writeUpperBound(existingUpperBound);
-                } else {
-                    ids.add(existingUpperBound.id());
+            if (targetThisBranch) {
+                // Case: targeting this branch, find an existing id for this branch if it exists
+                TransportVersionId targetId = maybeGetExistingId(existingUpperBound, existingDefinition);
+                if (targetId == null) {
+                    // Case: an id doesn't yet exist for this branch, so create one
+                    int increment = releaseBranch.equals(mainReleaseBranch) ? primaryIncrement : 1;
+                    targetId = TransportVersionId.fromInt(existingUpperBound.id().complete() + increment);
+                    var newUpperBound = new TransportVersionUpperBound(releaseBranch, name, targetId);
+                    resources.writeUpperBound(newUpperBound);
                 }
+                ids.add(targetId);
             } else {
-                if (targetReleaseBranches.contains(existingUpperBound.releaseBranch())) {
-                    TransportVersionId targetId = null;
-                    if (existingDefinition != null) {
-                        for (TransportVersionId id : existingDefinition.ids()) {
-                            if (id.base() == existingUpperBound.id().base()) {
-                                targetId = id;
-                                break;
-                            }
-                        }
-                    }
-                    if (targetId == null) {
-                        int increment = existingUpperBound.releaseBranch().equals(mainReleaseBranch) ? primaryIncrement : 1;
-                        TransportVersionId id = TransportVersionId.fromInt(existingUpperBound.id().complete() + increment);
-                        ids.add(id);
-                        resources.writeUpperBound(new TransportVersionUpperBound(existingUpperBound.releaseBranch(), name, id));
-                    }
-                } else {
-                    resources.writeUpperBound(existingUpperBound);
-                }
+                // Default case: we're not targeting this branch so reset it
+                resources.writeUpperBound(existingUpperBound);
             }
         }
 
@@ -230,4 +215,20 @@ private String findAddedTransportVersionName(
             }
         }
     }
+
+    private TransportVersionId maybeGetExistingId(TransportVersionUpperBound upperBound, TransportVersionDefinition existingDefinition) {
+        if (existingDefinition == null) {
+            return null;
+        }
+        if (upperBound.releaseBranch().equals(getMainReleaseBranch().get())) {
+            return existingDefinition.ids().get(0); // main is always the primary id
+        }
+        for (TransportVersionId id : existingDefinition.ids()) {
+            if (id.base() == upperBound.id().base()) {
+                return id;
+            }
+        }
+        return null; // no id for this release branch
+    }
+
 }