Skip to content

Add --resolve-conflict flag to transport version generate task #134421

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 12, 2025
Merged

Add --resolve-conflict flag to transport version generate task #134421

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cf458cb
Add --update flag to transport version generate task
rjernst Sep 10, 2025
1f80cee
update flag name
rjernst Sep 10, 2025
e005705
iter
rjernst Sep 10, 2025
2dcec3d
[CI] Auto commit changes from spotless
Sep 10, 2025
1d264ff
fix flag in tests
rjernst Sep 11, 2025
99990c6
Merge branch 'main' into transport/generate_update_flag
rjernst Sep 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 71 additions & 0 deletions ...ovy/org/elasticsearch/gradle/internal/transport/TransportVersionGenerationFuncTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,77 @@ class TransportVersionGenerationFuncTest extends AbstractTransportVersionFuncTes
assertUpperBound("9.2", "second_tv,8124000")
}

def "update flag works with current"() {
given:
referableAndReferencedTransportVersion("new_tv", "8123000")
file("myserver/src/main/resources/transport/latest/9.2.csv").text =
"""
<<<<<<< HEAD
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The intention is that folks do a merge (address conflicts) and then run this task right? So we should never be in a position where we have conflicts in files? I mean either way, the content of the file is sort of irrelevant as we're just going to override it so the test is still valid as-is. Just want to understand the intent here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's the intention. You're right that the contents don't matter. I just made the test mimic the scenario as much as possible, and this is what it would like during a merge with a conflict.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think the dev shouldn't have to resolve the conflicts in the generated files at all. I assume this is the intention still? So if that's the only conflict they can just run this gradle task again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think the dev shouldn't have to resolve the conflicts in the generated files at all. I assume this is the intention still?

I don't think we can avoid that. They are going to have to resolve those conflicts just like any other. The thing is that it doesn't really matter how they are resolved since we'll just fix it.

existing_92,8123000
=======
new_tv,8123000
>>>>>> name
""".strip()

when:
def result = runGenerateAndValidateTask("--update").build()

then:
assertGenerateAndValidateSuccess(result)
assertReferableDefinition("existing_92", "8123000,8012001")
assertReferableDefinition("new_tv", "8124000")
assertUpperBound("9.2", "new_tv,8124000")
}

def "update flag works with multiple branches"() {
given:
referableAndReferencedTransportVersion("new_tv", "8123000,8012001,7123001")
file("myserver/src/main/resources/transport/latest/9.2.csv").text =
"""
<<<<<<< HEAD
existing_92,8123000
=======
new_tv,8123000
>>>>>> name
""".strip()
file("myserver/src/main/resources/transport/latest/9.1.csv").text =
"""
<<<<<<< HEAD
existing_92,8012001
=======
new_tv,8012001
>>>>>> name
""".strip()
file("myserver/src/main/resources/transport/latest/8.19.csv").text =
"""
<<<<<<< HEAD
initial_8.19.7,7123001
=======
new_tv,7123001
>>>>>> name
""".strip()

when:
def result = runGenerateAndValidateTask("--update").build()

then:
assertGenerateAndValidateSuccess(result)
assertReferableDefinition("existing_92", "8123000,8012001")
assertUnreferableDefinition("initial_8.19.7", "7123001")
assertReferableDefinition("new_tv", "8124000,8012002,7123002")
assertUpperBound("9.2", "new_tv,8124000")
assertUpperBound("9.1", "new_tv,8012002")
assertUpperBound("8.19", "new_tv,7123002")
}

def "update flag cannot be used with backport branches"() {
when:
def result = runGenerateTask("--update", "--backport-branches=9.1").buildAndFail()

then:
assertGenerateFailure(result, "Cannot use --update with --backport-branches")
}

