Skip to content

Commit cbec989

Browse files
committed
Only validate primary ids on release branches (elastic#135044)
Primary ids are only incremented on the main branch. For release branches only patch ids will be created. The primary id validation won't always work on release branches because only some of the primary ids will be backported. This commit skips primary id validation if we are sure we are on a release branch. We can tell this by looking at the current upper bound compared to other upper bounds. If there is a newer one, we know we are on a release branch. If there isn't a newer one, we _might_ be on a release branch, or on main, but in this case doing primary id validation is ok because there's effectively no difference, we are looking at the latest upper bound.
1 parent 1af9b5d commit cbec989

File tree

4 files changed

+63
-7
lines changed

4 files changed

+63
-7
lines changed

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,9 @@ class AbstractTransportVersionFuncTest extends AbstractGradleFuncTest {
125125
tasks.named('generateTransportVersion') {
126126
currentUpperBoundName = '9.2'
127127
}
128+
tasks.named('validateTransportVersionResources') {
129+
currentUpperBoundName = '9.2'
130+
}
128131
"""
129132
referableAndReferencedTransportVersion("existing_91", "8012000")
130133
referableAndReferencedTransportVersion("older_92", "8122000")

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -277,4 +277,21 @@ class TransportVersionValidationFuncTest extends AbstractTransportVersionFuncTes
277277
"[myserver/src/main/resources/transport/definitions/referable/existing_92.csv]"
278278
)
279279
}
280+
281+
def "primary id checks skipped on release branch"() {
282+
given:
283+
file("myserver/build.gradle") << """
284+
tasks.named('validateTransportVersionResources') {
285+
currentUpperBoundName = '9.1'
286+
}
287+
"""
288+
referableAndReferencedTransportVersion("some_tv", "8125000")
289+
transportVersionUpperBound("9.2", "some_tv", "8125000")
290+
291+
when:
292+
def result = gradleRunner("validateTransportVersionResources").build()
293+
294+
then:
295+
result.task(":myserver:validateTransportVersionResources").outcome == TaskOutcome.SUCCESS
296+
}
280297
}

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

Lines changed: 23 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,9 @@
1212
import org.elasticsearch.gradle.Version;
1313
import org.elasticsearch.gradle.internal.ProjectSubscribeServicePlugin;
1414
import org.elasticsearch.gradle.internal.conventions.VersionPropertiesPlugin;
15+
import org.elasticsearch.gradle.internal.conventions.precommit.PrecommitPlugin;
16+
import org.elasticsearch.gradle.internal.conventions.precommit.PrecommitTaskPlugin;
17+
import org.gradle.api.Action;
1518
import org.gradle.api.Plugin;
1619
import org.gradle.api.Project;
1720
import org.gradle.api.file.Directory;
@@ -30,6 +33,7 @@ public class TransportVersionResourcesPlugin implements Plugin<Project> {
3033
public void apply(Project project) {
3134
project.getPluginManager().apply(LifecycleBasePlugin.class);
3235
project.getPluginManager().apply(VersionPropertiesPlugin.class);
36+
project.getPluginManager().apply(PrecommitTaskPlugin.class);
3337
var psService = project.getPlugins().apply(ProjectSubscribeServicePlugin.class).getService();
3438

3539
Properties versions = (Properties) project.getExtensions().getByName(VersionPropertiesPlugin.VERSIONS_EXT);
@@ -66,8 +70,9 @@ public void apply(Project project) {
6670
t.getReferencesFiles().setFrom(tvReferencesConfig);
6771
t.getShouldValidateDensity().convention(true);
6872
t.getShouldValidatePrimaryIdNotPatch().convention(true);
73+
t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor());
6974
});
70-
project.getTasks().named(LifecycleBasePlugin.CHECK_TASK_NAME).configure(t -> t.dependsOn(validateTask));
75+
project.getTasks().named(PrecommitPlugin.PRECOMMIT_TASK_NAME).configure(t -> t.dependsOn(validateTask));
7176

7277
var generateManifestTask = project.getTasks()
7378
.register("generateTransportVersionManifest", GenerateTransportVersionManifestTask.class, t -> {
@@ -76,19 +81,31 @@ public void apply(Project project) {
7681
t.getManifestFile().set(project.getLayout().getBuildDirectory().file("generated-resources/manifest.txt"));
7782
});
7883
project.getTasks().named(JavaPlugin.PROCESS_RESOURCES_TASK_NAME, Copy.class).configure(t -> {
79-
t.into("transport/definitions", c -> c.from(generateManifestTask));
84+
t.into(resourceRoot + "/definitions", c -> c.from(generateManifestTask));
8085
});
8186

87+
Action<GenerateTransportVersionDefinitionTask> generationConfiguration = t -> {
88+
t.setGroup(taskGroup);
89+
t.getReferencesFiles().setFrom(tvReferencesConfig);
90+
t.getIncrement().convention(1000);
91+
t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor());
92+
};
93+
8294
var generateDefinitionsTask = project.getTasks()
8395
.register("generateTransportVersion", GenerateTransportVersionDefinitionTask.class, t -> {
84-
t.setGroup(taskGroup);
8596
t.setDescription("(Re)generates a transport version definition file");
86-
t.getReferencesFiles().setFrom(tvReferencesConfig);
87-
t.getIncrement().convention(1000);
88-
t.getCurrentUpperBoundName().convention(currentVersion.getMajor() + "." + currentVersion.getMinor());
8997
});
98+
generateDefinitionsTask.configure(generationConfiguration);
9099
validateTask.configure(t -> t.mustRunAfter(generateDefinitionsTask));
91100

101+
var resolveConflictTask = project.getTasks()
102+
.register("resolveTransportVersionConflict", GenerateTransportVersionDefinitionTask.class, t -> {
103+
t.setDescription("Resolve merge conflicts in transport version internal state files");
104+
t.getResolveConflict().set(true);
105+
});
106+
resolveConflictTask.configure(generationConfiguration);
107+
validateTask.configure(t -> t.mustRunAfter(resolveConflictTask));
108+
92109
var generateInitialTask = project.getTasks()
93110
.register("generateInitialTransportVersion", GenerateInitialTransportVersionTask.class, t -> {
94111
t.setGroup(taskGroup);

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

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,13 @@ public Path getResourcesDir() {
6060
@Input
6161
public abstract Property<Boolean> getShouldValidatePrimaryIdNotPatch();
6262

63+
/**
64+
* The name of the upper bounds file which will be used at runtime on the current branch. Normally
65+
* this equates to VersionProperties.getElasticsearchVersion().
66+
*/
67+
@Input
68+
public abstract Property<String> getCurrentUpperBoundName();
69+
6370
private record IdAndDefinition(TransportVersionId id, TransportVersionDefinition definition) {}
6471

6572
private static final Pattern NAME_FORMAT = Pattern.compile("[a-z0-9_]+");
@@ -76,6 +83,7 @@ public void validateTransportVersions() throws IOException {
7683
Map<String, TransportVersionDefinition> allDefinitions = collectAllDefinitions(referableDefinitions, unreferableDefinitions);
7784
Map<Integer, List<IdAndDefinition>> idsByBase = collectIdsByBase(allDefinitions.values());
7885
Map<String, TransportVersionUpperBound> upperBounds = resources.getUpperBounds();
86+
boolean onReleaseBranch = checkIfDefinitelyOnReleaseBranch(upperBounds);
7987

8088
for (var definition : referableDefinitions.values()) {
8189
validateNamedDefinition(definition, referencedNames);
@@ -93,7 +101,9 @@ public void validateTransportVersions() throws IOException {
93101
validateUpperBound(upperBound, allDefinitions, idsByBase);
94102
}
95103

96-
validatePrimaryIds(resources, upperBounds, allDefinitions);
104+
if (onReleaseBranch == false) {
105+
validatePrimaryIds(resources, upperBounds, allDefinitions);
106+
}
97107
}
98108

99109
private Map<String, TransportVersionDefinition> collectAllDefinitions(
@@ -318,6 +328,15 @@ private void validatePrimaryIds(
318328
);
319329
}
320330

331+
private boolean checkIfDefinitelyOnReleaseBranch(Map<String, TransportVersionUpperBound> upperBounds) {
332+
// only want to look at definitions <= the current upper bound.
333+
// TODO: we should filter all of the upper bounds/definitions that are validated by this, not just in this method
334+
String currentUpperBoundName = getCurrentUpperBoundName().get();
335+
TransportVersionUpperBound currentUpperBound = upperBounds.get(currentUpperBoundName);
336+
337+
return upperBounds.values().stream().anyMatch(u -> u.definitionId().complete() > currentUpperBound.definitionId().complete());
338+
}
339+
321340
private void throwDefinitionFailure(TransportVersionDefinition definition, String message) {
322341
Path relativePath = getResources().get().getDefinitionPath(definition);
323342
throw new VerificationException("Transport version definition file [" + relativePath + "] " + message);

0 commit comments

Comments
 (0)