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 a344741d7ef88..3420ab0cbb639 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() + ); + } } } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java index 3462663266e9a..447140f2f9571 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/authc/AuthenticationTests.java @@ -51,7 +51,6 @@ import static org.hamcrest.Matchers.hasKey; import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.sameInstance;