milestoned and demilestoned events handling

[urkle]

So, github sends the "milestoned" and "demilestoned" events (setting or unsetting the milestone) for pull requests as issue events.. thus the rules for peril don't quite work as all will show up as issue events even if they are pull requests.

I believe we can perform a "workaround" though.
in the payload under "issue" there is a pull_request node that is set if the issue is an actual PR. So we could use that to detect that it is, in-fact, a PR and fake the event to be pull_request instead.

I'm thinking (from skimming the code) that this might be doable in github_runner.ts#runsForEvent and "correct" the event as pull_request

abbreviated webhook payload for milestoned

{
  "action": "milestoned",
  "issue": {
    "number": 123,
    "title": "my PR title",
    // other attributes (assignees, labels, user, etc..updated_at, created_at, etc..)
    "milestone": {
      // new milestone of PR
    },
    // how we can detect it is a pull request
    "pull_request": {
      // various pull request URLs
    },
    "body": "my description"
  },
  "repository": {
     // repository details
  },
  "organization": {
    "login": "myorg",
    // other org data
  },
  "sender": {
    "login": "sender",
    // other sender data
  },
  "installation": {
    "id": 12345
  }
}

[jtreanor]

This is something that would also be very useful for our use case. We require all pull requests to have a milestone, but if a PR is opened without one, there is no obvious way to re-run Peril.

@orta I see that you have concerns with how #343 departs from the Github documentation. I understand this point of view and I wonder if there is another way we could achieve this.

Do you think it could make sense to extend the peril DSL to provide a way to trigger a PR run, similar to how peril.runTask works now?

Perhaps it could look something like this:

import { peril } from "danger"

export default async (webhook: Issues) => {
  if (_.get(webhook, 'issue.pull_request')) {
    await peril.runPullRequest(webhook);
  }
}

[orta]

Yeah, sorry @urkle - I've not come back to think about it too hard, which is why I left it open.

I wonder if it's worth extending the check like in #343 but to allow only milestone/demilestoning to be inside pull_request, so as a user could write both:

{
  "pull_request.milestoned": "dangerfile.ts",
  "issue.milestoned": "dangerfile2.ts"
}

One of which would only trigger during a PR, the other for both Issue and PRs. We can treat it as a deliberate extension of the pull_request event scope. This can be doc'd as a special case extension.

#343 does this but at the tradeoff of not being able to run any issue based webhooks for issues with associated PRs, ideally we shouldn't trade for that

[urkle]

@orta I thought that Github doesn't send any "issue" events for pull_reaquests? Thus the scenario you are describing shouldn't even happen. For example the labeled event on a PR only gets sent as pull_request.labeled not issue.labeled. Thus it would seem odd that we are having milestoned and demilestoned behave differently than every other event?

All #343 is doing is checking to see if the "issue.milestoned" event should have been a pull_request.milestoned (due to GitHub's bug in their webhook handling) so that perl can handle things sanely.

Perhaps this whole code would be better served in some pre-filter to the events.. As we still have #342 to figure out how to solve which require inspecting that event payload and then generating fake runs for each of the IDs.