feedback

Author: rjernst

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

@@ -345,11 +345,12 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
         file("myserver/build.gradle") << """
             tasks.named('validateTransportVersionResources') {
                 shouldValidateDensity = false
+                shouldValidatePrimaryIdNotPatch = false
             }
         """
 
         when:
-        def result = runGenerateAndValidateTask("--backport-branches=9.2", "--increment=100").build()
+        def result = runGenerateAndValidateTask("--increment=100").build()
 
         then:
         assertGenerateAndValidateSuccess(result)

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

@@ -26,7 +26,7 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes
     def "definitions must be referenced"() {
         given:
         javaSource("myplugin", "org.elasticsearch.plugin", "MyPlugin",
-                "import org.elasticsearch.TransportVersion;", """
+            "import org.elasticsearch.TransportVersion;", """
             static final TransportVersion dne = TransportVersion.fromName("dne");
         """)
         when:

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

@@ -51,8 +51,8 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
     @Option(option = "name", description = "The name of the Transport Version definition, e.g. --name=my_new_tv")
     public abstract Property<String> getTransportVersionName();
 
-    @Optional
     @Input
+    @Optional
     @Option(
         option = "backport-branches",
         description = "The branches this definition will be backported to, e.g. --backport-branches=9.1,8.19"
@@ -85,9 +85,9 @@ public void run() throws IOException {
         checkReleaseBranches(mainUpperBounds, releaseBranches);
 
         if (name.isEmpty()) {
-            resetAllLatestFiles(resources);
+            resetAllUpperBounds(resources);
         } else {
-            List<TransportVersionId> ids = updateUpperBoundsFiles(resources, mainUpperBounds, releaseBranches, name);
+            List<TransportVersionId> ids = updateUpperBounds(resources, mainUpperBounds, releaseBranches, name);
             // (Re)write the definition file.
             resources.writeReferableDefinition(new TransportVersionDefinition(name, ids));
         }
@@ -111,7 +111,7 @@ private void checkReleaseBranches(List<TransportVersionUpperBound> mainUpperBoun
         }
     }
 
-    private List<TransportVersionId> updateUpperBoundsFiles(
+    private List<TransportVersionId> updateUpperBounds(
         TransportVersionResourcesService resources,
         List<TransportVersionUpperBound> existingUpperBounds,
         Set<String> targetReleaseBranches,
@@ -126,19 +126,11 @@ private List<TransportVersionId> updateUpperBoundsFiles(
 
         TransportVersionDefinition existingDefinition = resources.getReferableDefinitionFromMain(name);
         for (TransportVersionUpperBound existingUpperBound : existingUpperBounds) {
-
             String releaseBranch = existingUpperBound.releaseBranch();
-            boolean targetThisBranch = targetReleaseBranches.contains(existingUpperBound.releaseBranch());
 
-            // 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 (targetThisBranch) {
+            if (targetReleaseBranches.contains(releaseBranch)) {
                 // Case: targeting this branch, find an existing id for this branch if it exists
-                TransportVersionId targetId = maybeGetExistingId(existingUpperBound, existingDefinition);
+                TransportVersionId targetId = maybeGetExistingId(existingUpperBound, existingDefinition, name);
                 if (targetId == null) {
                     // Case: an id doesn't yet exist for this branch, so create one
                     int increment = releaseBranch.equals(mainReleaseBranch) ? primaryIncrement : 1;
@@ -167,7 +159,7 @@ private Set<String> getTargetReleaseBranches() {
         return releaseBranches;
     }
 
-    private void resetAllLatestFiles(TransportVersionResourcesService resources) throws IOException {
+    private void resetAllUpperBounds(TransportVersionResourcesService resources) throws IOException {
         // TODO: this should also _delete_ extraneous files from latest?
         for (TransportVersionUpperBound upperBound : resources.getUpperBoundsFromMain()) {
             resources.writeUpperBound(upperBound);
@@ -216,13 +208,20 @@ private String findAddedTransportVersionName(
         }
     }
 
-    private TransportVersionId maybeGetExistingId(TransportVersionUpperBound upperBound, TransportVersionDefinition existingDefinition) {
+    private TransportVersionId maybeGetExistingId(TransportVersionUpperBound upperBound, TransportVersionDefinition existingDefinition, String name) {
         if (existingDefinition == null) {
+            // the name doesn't yet exist, so there is no id to return
             return null;
         }
+        if (upperBound.name().equals(name)) {
+            // the name exists and this upper bound already points at it
+            return upperBound.id();
+        }
         if (upperBound.releaseBranch().equals(getMainReleaseBranch().get())) {
-            return existingDefinition.ids().get(0); // main is always the primary id
+            // the upper bound is for main, so return the primary id
+            return existingDefinition.ids().get(0);
         }
+        // the upper bound is for a non-main branch, so find the id with the same base
         for (TransportVersionId id : existingDefinition.ids()) {
             if (id.base() == upperBound.id().base()) {
                 return id;

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

@@ -9,57 +9,19 @@
 
 package org.elasticsearch.gradle.internal.transport;
 
-record TransportVersionId(int complete, int major, int server, int subsidiary, int patch) implements Comparable<TransportVersionId> {
-
-    /**
-     * The TV format:
-     * <p>
-     * MM_NNN_S_PP
-     * <p>
-     * M - The major version of Elasticsearch
-     * NNN - The server version part
-     * S - The subsidiary version part. It should always be 0 here, it is only used in subsidiary repositories.
-     * PP - The patch version part
-     */
-    public enum Component {
-        MAJOR(1_000_0_00, 2),
-        SERVER(1_0_00, 3),
-        SUBSIDIARY(1_00, 1),
-        PATCH(1, 2);
-
-        private final int value;
-        private final int max;
-
-        Component(int value, int numDigits) {
-            this.value = value;
-            this.max = (int) Math.pow(10, numDigits);
-        }
-    }
+record TransportVersionId(int complete, int base, int patch) implements Comparable<TransportVersionId> {
 
     public static TransportVersionId fromInt(int complete) {
-        int patch = complete % Component.PATCH.max;
-        int subsidiary = (complete / Component.SUBSIDIARY.value) % Component.SUBSIDIARY.max;
-        int server = (complete / Component.SERVER.value) % Component.SERVER.max;
-        int major = complete / Component.MAJOR.value;
+        int patch = complete % 1000;
+        int base = complete - patch;
 
-        return new TransportVersionId(complete, major, server, subsidiary, patch);
+        return new TransportVersionId(complete, base, patch);
     }
 
     static TransportVersionId fromString(String s) {
         return fromInt(Integer.parseInt(s));
     }
 
-    public TransportVersionId bumpComponent(Component component) {
-        int zeroesCleared = (complete / component.value) * component.value;
-        int newId = zeroesCleared + component.value;
-        if ((newId / component.value) % component.max == 0) {
-            throw new IllegalStateException(
-                "Insufficient" + component.name() + " version section in TransportVersion: " + complete + ", Cannot bump."
-            );
-        }
-        return fromInt(newId);
-    }
-
     @Override
     public int compareTo(TransportVersionId o) {
         // note: this is descending order so the arguments are reversed
@@ -70,8 +32,4 @@ public int compareTo(TransportVersionId o) {
     public String toString() {
         return Integer.toString(complete);
     }
-
-    public int base() {
-        return (complete / 1000) * 1000;
-    }
 }

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

@@ -57,6 +57,7 @@ public void apply(Project project) {
                 t.setDescription("Validates that all transport version resources are internally consistent with each other");
                 t.getReferencesFiles().setFrom(tvReferencesConfig);
                 t.getShouldValidateDensity().convention(true);
+                t.getShouldValidatePrimaryIdNotPatch().convention(true);
             });
         project.getTasks().named(LifecycleBasePlugin.CHECK_TASK_NAME).configure(t -> t.dependsOn(validateTask));
 

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

@@ -57,6 +57,9 @@ public Path getResourcesDir() {
     @Input
     public abstract Property<Boolean> getShouldValidateDensity();
 
+    @Input
+    public abstract Property<Boolean> getShouldValidatePrimaryIdNotPatch();
+
     private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {}
 
     private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+");
@@ -153,7 +156,7 @@ private void validateNamedDefinition(TransportVersionDefinition definition, Set<
             TransportVersionId id = definition.ids().get(ndx);
 
             if (ndx == 0) {
-                if (id.patch() != 0) {
+                if (getShouldValidatePrimaryIdNotPatch().get() && id.patch() != 0) {
                     throwDefinitionFailure(definition, "has patch version " + id.complete() + " as primary id");
                 }
             } else {