Make packer cache branches explicit #41990
Conversation
Before this change we would recurse to cache bwc versions. This proved to be problematic due to the number of steps it was generating taking too long. Also this required tricky maintenance to break the recursion for old branches we don't really care about. With this change we now cache specific branches only.
|
Pinging @elastic/es-core-infra |
|
@elasticmachine run elasticsearch-ci/1 |
|
I don't quite follow the motivation for this change. With this change we have yet another place in CI where we are manually tracking branches and needs to be updated when we do major version bumps. This is starting to get out of hand IMO. We really, really, really should be avoiding having hard-coded branches all over the place. From what I understand the existing logic is using |
|
The previus implementation also requires maintenance as we create new versions, we are not interested to cache some of the old versions, so we have to update them to break the recursion. I think due to the hidden non obvious nature that's worse than calling these out. We are also doing less work now, because with bwc versions we would do it for both I fully agree on dealing with the hard coded version names and finding a solution for that, but that will neither be easy nor quick, and right now the CI images are really old increasing the chances of network failures. |
|
run elasticsearch-ci/1 |
We have the known versions though. We can make the I think this is just another example that our version/branch/bwc logic is starting to get mind bendingly complex. We need to get this under control because I feel like half of the issues we have with CI jobs suddenly failing or acting funny are around release version handling. |
Without hard coding version numbers
|
@run elasticsearch-ci/1 |
|
@mark-vieira ready for another review |
| @@ -124,4 +124,7 @@ public int compareTo(Version other) { | |||
| return Integer.compare(getId(), other.getId()); | |||
| } | |||
|
|
|||
| public boolean isNextMajor() { | |||
| return getRevision() == 0 && getMinor() == 0; | |||
There was a problem hiding this comment.
This doesn't seem right. A version string without context cannot possibly determine if it is the next major version. This should really live in BwcVersions. For example Version.fromString("7.0.0").isNextMajor() would return true which is obviously incorrect.
There was a problem hiding this comment.
The method shouldn't exist probably, and we should just do the check directly.
BWC versions doesn't know about newer versions, e.x on 6.8 there's no way to know how far we'we got. That being said we have used x.0.0 to identify master, as we will never build ex 7.0.0 as soon as it's released we version bump and the branch becomes 7.0.1. This would break down only when we have a major staged (as we had before 7.0 was released but the 7.x branch cut). This would I think be acceptable for this use-case (without the isNextMajor method of course). WDYT @mark-vieira ?
There was a problem hiding this comment.
I'd be ok with renaming this something more pedantically accurate like isNewMajor().
|
@atorok does this actually need to be backported? As far as I can tell the packer images checkout only |
…sertion * elastic/master: Make packer cache branches explicit (elastic#41990) Re-mute all ml_datafeed_crud rolling upgrade tests Ensure testAckedIndexing uses disruption index settings add 7_3 as version (elastic#42368) Fix grammar problem in stemming reference. (elastic#42148)
…search-remote-settings * elastic/master: Make packer cache branches explicit (elastic#41990) Re-mute all ml_datafeed_crud rolling upgrade tests Ensure testAckedIndexing uses disruption index settings add 7_3 as version (elastic#42368) Fix grammar problem in stemming reference. (elastic#42148)
* elastic/master: Make packer cache branches explicit (elastic#41990) Re-mute all ml_datafeed_crud rolling upgrade tests Ensure testAckedIndexing uses disruption index settings add 7_3 as version (elastic#42368) Fix grammar problem in stemming reference. (elastic#42148)
|
@mark-vieira yes, so the bwc checkouts don't recourse too much when resolving dependencies. I'm going to do the back-ports now. |
Before this change we would recurse to cache bwc versions. This proved to be problematic due to the number of steps it was generating taking too long. Also this required tricky maintenance to break the recursion for old branches we don't really care about. With this change we now cache specific branches only.
Before this change we would recurse to cache bwc versions. This proved to be problematic due to the number of steps it was generating taking too long. Also this required tricky maintenance to break the recursion for old branches we don't really care about. With this change we now cache specific branches only.
Before this change we would recurse to cache bwc versions. This proved to be problematic due to the number of steps it was generating taking too long. Also this required tricky maintenance to break the recursion for old branches we don't really care about. With this change we now cache specific branches only.
|
@mark-vieira that is the only place that calls the script, but we do bwc checkouts as part of that when run on master and call |
|
Ah, of course. I understand now. |
Before this change we would recurse to cache bwc versions.
This proved to be problematic due to the number of steps it was
generating taking too long.
Also this required tricky maintenance to break the recursion for old
branches we don't really care about.
With this change we now cache specific branches only.