def "branches param order does not matter"() {
given:
referencedTransportVersion("test_tv")
Expand Down
4 changes: 2 additions & 2 deletions ...java/org/elasticsearch/gradle/internal/transport/GenerateInitialTransportVersionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,13 @@ public void run() throws IOException {
var definition = new TransportVersionDefinition(initialDefinitionName, List.of(id));
resources.writeUnreferableDefinition(definition);
var newUpperBound = new TransportVersionUpperBound(upperBoundName, initialDefinitionName, id);
resources.writeUpperBound(newUpperBound);
resources.writeUpperBound(newUpperBound, false);

if (releaseVersion.getRevision() == 0) {
Version currentVersion = getCurrentVersion().get();
String currentUpperBoundName = getUpperBoundName(currentVersion);
var currentUpperBound = new TransportVersionUpperBound(currentUpperBoundName, initialDefinitionName, id);
resources.writeUpperBound(currentUpperBound);
resources.writeUpperBound(currentUpperBound, false);
}
}

Expand Down
51 changes: 46 additions & 5 deletions ...a/org/elasticsearch/gradle/internal/transport/GenerateTransportVersionDefinitionTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,11 @@ public abstract class GenerateTransportVersionDefinitionTask extends DefaultTask
@Option(option = "increment", description = "The amount to increment the id from the current upper bounds file by")
public abstract Property<Integer> getIncrement();

@Input
@Optional
@Option(option = "update", description = "Update the transport version currently being added to upstream")
Copy link
Contributor

@jdconrad jdconrad Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like @mark-vieira's suggestion of refresh better than update. Update implies we are working on something that already exists which the version being worked on doesn't.

Copy link
Member Author

@rjernst rjernst Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per a suggestion from @nik9000 I've changed this to --resolve-conflict since that is what a developers is trying to do, resolve a merge conflict

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh hey. Neat.

Copy link
Contributor

@mark-vieira mark-vieira Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per a suggestion from @nik9000 I've changed this to --resolve-conflict since that is what a developers is trying to do, resolve a merge conflict

I'm still not a fan, mainly because just running this task doesn't actually resolve the conflict. You still have to merge upstream changes and then run this task. I'll defer my objections though if that makes intuitive sense to most folks. I might be too close to this work to provide an objective opinion here.

public abstract Property<Boolean> getIsUpdate();

/**
* The name of the upper bounds file which will be used at runtime on the current branch. Normally
* this equates to VersionProperties.getElasticsearchVersion().
Expand All @@ -82,7 +87,7 @@ public void run() throws IOException {
String targetDefinitionName = getTargetDefinitionName(resources, referencedNames, changedDefinitionNames);

List<TransportVersionUpperBound> upstreamUpperBounds = resources.getUpperBoundsFromUpstream();
Set<String> targetUpperBoundNames = getTargetUpperBoundNames(upstreamUpperBounds);
Set<String> targetUpperBoundNames = getTargetUpperBoundNames(resources, upstreamUpperBounds, targetDefinitionName);

getLogger().lifecycle("Generating transport version name: " + targetDefinitionName);
if (targetDefinitionName.isEmpty()) {
Expand All @@ -108,6 +113,7 @@ private List<TransportVersionId> updateUpperBounds(
throw new IllegalArgumentException("Invalid increment " + increment + ", must be a positive integer");
}
List<TransportVersionId> ids = new ArrayList<>();
boolean stageInGit = getIsUpdate().getOrElse(false);

TransportVersionDefinition existingDefinition = resources.getReferableDefinitionFromUpstream(definitionName);
for (TransportVersionUpperBound existingUpperBound : existingUpperBounds) {
Expand All @@ -121,12 +127,12 @@ private List<TransportVersionId> updateUpperBounds(
int targetIncrement = upperBoundName.equals(currentUpperBoundName) ? increment : 1;
targetId = TransportVersionId.fromInt(existingUpperBound.definitionId().complete() + targetIncrement);
var newUpperBound = new TransportVersionUpperBound(upperBoundName, definitionName, targetId);
resources.writeUpperBound(newUpperBound);
resources.writeUpperBound(newUpperBound, stageInGit);
}
ids.add(targetId);
} else {
// Default case: we're not targeting this branch so reset it
resources.writeUpperBound(existingUpperBound);
resources.writeUpperBound(existingUpperBound, false);
}
}

Expand Down Expand Up @@ -167,7 +173,19 @@ private String getTargetDefinitionName(
}
}

private Set<String> getTargetUpperBoundNames(List<TransportVersionUpperBound> upstreamUpperBounds) {
private Set<String> getTargetUpperBoundNames(
TransportVersionResourcesService resources,
List<TransportVersionUpperBound> upstreamUpperBounds,
String targetDefinitionName
) throws IOException {
if (getIsUpdate().getOrElse(false)) {
if (getBackportBranches().isPresent()) {
throw new IllegalArgumentException("Cannot use --update with --backport-branches");
}

return getUpperBoundNamesFromDefinition(resources, upstreamUpperBounds, targetDefinitionName);
}

Set<String> targetUpperBoundNames = new HashSet<>();
targetUpperBoundNames.add(getCurrentUpperBoundName().get());
if (getBackportBranches().isPresent()) {
Expand All @@ -191,9 +209,32 @@ private Set<String> getTargetUpperBoundNames(List<TransportVersionUpperBound> up
return targetUpperBoundNames;
}

private Set<String> getUpperBoundNamesFromDefinition(
TransportVersionResourcesService resources,
List<TransportVersionUpperBound> upstreamUpperBounds,
String targetDefinitionName
) throws IOException {
TransportVersionDefinition definition = resources.getReferableDefinition(targetDefinitionName);
Set<String> upperBoundNames = new HashSet<>();
upperBoundNames.add(getCurrentUpperBoundName().get());

// skip the primary id as that is current, which we always add
for (int i = 1; i < definition.ids().size(); ++i) {
TransportVersionId id = definition.ids().get(i);
// we have a small number of upper bound files, so just scan for the ones we want
for (TransportVersionUpperBound upperBound : upstreamUpperBounds) {
if (upperBound.definitionId().base() == id.base()) {
upperBoundNames.add(upperBound.name());
}
}
}

return upperBoundNames;
}

private void resetAllUpperBounds(TransportVersionResourcesService resources) throws IOException {
for (TransportVersionUpperBound upperBound : resources.getUpperBoundsFromUpstream()) {
resources.writeUpperBound(upperBound);
resources.writeUpperBound(upperBound, false);
}
}

Expand Down
12 changes: 11 additions & 1 deletion ...in/java/org/elasticsearch/gradle/internal/transport/TransportVersionResourcesService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,12 @@ Map<String, TransportVersionDefinition> getReferableDefinitions() throws IOExcep
return readDefinitions(transportResourcesDir.resolve(REFERABLE_DIR));
}

/** Return a single referable definition by name */
TransportVersionDefinition getReferableDefinition(String name) throws IOException {
Path resourcePath = transportResourcesDir.resolve(getReferableDefinitionRelativePath(name));
return TransportVersionDefinition.fromString(resourcePath, Files.readString(resourcePath, StandardCharsets.UTF_8));
}

/** Get a referable definition from upstream if it exists there, or null otherwise */
TransportVersionDefinition getReferableDefinitionFromUpstream(String name) {
Path resourcePath = getReferableDefinitionRelativePath(name);
Expand Down Expand Up @@ -218,10 +224,14 @@ List<TransportVersionUpperBound> getUpperBoundsFromUpstream() throws IOException
}

/** Write the given upper bound to a file in the transport resources */
void writeUpperBound(TransportVersionUpperBound upperBound) throws IOException {
void writeUpperBound(TransportVersionUpperBound upperBound, boolean stageInGit) throws IOException {
Path path = transportResourcesDir.resolve(getUpperBoundRelativePath(upperBound.name()));
logger.debug("Writing upper bound [" + upperBound + "] to [" + path + "]");
Files.writeString(path, upperBound.definitionName() + "," + upperBound.definitionId().complete() + "\n", StandardCharsets.UTF_8);

if (stageInGit) {
gitCommand("add", path.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary? I mean we don't do this for any other kind of generation tasks. Can we not just rely on folks to do this themselves just as with any other local changes that need to be committed/pushed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary, but I think it's good. I don't think devs should think about the files that need to be added, when there is a conflict, they just run this command, and these files will no longer show as unresolved. Otherwise they would have to add themselves (or run git commit -a, but not everyone does that with merge conflicts).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean we don't do this for any other kind of generation tasks

We could consider doing this for other files...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I worry if we don't add the files it would actually be more confusing since they are absolutely necessary to run ES so I'm +1 on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have super strong feelings on this but I'm generally -1 on things that muck with folks' local git state. There's just so many unique workflows people use and we just don't know when we might be stepping on those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the question is "how magic is the tool/transport versions". We don't auto-stage spotlessApply because it's touching code we wrote. No magic. But we do auto-stage it when CI does spotlessApply for us.

If transport versions are managed by the tool almost entirely, then, yeah, staging is nice and useful.

As far as commit -a - whenever I get a merge conflict I manually git add each resolved file one by one on the command line. But I'm old now. I have no idea what most folks do.

I'd be really creeped out if it auto-staged files it isn't managing. But that's not this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If transport versions are managed by the tool almost entirely, then, yeah, staging is nice and useful.

That's a fair distinction. In which case to my previous comment, I would then expect it to auto stage for any changes it makes to files it manages, not just when passing --resolve-conflict.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just thinking maybe it should auto-stage for anything involving the internal transport version files because we really don't expect or want users to have to consider these at all.

Copy link
Contributor

@mark-vieira mark-vieira Sep 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we expect folks to understand all those distinctions. Basically we don't expect folks to ever manually touch any of these files. If that's the case, then I agree with Nik that the task should just "own" these files and handle staging them in all scenarios if that's what we're gonna do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

}
}

/** Return the path within the repository of the given latest */
Expand Down