Fix Detect Breaking Changes version selection logic for previous released version

[cwperks]

Description

Currently the logic for Detect Breaking Changes simply selects the current released version of OpenSearch as the version to test against for breaking changes.

This PR updates the logic to be more intelligent and selects the last minor version.

This will resolve issues like #21434 (comment) where backports against the 3.5 branch are choosing the wrong version (i.e. 3.6 instead of 3.4) to test against with the Detect Breaking Changes CI check.

Related Issues

Fixes issues as seen in #21434 (comment)

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit cd1512a)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incorrect Filtering The filter v -> v.before(current) may not work correctly when currentVersion is a pre-release or SNAPSHOT version (e.g., "4.0.0-beta1"). The Version.fromString may parse "4.0.0-beta1" as 4.0.0, causing versions equal to 4.0.0 to be excluded. The test returnsNullForPreReleaseOfInitialMajor passes, but the behavior for cases like "4.0.1-beta1" with "4.0.0" in the list should be validated — it's unclear if before handles qualifiers correctly. .filter(v -> v.before(current)) Missing Test Case There is no test covering the case where currentVersion itself appears in releasedVersions (e.g., latestPriorReleasedVersion("3.5.0", List.of("3.4.0", "3.5.0"))). The v.before(current) filter should exclude the current version, but this edge case is not explicitly tested to confirm the behavior. .filter(v -> v.before(current)) .sorted(Comparator.naturalOrder()) .collect(Collectors.toList()); Null Safety If releasedVersions is null, the stream call will throw a NullPointerException. Consider adding a null/empty guard or documenting that callers must provide a non-null list. List<Version> candidates = releasedVersions.stream()

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to cd1512a

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Guard against invalid current version input If currentVersion is a pre-release string like "4.0.0-beta1" or "3.6.0-SNAPSHOT", Version.fromString may throw or produce unexpected results. The test returnsNullForPreReleaseOfInitialMajor passes "4.0.0-beta1" as currentVersion, so this path must be handled. Add null/exception handling or input validation before calling Version.fromString on the current version. buildSrc/src/main/java/org/opensearch/gradle/VersionSelection.java [25] +if (currentVersion == null || !currentVersion.matches("\\d+\\.\\d+\\.\\d+.*")) { + return null; +} Version current = Version.fromString(currentVersion); Suggestion importance[1-10]: 5 __ Why: The test returnsNullForPreReleaseOfInitialMajor passes "4.0.0-beta1" and handlesSnapshotVersion passes "3.6.0-SNAPSHOT" as currentVersion, so Version.fromString must handle these inputs. Adding a guard or validation before calling Version.fromString is a reasonable defensive measure, though the improved code's regex "\\d+\\.\\d+\\.\\d+.*" would still match pre-release versions, making the guard incomplete for the stated concern.	Low
Possible issue	Clarify sort order for latest version selection The list is sorted in natural (ascending) order, and the last element is taken as the latest prior version. However, if Comparator.naturalOrder() sorts in descending order for Version, the last element would be the smallest, not the largest. To make the intent explicit and correct regardless of the Version comparator direction, sort in ascending order explicitly or use Comparator.reverseOrder() and take the first element. Verify that Version implements Comparable in ascending order to ensure correctness. buildSrc/src/main/java/org/opensearch/gradle/VersionSelection.java [31-32] .filter(v -> v.before(current)) -.sorted(Comparator.naturalOrder()) +.sorted(Comparator.reverseOrder()) Suggestion importance[1-10]: 3 __ Why: The suggestion asks to change Comparator.naturalOrder() to Comparator.reverseOrder(), but the current code correctly takes the last element of an ascending-sorted list to get the maximum. The existing logic is correct if Version implements Comparable in ascending order, which is the standard convention. The suggestion introduces an unnecessary change that would break the logic unless paired with changing the retrieval index.	Low
	Align retrieval index with sort order If the sort order is ascending (natural order), candidates.get(candidates.size() - 1) correctly returns the largest element. However, if the sort is changed to descending (as suggested), this should be candidates.get(0). Align the retrieval index with the chosen sort order to avoid returning the wrong version. buildSrc/src/main/java/org/opensearch/gradle/VersionSelection.java [38] -return candidates.get(candidates.size() - 1).toString(); +return candidates.get(0).toString(); Suggestion importance[1-10]: 1 __ Why: This suggestion is contingent on suggestion 1 being applied (changing to Comparator.reverseOrder()). Since the current code correctly uses ascending sort and takes the last element, this change alone would return the smallest version instead of the largest, making it incorrect in isolation.	Low

---

Previous suggestions

Suggestions up to commit 05f82a5

