Improve Gradle pre-commit checks to pre-empt Jenkins build

[Bukhtawar]

Is your feature request related to a problem? Please describe.

1. The gradle pre-commit is a check used exhaustively before making commits to the repository. If the commit contains files whose java docs are missing the precommit check passes however the gradle check Jenkins build which runs the full integ test suite detects it towards the end, which seems wasteful See : https://build.ci.opensearch.org/job/gradle-check/1615/console

2. Trigger pre-commit checks before triggering a full gradle check since if the pre-commit checks fail so will the gradle check Jenkins build, so we end up doing redundant runs. See : https://build.ci.opensearch.org/job/gradle-check/1613/console

Describe the solution you'd like A clear and concise description of what you want to happen.

Describe alternatives you've considered A clear and concise description of any alternative solutions or features you've considered.

Additional context Add any other context or screenshots about the feature request here.

[dbwiddis]

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

[sreenath-tm]

I'm interested in contributing to this issue, so before I start working it, would you mind sparing your time explaining what the issue is about and pointing me to some resources to get started if this is an issue where a fix is required.

[dbwiddis]

Hey @sreenath-tm great to hear you are interested. We'd love your contribution here.

First, take a look at any PR. If you scroll to the bottom you will see the latest "checks" that are done with every PR. It will look like this:

Ideally everything is green. In the above tests, the precommit checks failed. Therefore we knew the Jenkins test would also fail. Since the Jenkins tests consume a lot more resources, we would like to wait until the precommit checks pass before even attempting the Jenkins test.

If you drill down on the test details for PR checks, you will find the associated config files for these two checks are .github/workflows/gradle-check.yml and .github/workflows/precommit.yml.

You will want to edit the gradle-check.yml file to add a workflow_run section per the link I posted in this comment to run after the precommit check. You will also need to add a conditional (on github.event.workflow_run.conclusion) in the jobs section to only run on successful completion, also described in the same link.

Let me know if you have any questions!

[dbwiddis]

@sreenath-tm actually there are two problems called out in the description. My answer above addressed the second point.

For the first point, we'd like to test for missing javadocs much earlier than we currently do. Some candidates would be:

• create a new github action that runs those javadoc tests and make it also a prerequisite for the Jenkins gradle check build
• add the javadoc checks to the precommit tests in the gradle build file
• reorder the tests in the gradle build file

@Bukhtawar do you have a suggestion which approach would be best?

[yeohbraddy]

Hi all,

I'd like to take this if @sreenath-tm is not. It is my first time contributing to open-source so apologies if this is against etiquette

Thank you!

[sreenath-tm]

Hi @yeohbraddy ,I was waiting for @Bukhtawar confirmation on an approach to get started ,but fell free to work on this issue and please do let me know if there is any issues while working on it.

[dbwiddis]

Hi @yeohbraddy looks like you're all clear to work on this. Welcome to Open Source and feel free to ask any questions.

Even if you only implement one of the two improvements it would be very helpful.

[dbwiddis]

Also, it looks like the ordering of the Javadoc check at the end is rather intentional. It's explicitly placed in project.afterEvaluate. The reasoning is that some packages may refer to each other:

  project.afterEvaluate {
    // Handle javadoc dependencies across projects. Order matters: the linksOffline for
    // org.opensearch:opensearch must be the last one or all the links for the
    // other packages (e.g org.opensearch.client) will point to server rather than
    // their own artifacts.

In theory we could add an additional more basic check (e.g., missing javadocs) up front while still fully evaluating the javadoc content at the end. However this increases complexity (and makes the build even longer).

For now, unless @Bukhtawar has other suggestions, I'd suggest @yeohbraddy just deal with the GHA dependency.

[dbwiddis]

Hey @yeohbraddy it looks like just running ./gradlew javadoc is pretty fast and does the missing doc check, while we can leave the full validation to the end of the full build.

My suggestion to handle point 1 is just to add the javadoc check to the precommit GitHub Action. Just change ./gradlew precommit --parallel to ./gradlew javadoc precommit --parallel.

[yeohbraddy]

Hi @dbwiddis,

I implemented the suggested solution for 1) and 2), and I am wondering what is the best approach to test these changes ?

[dbwiddis]

Hey @yeohbraddy I'm not sure there is any automated testing as you're actually testing the automation!

• you should be able to validate the javadoc check yourself just by checking on the command line that it fails for missing javadocs
• I believe the github actions automation tries (and always fails) on your forked GitHub anyway, so when you push it you should see it not running the gradle check.

Once you've confirmed both of those, submit a PR and we'll see if it makes a difference on the main project... would be great to have an intentionally failing precommit check as part of the initial PR.

[dblock]

If it's hard to test on a fork but we have reasonable expectation that it will work once merged we can merge that change and see what happens, it's fine.

[yeohbraddy]

https://github.com/opensearch-project/OpenSearch/pull/4660/

[saratvemulapalli]

Thanks @yeohbraddy for the contribution. @Bukhtawar I'll close this issue, feel free to re-open.