Check changed files before running gradle check #13498

gaiksaya · 2024-05-01T23:53:27Z

Description

The repository requires the gradle check job status to pass before merging any PR. With path-ignore filter, this status check was skipped. However, GitHub does not resolve this conflict or allows to specify the requirement to be optional. See #13494 still waiting for gradle-check status to be set.
There are multiple discussions on GH community forums https://github.com/orgs/community/discussions/44490 and all of them requires work around as of now. This change adds one more step to gradle-check job to detect the files changed and accordingly decide whether to run the consecutive steps or not. See examples below:

Where gradle check runs for change in build.gradle test gaiksaya/OpenSearch#10
Gradle check steps are skipped for changes in md file test gaiksaya/OpenSearch#11

I set my fork requirements to mimic gradle check status as required and this satisfies both conditions

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

peternied

I insist we are very careful with allowing the bypass of the gradle check.

If there are changes that are blocked because of the missing gradle check results, lets revert the initial update so we have more time to iterate on this change

[Edit] I've created a PR to revert Revert "Skip running gradle checks on release notes" #13499

peternied · 2024-05-02T01:42:19Z

.github/workflows/gradle-check.yml

+        with:
+          files_ignore: |
+            release-notes/**
+            .github/**


We cannot skip files under .github, if you wanted to modify this to specific files - code owners, this is OK with me.

Are files under .github related to gradle check in anyway?

They never are, these are github workflows. And the workflow itself runs on pull request target, so it doesn't impact what's used on the PR itself with the changes to it.

peternied · 2024-05-02T01:43:13Z

.github/workflows/gradle-check.yml

+        uses: tj-actions/changed-files@v44
+        with:
+          files_ignore: |
+            release-notes/**


I don't trust the ** annotation, lets be very selective

Suggested change

release-notes/**

release-notes/*.md

.github/workflows/gradle-check.yml

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

peternied · 2024-05-02T17:21:16Z

.github/workflows/gradle-check.yml

+        uses: tj-actions/changed-files@v44
+        with:
+          files_ignore: |
+            release-notes/*.md


Could you please add a comment after each file/entry that justifies why it should be ignored?

If the answer is we want to go fast, why not just have an admin merge bypassing checks?

I believe saving time and resources is a generic reason even enough for this change? Running a job for 1+ hour and the backport (1+ hour) which adds no value to the run but can be a blocker due to flaky tests. Bypassing is always an option but usually not an opted one.

Also I am not sure if adding a comment would work or not (needs more testing) as I see one more input files_ignore_separator in the action used https://github.com/tj-actions/changed-files?tab=readme-ov-file#inputs-%EF%B8%8F

github-actions · 2024-05-02T17:52:17Z

❌ Gradle check result for 6179f01: 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 · 2024-05-02T18:08:41Z

❌ Gradle check result for 574d718: 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 · 2024-05-02T21:58:45Z

✅ Gradle check result for 6179f01: SUCCESS

codecov · 2024-05-02T22:00:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.49%. Comparing base (b15cb0c) to head (6179f01).
Report is 256 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #13498      +/-   ##
============================================
+ Coverage     71.42%   71.49%   +0.07%     
- Complexity    59978    61012    +1034     
============================================
  Files          4985     5050      +65     
  Lines        282275   286806    +4531     
  Branches      40946    41552     +606     
============================================
+ Hits         201603   205046    +3443     
- Misses        63999    64848     +849     
- Partials      16673    16912     +239

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

I won't block merging but I've got limited availability for follow up. Please iterate to green on the description check and document the CI failures

gaiksaya · 2024-05-03T19:51:14Z

@kotwanikunal @reta Can you please review this change? Let me know if I need to add more maintainers.
Thanks!

dblock · 2024-05-03T19:56:04Z

.github/workflows/gradle-check.yml

+            RELEASING.md
+            SECURITY.md
+            TESTING.md
+            TRIAGING.md


What's the reason not to include all *.md at root?

See the conversation here #13498 (comment)

dblock · 2024-05-03T19:57:23Z

.github/workflows/gradle-check.yml

      - name: Setup environment variables (PR)
-        if: github.event_name == 'pull_request_target'
+        if: github.event_name == 'pull_request_target' && steps.changed-files-specific.outputs.any_changed == 'true'


Can we split out the gradle check decision, setup and workflow into two separate jobs, and make the second job dependent?

See #13498 (comment)

👍 to @dblock 's suggestion, the same condition is propagated to (mostly) every step

So I did try that but does not work as expected. See comment posted above. Let me know if you can think of better way.

I don't think I understand why the gradle check job is different from other jobs. It should be skippable, shouldn't it?

The issue is this repository has added gradle-check as a required workflow check before merging any PR. GitHub is unable to resolve the conflict if the workflow is skipped due to any condition. It HAS to be passed. Not failed, not skipped :(

@gaiksaya I think there might be another approach as described here: https://stackoverflow.com/questions/65844033/conditional-needs-in-github-actions

We'd need to something like:

jobs: changed-files-specific: steps: - set the any_changed boolean gradle-check: runs-on: ubuntu-latest needs: changed-files-specific if: ${{ needs.changed-files-specific.outputs.any_changed == 'true' }} steps: - run the gradle check steps check-result: needs: gradle-check if: always() steps: - name: fail if conditional job failed if: ${{ needs.changed-files-specific.outputs.any_changed == 'true' && needs.gradle-check.result == 'failure' }} run: exit 1

And then the branch protection rule would be on check-result which will always run but is smart enough to ignore gradle-check if no relevant files changed. Would this work?

Sorry for delayed response. Missed this. Not really, it has to pass if it is required. Cannot skip or fail. So I believe only approach I think is follow what you just posted and add one more step which will print something like "Skipping check" and show it as pass. Let me know if everyone are okay with this approach

@gaiksaya In my suggestion I think the check-result step will always run, and it is the thing that will be marked as required. Why would you need another step?

reta · 2024-05-03T19:57:32Z

.github/workflows/gradle-check.yml

        run: |
          set -e
          set -o pipefail
          bash opensearch-build/scripts/gradle/gradle-check.sh ${{ secrets.JENKINS_GRADLE_CHECK_GENERIC_WEBHOOK_TOKEN }} | tee -a gradle-check.log

      - name: Setup Result Status
-        if: always()
+        if: always() && steps.changed-files-specific.outputs.any_changed == 'true'


I think always() does not apply anymore:

Suggested change

if: always() && steps.changed-files-specific.outputs.any_changed == 'true'

if: steps.changed-files-specific.outputs.any_changed == 'true'

If always() is removed, the step would be skipped if previous check failed as default is success()

But this is and condition, right? Since always() is always true, only steps.changed-files-specific.outputs.any_changed == 'true' counts

failure() && steps.changed-files-specific.outputs.any_changed == 'true' will be evaluated to false if previous step fails right? In that case we need always() for first condition to be set to true

You might be right, thanks @gaiksaya , it seems like semantics of always() is more than just true

dblock

Looks like @gaiksaya did a ton of iterating on this one and all our suggestions have already been tried :) I am ok with this change as is. One of the concerns I do have is that a gradle check will visibly pass even if it did nothing.

Will leave for more comments for now.

reta · 2024-05-06T13:04:22Z

@gaiksaya @dblock have some thoughts regarding the problem over the weekend, I think the idea is sound but the way we approach it probably not. Primarily, because for some reason, we only focus on Gradle check but the same "skip me" rules apply to other workflows like assemble (and very likely even precommit).

What if we combine changeset detection + Gradle build instead, and just skip most of the tasks if there are no relevant changes?

dblock · 2024-05-06T16:26:26Z

What if we combine changeset detection + Gradle build instead, and just skip most of the tasks if there are no relevant changes?

Maybe.

I am confused why we needed to do anything special about gradle check in terms of requiring it to pass. Isn't that what any check does by default?

reta · 2024-05-06T17:00:32Z

I am confused why we needed to do anything special about gradle check in terms of requiring it to pass. Isn't that what any check does by default?

I mean - we don't need to run the whole test suite if only CHANGELOG.md changes, right?

gaiksaya · 2024-05-06T17:08:08Z

What if we combine changeset detection + Gradle build instead, and just skip most of the tasks if there are no relevant changes?

Maybe.

I am confused why we needed to do anything special about gradle check in terms of requiring it to pass. Isn't that what any check does by default?

Other workflows are not set to mandatory at branch settings. Current settings for this repo that applies to all branches:

gaiksaya · 2024-05-06T17:22:36Z

@gaiksaya @dblock have some thoughts regarding the problem over the weekend, I think the idea is sound but the way we approach it probably not. Primarily, because for some reason, we only focus on Gradle check but the same "skip me" rules apply to other workflows like assemble (and very likely even precommit).

What if we combine changeset detection + Gradle build instead, and just skip most of the tasks if there are no relevant changes?

I believe main reason would be other workflows are not as flaky and not so time consuming. Gradle check is a whole different story. What you are suggesting does not need a common workflow. Just adding path-ignore in those workflows would work as GH would just skip them and won't wait at PR merge asking for status.
Drawback: Each workflow would have it own list to ignore which I think is fine?

Also, I believe you mean gradle tasks here? just skip most of the tasks That would need to break down the gradle check right?

reta · 2024-05-06T17:32:47Z

Thanks @gaiksaya

Drawback: Each workflow would have it own list to ignore which I think is fine?

I think the ignore list is per repository, right? The only thing we may need to figure out - what has changed (changeset)

Also, I believe you mean gradle tasks here? just skip most of the tasks That would need to break down the gradle check right?

Correct, more like onlyIf { hasChanges() } condition.

gaiksaya · 2024-05-06T18:13:46Z

I think the ignore list is per repository, right? The only thing we may need to figure out - what has changed (changeset)

It's per workflow. Example: https://github.com/opensearch-project/OpenSearch/pull/13477/files# and path.ignore

Also, I believe you mean gradle tasks here? just skip most of the tasks That would need to break down the gradle check right?

Correct, more like onlyIf { hasChanges() } condition.

Can this be a part of gradle check breakdown issue?

reta · 2024-05-06T18:56:17Z

It's per workflow. Example: https://github.com/opensearch-project/OpenSearch/pull/13477/files# and path.ignore

This is how we implemented it, correct, but it does not need to be

Can this be a part of gradle check breakdown issue?

May be, I just wanted to run the idea over you folks, I haven't prototyped it

dblock · 2024-05-07T06:53:59Z

I think we should decide whether we want to merge this as is. WDYT @gaiksaya @reta? I am good with whatever you two choose!

gaiksaya · 2024-05-07T17:03:57Z

I think we should decide whether we want to merge this as is. WDYT @gaiksaya @reta? I am good with whatever you two choose!

I am good to merge as is too.

reta · 2024-05-07T17:17:51Z

I am good to merge as is too.

We have no better working option at the moment (and @gaiksaya did really good job here figuring out GA internals) but do we need to rush the merge? The workflow becomes difficult to follow and change, this is the concern I have (and that is the motivation to look for other options).

gaiksaya requested a review from peternied as a code owner May 1, 2024 23:53

gaiksaya mentioned this pull request May 1, 2024

Skip running gradle checks on release notes #13477

Merged

8 tasks

gaiksaya requested review from reta and kotwanikunal May 1, 2024 23:54

gaiksaya marked this pull request as draft May 2, 2024 00:22

gaiksaya marked this pull request as ready for review May 2, 2024 00:48

gaiksaya added the skip-changelog label May 2, 2024

peternied previously requested changes May 2, 2024

View reviewed changes

peternied mentioned this pull request May 2, 2024

Revert "Skip running gradle checks on release notes" #13499

Merged

8 tasks

gaiksaya added 3 commits May 2, 2024 10:12

Check changed files before running gradle check

e02f241

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

Convert to steps

940c1c5

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

Refactor files list

574d718

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

gaiksaya force-pushed the fix-gradle-check branch from 95cca5c to 574d718 Compare May 2, 2024 17:15

refactor

6179f01

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>

peternied reviewed May 2, 2024

View reviewed changes

gaiksaya mentioned this pull request May 2, 2024

Fix change log entry #13494

Open

1 task

gaiksaya requested a review from peternied May 3, 2024 17:07

dblock reviewed May 3, 2024

View reviewed changes

reta reviewed May 3, 2024

View reviewed changes

dblock reviewed May 4, 2024

View reviewed changes

	if: always() && steps.changed-files-specific.outputs.any_changed == 'true'
	if: steps.changed-files-specific.outputs.any_changed == 'true'

Check changed files before running gradle check #13498

Are you sure you want to change the base?

Check changed files before running gradle check #13498

Conversation

gaiksaya commented May 1, 2024 • edited

Description

Related Issues

Check List

peternied left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 2, 2024

github-actions bot commented May 2, 2024

github-actions bot commented May 2, 2024

codecov bot commented May 2, 2024

Codecov Report

gaiksaya commented May 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock May 3, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dblock left a comment

Choose a reason for hiding this comment

reta commented May 6, 2024

dblock commented May 6, 2024

reta commented May 6, 2024

gaiksaya commented May 6, 2024

gaiksaya commented May 6, 2024 • edited

reta commented May 6, 2024

gaiksaya commented May 6, 2024

reta commented May 6, 2024

dblock commented May 7, 2024

gaiksaya commented May 7, 2024

reta commented May 7, 2024

gaiksaya commented May 1, 2024 •

edited

peternied left a comment •

edited

dblock May 3, 2024 •

edited

gaiksaya commented May 6, 2024 •

edited