Category	Suggestion	Impact
Possible issue	Normalize current version before parsing The currentVersion string may itself contain a qualifier (e.g., "3.0.0-beta1"), but it is passed directly to Version.fromString. If Version.fromString does not strip qualifiers, the major/minor comparison in isBeforeCurrentMinor could produce incorrect results. Ensure the current version is parsed or normalized to strip any pre-release qualifier before comparison. buildSrc/src/main/java/org/opensearch/gradle/VersionSelection.java [24] -Version current = Version.fromString(currentVersion); +String normalizedCurrentVersion = currentVersion.replaceAll("-.*$", ""); +Version current = Version.fromString(normalizedCurrentVersion); Suggestion importance[1-10]: 6 __ Why: The test ignoresQualifiedReleaseCandidates passes "3.0.0-beta1" as currentVersion, so if Version.fromString doesn't handle qualifiers, the comparison could break. However, the test already passes with the current implementation (implying Version.fromString handles it), so this may be a precautionary improvement rather than a critical fix.	Low
General	Clarify test intent for same-minor filtering The test ignoresQualifiedReleaseCandidates includes "3.0.0" (a full release) in the candidate list, but the expected result is "2.19.5". If "3.0.0" has the same major/minor as the current version "3.0.0-beta1" (both minor=0), it should be filtered out by isBeforeCurrentMinor. However, this test also validates that "3.0.0" (same minor line) is correctly excluded — consider adding an explicit assertion or comment to clarify this intent, and add a test case where "3.0.0" would be the only candidate to confirm the error is thrown. buildSrc/src/test/java/org/opensearch/gradle/VersionSelectionTests.java [37-40] -VersionSelection.latestReleasedBeforeCurrentMinor( - "3.0.0-beta1", - List.of("2.19.4", "2.19.5", "3.0.0-alpha1", "3.0.0-beta1", "3.0.0") - ) +// Verifies that same-minor full releases are also excluded, not just pre-releases +assertEquals( + "2.19.5", + VersionSelection.latestReleasedBeforeCurrentMinor( + "3.0.0-beta1", + List.of("2.19.4", "2.19.5", "3.0.0-alpha1", "3.0.0-beta1", "3.0.0") + ) +); Suggestion importance[1-10]: 2 __ Why: The suggestion only adds a comment to clarify intent and proposes adding a new test case, but the improved_code only adds a comment without actually adding the new test case. This is a minor documentation improvement with low impact.	Low

Suggestions up to commit 85f71c5

Category	Suggestion	Impact
Possible issue	Handle pre-release qualifiers in current version parsing The currentVersion itself is not excluded from the candidates list. If currentVersion is a clean release (e.g., "3.5.0") and it appears in releasedVersions, it would pass the regex filter but then be correctly excluded by isBeforeCurrentMinor. However, if currentVersion is something like "3.5.0" and releasedVersions contains "3.5.0", the filter isBeforeCurrentMinor would correctly exclude it since it's not strictly before the current minor. This is fine, but the ignoresQualifiedReleaseCandidates test passes "3.0.0-beta1" as currentVersion, which would fail Version.fromString if it doesn't support pre-release qualifiers. Verify that Version.fromString handles pre-release version strings like "3.0.0-beta1" without throwing an exception. buildSrc/src/main/java/org/opensearch/gradle/VersionSelection.java [24-30] -Version current = Version.fromString(currentVersion); +Version current = Version.fromString(currentVersion.replaceAll("-.*$", "")); List<Version> candidates = releasedVersions.stream() .filter(version -> version.matches("\\d+\\.\\d+\\.\\d+")) .map(Version::fromString) .filter(releasedVersion -> isBeforeCurrentMinor(current, releasedVersion)) .sorted(Comparator.naturalOrder()) .collect(Collectors.toList()); Suggestion importance[1-10]: 7 __ Why: The ignoresQualifiedReleaseCandidates test passes "3.0.0-beta1" as currentVersion, which would fail Version.fromString if it doesn't support pre-release qualifiers. Stripping the qualifier before parsing is a valid defensive fix, though it depends on whether Version.fromString already handles this case.	Medium
General	Improve test coverage for same-minor exclusion The test selectsLatestReleasedVersionBeforeCurrentMinor includes "3.6.0" in the released versions list, which is a newer minor than the current version "3.5.1". The expected result "3.4.0" is correct since "3.5.0" is in the same minor line and "3.6.0" is newer. However, "3.5.0" should also be excluded since it's in the same minor line as "3.5.1". The test name says "before current minor" but "3.5.0" is in the same minor — confirm the test correctly validates that same-minor versions are excluded. buildSrc/src/test/java/org/opensearch/gradle/VersionSelectionTests.java [20] -assertEquals("3.4.0", VersionSelection.latestReleasedBeforeCurrentMinor("3.5.1", List.of("3.4.0", "3.5.0", "3.6.0"))); +assertEquals("3.4.0", VersionSelection.latestReleasedBeforeCurrentMinor("3.5.1", List.of("3.4.0", "3.5.0", "3.5.1", "3.6.0"))); Suggestion importance[1-10]: 3 __ Why: The suggestion adds "3.5.1" (the current version itself) to the input list to explicitly verify it's excluded, but the existing test already validates same-minor exclusion via "3.5.0". The improvement is marginal and the improved_code changes the test in a way that doesn't clearly add meaningful new coverage.	Low

[github-actions]

✅ Gradle check result for 85f71c5: SUCCESS

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.48%. Comparing base (6c7d6ff) to head (3fdf706).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21529      +/-   ##
============================================
+ Coverage     73.36%   73.48%   +0.11%     
- Complexity    74366    74483     +117     
============================================
  Files          5970     5970              
  Lines        338267   338267              
  Branches      48753    48753              
============================================
+ Hits         248171   248562     +391     
+ Misses        70317    69885     -432     
- Partials      19779    19820      +41     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit 05f82a5

[github-actions]

❌ Gradle check result for 05f82a5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 05f82a5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 05f82a5: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

❌ Gradle check result for 05f82a5: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit cd1512a

[github-actions]

❌ Gradle check result for cd1512a: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[reta]

Thanks @cwperks !

[github-actions]

❌ Gradle check result for 3fdf706: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

✅ Gradle check result for 3fdf706: SUCCESS