Add test to ensure backports from 9.1 are done correctly #122016
Conversation
ldematte
added
>test
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Could this be made generic to the branch it is on? Maybe using information in the version lookup csv? Then it can be backported to all extant branches |
Isn't that self-fulfilling? AFAIK the version lookup csv is generated from TransportVersions, so adding a new version will add the entry to the CSV file? (or does this happen only during the release process, so it's guaranteed the file is "frozen"?) |
When we tag the git commit for the release, we add the mapping to the csv. For the current release, we use the current build version. But I'm not sure it will help here since we need to know the "base" version for the branch. I think the assertion you want is "is the latest version a patch of the one this branch started with". Here you hardcode the first 9.0 constant. I don't know how we could do that without explicitly marking the constant (eg similar to min compat constant), but if we had such a constant, the test could rely on it. |
| */ | ||
| public void testMaximumAllowedTransportVersion() { | ||
| assertThat( | ||
| TransportVersions.LATEST_DEFINED.onOrBefore(TransportVersions.ELASTICSEARCH_9_0) |
There was a problem hiding this comment.
What is this check achieving? The latest will never be before a known transport constant, and if it would only be equal if there are no patches yet, but isPatchFrom would return true in that case anyways, so I think this is redundant.
That wouldn't have caught the issue with 9.0 though, because it's not yet a released version, therefore no such entry in the CSV would exit, right? |
|
I struggle to see how we can implement such a check in a way that relies only on inferences drawn from information in the current branch. It seems to me that by definition, a messed up backport, requires some information about forward branches. That doesn't rule out us implementing such a safeguard, but I don't think a standalone unit test in any given branch will provide us the safety we're looking for here. |
|
If we knew the current branch was a release branch, then I think the check here will work. The intent is to ensure any new versions added are patches. If we "stash" the transport version at branching time (or, as we will soon do, create a new transport version at branching) as a constant similar to min compat version, then we can assert, as this test does, that the latest version is a patch of that stashed version. I do agree that it's not complete, in that someone could still incorrectly add an earlier transport version. For that, having a cross branch check would be great, to validate the other views of what transport versions exist don't conflict with what we have locally. |
Right. And we do know this. Essentially if the current version number doesn't end with |
We might also have patches before the first .0 is released, like we do not with 9.0.0. |
|
Yup. For "staged" branches this becomes more complicated.
…On Fri, Feb 7, 2025, 4:11 PM Ryan Ernst ***@***.***> wrote:
Essentially if the current version number doesn't end with 0
We might also have patches before the first .0 is released, like we do not
with 9.0.0.
—
Reply to this email directly, view it on GitHub
<#122016 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA7KTMBGTUJGRFDPLRWHKVD2OVDUFAVCNFSM6AAAAABWVWZIYWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDMNBUGM2TOMRSGY>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
|
I have the same feeling, that the information on the CSV is not "right" for this case.
++ |
SGTM, let's protect these immediately, and figure out as a followup how to ensure we don't need to keep changing the test on each new release branch creation. |
…ticsearch into max-transport-version-test
This PR adds a test to ensure that backports with a transport version change address this correctly, by creating a new transport version in the target branch.