-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Simulate BWC tests that start with the current version #39102
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.
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
Conversation
|
Pinging @elastic/es-core-infra |
|
I was too quick to submit this one as it has changes from #39059. Added the WIP label. |
Please ping when this is ready fro review again. |
|
@mark-vieira @jasontedor @rjernst This is now ready for review. The last test failure is being addressed by @jkakavas ( the test assumed that a master on current version will not be elected in I will not merge this before that fix is in, but I think we can start the review process. |
|
Can't provide a lot of insight here as I'm not completely aware of all the business rules with regards to versions. I wouldn't mind sitting down with someone to go over the differences between bugfix, maintenance, staged, etc as well as our definitions of wire-compatible vs index-compatible. |
|
@mark-vieira I am happy to do this! Please ping me tomorrow? |
| } | ||
| } | ||
|
|
||
| ['archives:windows-zip','archives:oss-windows-zip', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please explain this block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While reading the javadoc there was a bit of confusion, with regards to the following:
<li>the unreleased <b>bugfix</b>, M.N.c (c > 0) on the `M.b` branch</li>
I believe the branch in question here should be M.N as it's never explained what b is. As I understand it, work on 6.6.2 should be done in the 6.6 branch, yes?
We make a similar mention to an undefined b later in the javadoc:
* E.x when M.N.c is released M.N.c+1 is added to the Version class mentioned above in all the following branches:
* `M.b`, `M.x` and `master` so we can reliably assume that the leafs of the version tree are unreleased.
Again, I think we should refer to branch M.N rather than M.b.
| collect.forEach(uvi -> consumer.accept(uvi)); | ||
| } | ||
|
|
||
| private String getGradleProjectNameFor(Version version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we should rename this getGradleProjectPathFor() to be more pedantically accurate.
Similarly, UnreleasedVersionInfo.gradleProjectName should probably be gradleProjectPath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
renamed, makes much more sense.
| collect.forEach(uvi -> consumer.accept(uvi)); | ||
| } | ||
|
|
||
| private String getGradleProjectNameFor(Version version) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While @jasontedor and I were walking through this together we found it really hard to reason about this logic. I later read the top-level javadoc and that cleared a few things up but I think in general this method in particular could use some comments to guide the reader along. Can we add some comments to the logic in this method to better explain what is happening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a few comments and a pointer to the class javadoc
Fixed all of those, good catch, this was an oversight, it really should have been |
|
Will back-port build setup only, without enabling the tests to make it easier to back-port future build changes. |
This back-ports how versions are determined and bwc test are set up from elastic#39102 without enabling the bwc from current version tests so it's easier/possible to backmerge future buld changes. It's expected that the tets are lacking many of the required fixes in this version to enable them.
* Back port build changes from #39102 This back-ports how versions are determined and bwc test are set up from #39102 without enabling the bwc from current version tests so it's easier/possible to backmerge future buld changes. It's expected that the tets are lacking many of the required fixes in this version to enable them.
This changes does the following:
:distributionfor the current version:distributionnow emulates the configurations of a project that builds an unreleased version so bwc tests can use it.This change causes the current version to be included as an "old" version in bwc tests, such that bwc tests will now test a currentVersion to currentVersion upgrade.
This has the advantage of testing the plain restarts, but also makes the part of the test meant to run against the old cluster also run against the current version, this is important because in time the current version actually becomes the old version and suddenly we have a lot of test code that was never ran against it. This change makes sure that this is not the case and version bumps can be smooth.