Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Trigger manifest check only for mentioned paths #4355

Merged
merged 4 commits into from
Jan 18, 2024

Conversation

gaiksaya
Copy link
Member

@gaiksaya gaiksaya commented Jan 18, 2024

Description

Trigger manifest check only for mentioned paths. Currently they are triggered for all PRs. https://github.com/opensearch-project/opensearch-build/actions/workflows/manifests.yml and aborted later
Also fixes the badge in readme to green once the check are only related to actual run and not aborted ones

Issues Resolved

List any issues this PR will resolve, e.g. Closes [...].

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.

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

codecov bot commented Jan 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (81dcc81) 91.35% compared to head (53f2d55) 91.35%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4355   +/-   ##
=======================================
  Coverage   91.35%   91.35%           
=======================================
  Files         190      190           
  Lines        6175     6175           
=======================================
  Hits         5641     5641           
  Misses        534      534           

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

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>
@Divyaasm
Copy link
Collaborator

We should now be able to trigger the manifests gh action only if PR contains manifest files from the provided paths. LGTM @gaiksaya

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

@peterzhuamazon peterzhuamazon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see these already present here:
dd41a58#diff-947e5f024300451482689c8e4260d039612c1926a9f5cdeadf4d508be652d9deL6

Also, I dont think we need to include legacy ones as that is the point of being legacy.

Thanks.

@gaiksaya
Copy link
Member Author

I see these already present here: dd41a58#diff-947e5f024300451482689c8e4260d039612c1926a9f5cdeadf4d508be652d9deL6

Also, I dont think we need to include legacy ones as that is the point of being legacy.

Thanks.

The lines are missing from main workflow here: https://github.com/opensearch-project/opensearch-build/blob/main/.github/workflows/manifests.yml

I added legacy ones, just in case someone tries to make changes. We need the ci check to pass if we make any change to legacy manifest too. Won't hurt to keep the check.

@peterzhuamazon
Copy link
Member

Seems like @zelinh PR mistakenly remove it here:
#4264

Thanks.

@zelinh
Copy link
Member

zelinh commented Jan 18, 2024

Seems like @zelinh PR mistakenly remove it here: #4264

Thanks.

I think we did that on purpose. Since we run

        id: list-changed-manifests
        with:
          files: manifests/**/opensearch*.yml

the trigger there at front won't serve any purpose.

@peterzhuamazon
Copy link
Member

Seems like @zelinh PR mistakenly remove it here: #4264
Thanks.

I think we did that on purpose. Since we run

        id: list-changed-manifests
        with:
          files: manifests/**/opensearch*.yml

the trigger there at front won't not serve any purpose.

Yeah, now I remember this now. Thanks.

@gaiksaya
Copy link
Member Author

Seems like @zelinh PR mistakenly remove it here: #4264
Thanks.

I think we did that on purpose. Since we run

        id: list-changed-manifests
        with:
          files: manifests/**/opensearch*.yml

the trigger there at front won't not serve any purpose.

The workflows are still getting triggered. https://github.com/opensearch-project/opensearch-build/actions/workflows/manifests.yml

This will save resources. Only do checks if specific path is changed

@gaiksaya gaiksaya merged commit 34f27e9 into opensearch-project:main Jan 18, 2024
11 checks passed
@gaiksaya gaiksaya deleted the fix-manifest-path branch January 18, 2024 23:49
Copy link
Member

@zelinh zelinh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure makes sense for me. We could add this back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

4 participants