Fix logic to determine unreleased versions#39059
Fix logic to determine unreleased versions#39059alpar-t wants to merge 1 commit intoelastic:masterfrom
Conversation
Arround this time we have a bugfix that's too majors behind master. The logic to determine unreleased versions was not accounting for this. The logic didn't account to have two unreleased minors 6.7 and 7.1 and a staged minor 7.0, thus the change to only consider index compatible versions. This is fine since the point of the logic is to be able to set up BWC tests we will not care for mote than one major behind and as such will have fewer versions to map to gradle projects making the logic valid again.
|
Pinging @elastic/es-core-infra |
|
What is the actual problem trying to be solved? Master should not need to know anything about 6.7, from the bwc compat standpoint. I think making version collection know about 3 major versions should not be necessary. Instead, we could ignore these older versions when reading Version.java? |
|
@rjernst the problem being solved is |
| } | ||
|
|
||
| public List<Version> getUnreleased() { | ||
| protected List<Version> getUnreleased() { |
There was a problem hiding this comment.
We reduced visibility on this, but we still use this method if a couple of places outside this class and its tests. We now have this weirdness where VersionCollections.unreleased and VersionCollections.getUnreleased are different sets. Maybe we should rename the former to make it clear it includes on index compatible versions? We might also consider refactoring external usages.
| } | ||
| } | ||
|
|
||
| // Special case when we are about to release a new major, we have branched and incremented versions |
There was a problem hiding this comment.
As @rjernst mentioned, what do we actually use these versions for? They are never included in any of the BWC tests. Basically, if an unreleased version isn't wire or index compatible it's kind of irrelevant isn't it?
mark-vieira
left a comment
There was a problem hiding this comment.
I have some concerns that the logic in the class, which is already pretty complex and difficult to reason about, is starting to balloon even more. It might be worth reexamining its true purpose (which is to determine the BWC test matrix) and perhaps do some refactoring to simplify it to that use case rather than attempt to be a generic repository of all ES version info.
|
@mark-vieira @rjernst the reason I kept these version around is that I want to implement a new check for PRs against older branches that tests forwards comparability. I completely agree on the complexity and need of refactor and all for it, but would like to keep it separate from this PR ( I will look at |
|
Closing this in favor of a simpler approach in #39624 |
Around this time we have a bugfix that's too majors behind master.
The logic to determine unreleased versions was not accounting for this.
The logic didn't account to have two unreleased minors 6.7 and 7.1 and
a staged minor 7.0, thus the change to only consider index compatible
versions.
This is fine since the point of the logic is to be able to set up BWC
tests we will not care for mote than one major behind and as such will
have fewer versions to map to gradle projects making the logic valid
again.
Since master is not index compatible with the versions the build previously failed to identify, the bwc tests were not affected.
Closes #38708