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 github pull request merge action #117

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

resec
Copy link

@resec resec commented Jul 6, 2021

Referring: https://docs.github.com/en/developers/webhooks-and-events/webhooks/webhook-events-and-payloads#pull_request

A merge action can be found in the close action hook.

closed: If the action is closed and the merged key is false, the pull request was closed with unmerged commits. If the action is closed and the merged key is true, the pull request was merged.

@CLAassistant
Copy link

CLAassistant commented Jul 6, 2021

CLA assistant check
All committers have signed the CLA.

@resec
Copy link
Author

resec commented Jul 6, 2021

The pr test data json seems incorrect, when a pr is not merged merged==false, the merged_at field should be null.

Which causing the all the 3 cases are failing.

@bradrydzewski
Copy link
Member

The pr test data json seems incorrect, when a pr is not merged merged==false, the merged_at field should be null.

I cannot speak to the implementation, but I can confirm the webhook payloads in the testdata folder are real payloads that were captured directly from github.

@@ -371,7 +371,7 @@
"site_admin": false
},
"merge_commit_sha": "e5bd3914e2e596debea16f433f57875b5b90bcd6",
"merged": false,
"merged": true,
Copy link
Member

@bradrydzewski bradrydzewski Jul 6, 2021

Choose a reason for hiding this comment

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

the json payload in pr.json is for an open pull request; therefore it should not be set to merged: true. I would instead suggest adding a new, real world json file and creating a new test case. I think you will find our existing merged_at logic works as expected if you provide it with a real world payload for a merged action.

Copy link
Author

Choose a reason for hiding this comment

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

I tested on my own repo, the merged and merge_at are always marked or filled in the case of merged-pr. And both not marked or filled in the case of closed-pr and opening-pr.

Copy link
Member

@bradrydzewski bradrydzewski Jul 6, 2021

Choose a reason for hiding this comment

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

I think there may be a misunderstanding. The pr.json file is for an open pull request (the state field is set to open) which means it cannot have merged set to true. If you want to create a test case for a merged pull request, you should add a new file, for example pr_merged.json; this should be a real json payload captured from github, not something modified by hand.

Copy link
Author

Choose a reason for hiding this comment

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

Hmmm, it the pr.json is for a opening-pr, then its merge_at must be null. If it is not merged (opening), how can it have a merge time... So I think something in the pr.json must be wrong.

But having another test json (maybe pr_merged.json) make sense.

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.

3 participants