-
Notifications
You must be signed in to change notification settings - Fork 32
Update speculative build logic #2200
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
Mpdreamz
left a comment
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.
Thanks for catching this! One review comment
| var productVersion = versioningSystem.Current; | ||
| var previousMinorVersion = new SemVersion(productVersion.Major, Math.Max(productVersion.Minor - 1, 0), 0); | ||
| if (v >= productVersion) | ||
| if (v.Minor >= productVersion.Minor) |
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 think we should normalize the product version to Major.Minor this might cause issues if one of the majors differ.
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.
Thanks for jumping in on this!
Mpdreamz
left a comment
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.
Updated the assertion to match against the anchored product version (Major.Minor.0) and included a bunch more tests.
reakaleek
left a comment
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.
LGTM
In #2177, @Mpdreamz updated the logic to support speculative builds for the previous minor version. However, I think this resulted in losing support for speculative builds for the current minor version.
I'm seeing this in PRs like elastic/elastic-agent#11069 where before #2177 was merged the preview build worked as expected, but after it did not.
For the case of elastic/elastic-agent, today when the current version is
9.2.1I'd expect PR previews to build for9.1and9.2, but not for9.0. Before making the changes in this PR, I ranassembler content-source matchfor each minor version:✅
Results:9.0: Worked as expected (NO speculative build)✅
Results:9.1: Worked as expected (speculative build)❌
Results:9.2: Did NOT work as expected (NO speculative build)I think this is because in
if (v >= productVersion)we're comparing9.2.0to9.2.1when we should be checking that the branch (9.2) is greater than or equal to the latest minor (9.2.1➡️9.2).After adding the changes in this PR I reran
assembler content-source matchfor each minor version again and got the results I'd expect.I also added
fleet-serverto the product list so this logic will also work on PRs in elastic/fleet-server (example).