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

Support merge_group event natively #433

Merged
merged 2 commits into from
Jul 31, 2024

Conversation

masaru-iritani
Copy link
Contributor

@masaru-iritani masaru-iritani commented Jul 25, 2024

What

Support merge_group event natively.

Why

Closes #432

Our team uses the merge queue to serialize and validate changes on the default branch. I want to run ShellCheck when a change is added to the merge queue so that I can detect problems introduced by a bad merge before pushing them to the default branch.

While I can configure the workflow to use "manual" triggering event with the base & head SHAs, it'd be convenient if the workflow natively support merge_group.

How tested

Will see the CI test results.

References

The definition of merge_group event.
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#merge_group

Copy link

codecov bot commented Jul 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.89%. Comparing base (b8c7b83) to head (f753aef).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #433      +/-   ##
==========================================
+ Coverage   85.71%   85.89%   +0.17%     
==========================================
  Files           5        5              
  Lines         315      319       +4     
==========================================
+ Hits          270      274       +4     
  Misses         45       45              

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

Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

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

Thank you for contributing. At first glance, it seems great.

Please remove the Merge commit from the history.

@masaru-iritani
Copy link
Contributor Author

Please remove the Merge commit from the history.

Sure, I did.

@jamacku
Copy link
Member

jamacku commented Jul 30, 2024

I have enabled the merge queue for the main branch. Can you also provide a workflow to run Differential ShellCheck in the merge queue? It would be very helpful. Thank you.

@masaru-iritani
Copy link
Contributor Author

Thank you for enabling the merge queue, @jamacku!

I created another pull request #434 to illustrate how to run Differential ShellCheck on a merge_group event without this change.

@jamacku
Copy link
Member

jamacku commented Jul 31, 2024

Thank you for enabling running ShellCheck on merge_group. Unfortunately, it seems that we hit an issue where if the branch doesn't contain any previous commits the base is referred to as sha 0000000000000000000000000000000000000000.

I think that the github.event.merge_group.base_sha is holding the same value.

Copy link
Member

@jamacku jamacku left a comment

Choose a reason for hiding this comment

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

The code looks great. LGTM

Thank you for your contribution. I'm going to merge this PR. Only this repository uses @main, so unless we release a new version, nobody will be able to use this feature.

I plan to do some testing, and I'll try to find a way how to fix the issue mentioned in #433 (comment)

@jamacku jamacku added the type: feature New feature label Jul 31, 2024
@jamacku jamacku added this pull request to the merge queue Jul 31, 2024
Merged via the queue into redhat-plumbers-in-action:main with commit 104fc14 Jul 31, 2024
9 checks passed
@masaru-iritani
Copy link
Contributor Author

Thank you for incorporating this change and working on some tests!

if the branch doesn't contain any previous commits the base is referred to as sha 0000000000000000000000000000000000000000.

Ah, I didn't thought about the case. Although I believe in most cases the workflow will be enabled for merge_group after using the merge queue at least once, the workflow may need to handle the case by making the check non-differential.

@jamacku
Copy link
Member

jamacku commented Jul 31, 2024

So, I have verified that merge_group works as expected:

The issue with 000... is appearing on push (triggering-event: push) to merge queue temporary branch:

For a workaround, you could provide a list of branches, etc. (e.g. on: push ; branches: [ main ]), or you can have the merge_group trigger in a separate workflow.

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

Successfully merging this pull request may close these issues.

Add support for merge_group event
2 participants