search

dcoapp / app

GitHub App that enforces the Developer Certificate of Origin (DCO) on Pull Requests
https://github.com/apps/dco
ISC License
296 stars 73 forks source link

Add merge_group trigger to enable DCO in merge queue #200

Open jcwchen opened 1 year ago

jcwchen commented 1 year ago

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: https://github.com/dcoapp/app/issues/199. This PR is trying to enable DCO in the new merge queue. There are some related discussion about DCO here.

vercel[bot] commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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?