[Proposal] Set `bwc.checkout.align=true` in gradle check CI workflow

[andrross]

Is your feature request related to a problem? Please describe

See opensearch-project/OpenSearch#2350 for more detailed discussion, but the tl;dr is that changes to the 2.x branch can break the CI workflows while iterating on a PR. The "fix" in these cases is to rebase your PR from main, but the failures are generally unrelated to your PR. The pain here is particularly acute when all you're doing on your PR is retrying flaky tests and then they start failing due to a version increment (for example) and then it requires you to do more work to diagnose and rebase.

Describe the solution you'd like

Enable bwc.checkout.align=true by default in the CI workflows. This is existing functionality in our build tooling that does a best-effort lookup that matches a commit on the BWC branch to your commit based on time. What this means is that 2.x is no longer a moving target and is instead fixed to a particular commit.

An example:

git checkout c0fca74709e96ec7c8cf295cdca6110946b592b8
./gradlew ':qa:verify-version-constants:v2.12.0#integTest' --tests "org.opensearch.qa.verify_version_constants.VerifyVersionConstantsIT" -Dbwc.checkout.align=true

Without -Dbwc.checkout.align=true this test would fail as the 2.x branch has been updated to 2.13. However, aligning it to a commit on 2.x prior to the version change allows the test to pass.

The risk of this change is that if something is committed and backported that is truly incompatible with your change, then you might not detect it until after your change is committed. This risk exists today, but with this change might make such a situation more likely. However, on balance I think the improved developer experience is worth the risk. I'm interested in others' opinions on this.

Related component

Build

Describe alternatives you've considered

Do nothing. Require all devs to rebase in-flight PRs when things like version changes happen.

Additional context

No response

[reta]

However, on balance I think the improved developer experience is worth the risk. I'm interested in others' opinions on this.

Thanks @andrross , :+1: to thank, it looks like a reasonable balance to me

[peternied]

I think the improved developer experience is worth the risk. I'm interested in others' opinions on this.

👍

[peternied]

[Triage - attendees 1 2 3 4 5] @andrross Thanks for filing, this looks like a solid improvement

[andrross]

The definition of the command that will need the new parameter is here: https://github.com/opensearch-project/opensearch-build-libraries/blob/8fe100542133d5f5e85cd9868d3df3396075fe3e/vars/runGradleCheck.groovy#L74

However, I think we should parameterize it so that only when the gradle check is the result of a PR (this case) do we pass the align parameter. This ensures the gradle checks that run as a result of a commit are always running against the latest committed in the bwc branch.

@bbarani Can we transfer this to the opensearch-build repo as I believe the changes have to be made there?

[bbarani]

@prudhvigodithi @gaiksaya Can you please take a look?

[peterzhuamazon]

[Triage] Need to sync up with @andrross on the details of this change. Whether or not adding -Dbwc.checkout.align=true directly to the gradle command is good enough.

Moving this to the lib repo as the changes should be made there. Thanks.

[andrross]

Whether or not adding -Dbwc.checkout.align=true directly to the gradle command is good enough.

@peterzhuamazon Yes, that parameter is all that is needed here.

[andrross]

Every time we do a release or update Lucene versions (both of which we have done very recently) we see a bunch of PRs start failing their bwc tests because the version numbers on the 2.x branch have changed. They must rebase their PR in order to get the tests to pass. I think that problem will pretty much go away if we implement this. So just a friendly suggestion to prioritize this if at all possible :)

[rishabh6788]

@andrross Can you please confirm if just adding -Dbwc.checkout.align=true here is sufficient or do we need to make sure it is only added in case it is triggered as part of pull request action and not post_merge_action. Making it configurable is not a problem, just want to make sure on which action should this be included.

[andrross]

@rishabh6788 I think we should probably make it configurable so that post_merge_action always runs with the latest code on the bwc branch.

[rishabh6788]

@andrross Please take a look at two PRs and let me know if they are as per your expectations.

[rishabh6788]

@andrross The changes have been merged and should get picked up by gradle-check runs initiated by pull requests. Will keep this open for a couple of days to confirm if everything is working as expected and then close it.