Tolerate BWC branch patch version bump

[andrross]

When a patch release (e.g., 2.19.5) is published and the release branch is bumped to the next patch (2.19.6), BWC tests on main fail because Version.java still references the old patch version. This causes all in-flight PRs to fail until Version.java is updated.

This change relaxes the logic so that BWC tests will still pass if the checked out code uses a patch version one greater than expected. This prevents CI failures every time a release branch increments its patch version, but still prevents the main branch from drifting by more than one patch version.

I tested this manually with a local 3.5 branch. Works as expected when I bump 3.5.0 to 3.5.1, but fails with 3.5.2.

Check List

• Functionality includes testing.

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.

[andrross]

@reta Curious what you think about this. Is there any downside? It would make it much smoother to push changes like #20867 without causing all inflight PRs to break and need a rebase.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Expanded Directory Logic Change The old code only created expandedDistDir for versions on or after "7.10.0" (if (version.onOrAfter("7.10.0") && (name.endsWith("zip") || name.endsWith("tar")))). The new code removes this version guard and always sets expandedDistDir for zip/tar projects. This could cause issues for BWC versions prior to 7.10.0 where the expanded install directory may not exist. if (name.endsWith("zip") || name.endsWith("tar")) { this.expandedDistDir = new File(checkoutDir, baseDir + "/" + name + "/build/install"); } else { this.expandedDistDir = null; } Artifact File Name Mismatch The artifactFileName used to compute suffix and archIndex is derived from getExpectedDistFile().getName(), but the actual artifact registered via artifactFileProvider may resolve to the fallback file with a different name/version string. The suffix and classifier computed from the expected file name may not match the fallback file, potentially causing artifact resolution issues downstream. String artifactFileName = distributionProject.getExpectedDistFile().getName(); String artifactName = "opensearch"; String suffix = artifactFileName.endsWith("tar.gz") ? "tar.gz" : artifactFileName.substring(artifactFileName.length() - 3); int archIndex = artifactFileName.indexOf("x64"); Provider<File> artifactFileProvider = providerFactory.provider(() -> { if (distributionProject.getExpectedDistFile().exists()) { return distributionProject.getExpectedDistFile(); } else if (distributionProject.getFallbackDistFile().exists()) { return distributionProject.getFallbackDistFile(); } // File doesn't exist, validation will fail elsewhere but we must return a File here return distributionProject.getExpectedDistFile(); }); bwcProject.getConfigurations().create(distributionProject.name); bwcProject.getArtifacts().add(distributionProject.name, artifactFileProvider, artifact -> { artifact.setName(artifactName); artifact.builtBy(buildBwcTask); artifact.setType(suffix); String classifier = ""; if (archIndex != -1) { int osIndex = artifactFileName.lastIndexOf('-', archIndex - 2); classifier = "-" + artifactFileName.substring(osIndex + 1, archIndex - 1) + "-x64"; } artifact.setClassifier(classifier); }); Cache Invalidation Risk Both getExpectedDistFile() and getFallbackDistFile() are registered as outputs of the BWC build task. When the fallback file is produced instead of the expected file, subsequent incremental builds may not correctly detect the change, since Gradle's up-to-date checks will see the expected file as missing but the fallback as present. This could lead to unexpected caching or skipped-build behavior. c.getOutputs().files(distributionProject.getExpectedDistFile(), distributionProject.getFallbackDistFile());

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Restore version guard for expanded dist directory The original code had a version guard (version.onOrAfter("7.10.0")) before setting expandedDistDir, but the refactored constructor removed this check. This changes behavior for versions prior to 7.10.0, where expandedDistDir should remain null even for zip/tar projects. The expectedVersion parameter should be used to preserve the original logic. buildSrc/src/main/java/org/opensearch/gradle/internal/InternalDistributionBwcSetupPlugin.java [307-311] -if (name.endsWith("zip") || name.endsWith("tar")) { +if (expectedVersion.onOrAfter("7.10.0") && (name.endsWith("zip") || name.endsWith("tar"))) { this.expandedDistDir = new File(checkoutDir, baseDir + "/" + name + "/build/install"); } else { this.expandedDistDir = null; } Suggestion importance[1-10]: 7 __ Why: The original code had a version.onOrAfter("7.10.0") guard before setting expandedDistDir, but the refactored constructor removed this check. This changes behavior for versions prior to 7.10.0, where expandedDistDir should remain null even for zip/tar projects. The expectedVersion parameter is available in the constructor and should be used to preserve the original logic.	Medium
General	Avoid declaring both files as required task outputs Declaring both expectedDistFile and fallbackDistFile as task outputs means Gradle's up-to-date checking and build cache will require both files to exist after the task runs. If only one of the two files is produced, Gradle may consider the task out-of-date or fail cache storage. Consider using a single lazy output that resolves to whichever file actually exists after execution. buildSrc/src/main/java/org/opensearch/gradle/internal/InternalDistributionBwcSetupPlugin.java [242] -c.getOutputs().files(distributionProject.getExpectedDistFile(), distributionProject.getFallbackDistFile()); +c.getOutputs().files(providerFactory.provider(() -> { + File expected = distributionProject.getExpectedDistFile(); + File fallback = distributionProject.getFallbackDistFile(); + if (expected.exists()) return expected; + if (fallback.exists()) return fallback; + return expected; +})); Suggestion importance[1-10]: 6 __ Why: Declaring both expectedDistFile and fallbackDistFile as task outputs could cause Gradle's up-to-date checking to require both files to exist after the task runs. Using a lazy provider that resolves to whichever file actually exists would be more correct, though the actual impact depends on Gradle's behavior with missing output files.	Low

[github-actions]

✅ Gradle check result for afd6e96: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 0% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.38%. Comparing base (4d6ccf1) to head (afd6e96).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...e/internal/InternalDistributionBwcSetupPlugin.java	0.00%	36 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #20871      +/-   ##
============================================
+ Coverage     73.30%   73.38%   +0.08%     
- Complexity    72280    72337      +57     
============================================
  Files          5796     5797       +1     
  Lines        330263   330323      +60     
  Branches      47663    47676      +13     
============================================
+ Hits         242102   242420     +318     
+ Misses        68754    68506     -248     
+ Partials      19407    19397      -10     

☔ 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.

[reta]

@reta Curious what you think about this. Is there any downside? It would make it much smoother to push changes like #20867 without causing all inflight PRs to break and need a rebase.

Thanks @andrross , I think this is pretty clever idea, the only issue I see here is that we may let some changes that require version specific logic to slip in, a hypothetical (but real) example:

• change uses Version.onOrAfter(2.19.4)
• meanwhile, we bump to 2.19.5 (now the pull request would need rebase/merge, bwc fails)
• with relaxed BWC, the change will go to 2.19.5 under assumption it is still 2.19.4

I certainly see the benefits, the alternative I see (which could be a bit easier / straightforward to implement), is to have some sort of automerge (like dependabot does): on version bump, merge the active pull requests with target branch.

In any case, certainly +1 to try it out, would love to have an option to cap it by time: relax BWC version fallback to one week, hard fail after (although that would require more work for doubtful benefits).

[andrross]

FYI @reta, didn't have much success with this approach. It turns out FullClusterRestart has some test assertions that fail on a patch mismatch: https://build.ci.opensearch.org/job/gradle-check/72679/testReport/