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

Add merge_group trigger to enable DCO in merge queue #200

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jcwchen
Copy link

@jcwchen jcwchen commented Feb 15, 2023

Description
Not sure whether this is the right fix -- add merge_group in the app.yml to enable DCO to run in merge queue.

Motivation
Pull Request Merge Queue Public Beta is out, but DCO app cannot run in the merge queue. See related issue: #199. This PR is trying to enable DCO in the new merge queue. There are some related discussion about DCO here.

Signed-off-by: jcwchen <jacky82226@gmail.com>
@vercel
Copy link

vercel bot commented Feb 15, 2023

@jcwchen is attempting to deploy a commit to the DCO App Team on Vercel.

A member of the Team first needs to authorize it.

@jcwchen
Copy link
Author

jcwchen commented Feb 23, 2023

We are very looking forward to using DCO app in the new merge_queue feature. @gr2m if you have bandwidth, could you please take a look at this PR? Thank you for your time!

@Fox32
Copy link

Fox32 commented Feb 23, 2023

I'm not an expert, but I believe that this change is not sufficient to make it work.

The comment in the file actually says that you have to perform the changes manually in the GitHub UI, so I guess it will not make much difference.

I would also expect that you have to listen for the event: https://github.com/dcoapp/app/blob/main/index.js#L12

Also the code currently assume that the event payload is always a pull request event, which I guess is not one once the new event is added: https://github.com/dcoapp/app/blob/main/index.js#L32

If I understand the merge queue feature correctly, each PR would already have run the DCO check as part of the PR pipeline before it is added to the queue, so we don't need to run the check again. Instead, we could just detect that we have a merge queue event and set the check state to success?

@jcwchen
Copy link
Author

jcwchen commented Feb 23, 2023

@Fox32 Thank you for the reviews and instructions. Appreciate it. Then I think the right fix is quite out of my scope, especially I don't know how to test my modification... It would be great if someone from DCO team can chime in.

If I understand the merge queue feature correctly, each PR would already have run the DCO check as part of the PR pipeline before it is added to the queue, so we don't need to run the check again. Instead, we could just detect that we have a merge queue event and set the check state to success?

I agreed with you -- ideally some pipelines/checks should be able to "only" run in the PR rather than merge queue, but currently merge queue does not support this. (See related discussion) So for now, as you mentioned, directly setting success for merge_group event would be a good approach.

@Licenser
Copy link

Licenser commented Apr 18, 2023

Currently this is blocking projects requiring DCO to use maerge groups, while enabling DCO checks for merge groups isn't the best solution perhaps it's good enough for now?

Edit: I don't think DCO is compute-intensive to run, so it probably won't be a big issue to check twice?

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

Successfully merging this pull request may close these issues.

None yet

3 participants