search

octokit / webhooks.js

GitHub webhook events toolset for Node.js
MIT License
313 stars 79 forks source link

Typescript: "organization" key missing in Webhook payload definitions #87

Closed acazacu closed 4 years ago

acazacu commented 5 years ago

Using typescript, the webhook event payload type definitions do not match the actual payloads sent by Github.

Example

The handler for the check_suite.completed event receives an argument of type Webhook.WebhookEvent<Webhook.WebhookPayloadCheckSuite>

The Webhook.WebhookPayloadCheckSuite type is defined as:

type WebhookPayloadCheckSuite = {
    action: string;
    check_suite: WebhookPayloadCheckSuiteCheckSuite;
    repository: PayloadRepository;
    sender: WebhookPayloadCheckSuiteSender;
};

The actual received payload contains two extra fields which are missing from the type definition, these fields are installation and organization:

{
  "action": "completed",
  "check_suite": { ... },
  "repository": { ... },
  "sender": { ... },
  "organization": { ... },
  "installation": {
    "id": 1234,
    "node_id": "dummy_node_id"
  }
}

The type definitions should be updated to match the responses provided by Github.

wolfy1339 commented 5 years ago

The payload definitions are unfortunately auto generated by scraping the GitHub docs. If GitHub doesn't include a certain field in the docs then it won't be here at all.

It could be just a name clash with another type, but I'm not totally sure

gr2m commented 5 years ago

https://github.com/octokit/webhooks is scraping https://developer.github.com/v3/activity/events/types/ in a nightly cron job.

If the response for the check suite event is lacking fields, it would be great if you’d tell support@github.com. You can reference this issue and please keep us posted :pray: Once the documentation are updated, this issue should be resolved within a day or two via automated pull requests

acazacu commented 5 years ago

@gr2m Alright, on it! Will keep this issue updated with the progress.

ianwremmel commented 5 years ago

@acazacu any updates? (I'm running into similar issues)

gr2m commented 5 years ago

@ianwremmel what is the issue for you, specifically?

ianwremmel commented 5 years ago

Exactly what was described above. The installation property is missing property is missing from payloads.

gr2m commented 5 years ago

Okay thanks. The only thing we can do at this point is pinging support@github.com and asking for the payload to be fixed. You can reference this issue. I'm sure the docs team does what they can. Having more people report it will bump the priority though

ianwremmel commented 5 years ago

Will do. Can you explain a little more how the scraper works? I feel like the examples in the docs still show the effected fields, but I’ve been locked at 6.2.1 for a while because of all the breakages it caused.

When I’m back at that computer, I’ll figure out all the payload that seem wrong, but it would be good to know how to find the broken source data.

gr2m commented 5 years ago

I've described how the scraper works at https://github.com/octokit/webhooks#how-it-works

The check_suite payload example scraped from the current docs is here: https://github.com/octokit/webhooks/blob/bf6c391de4ab3841c996c12e2360e7778cc8ce48/index.json#L325-L548

Once the check_suite example payload in the docs is updated with the new fields, the nightly cron job on https://github.com/octokit/webhooks will pick up the change and create a pull request. Once merged, a new release is created which will then trigger a pull request in this repository, which again will trigger a new release once merged. Besides merging the PRs for safety reasons it's all automated.

ianwremmel commented 5 years ago

awesome, thanks! I'll pull the details together tonight and open a support ticket.

ianwremmel commented 5 years ago

Request Submitted. It was indeed the CheckSuite event (I've been pinned at 6.2.1 long enough that I couldn't recall if there were other issues). I'll post here if I get a response.

SnakyBeaky commented 5 years ago

I wonder why GitHub lacks useful documentation around webhooks and forces developers to build scrappers and guess fields based on responses instead.

gr2m commented 5 years ago

Hey Oriol, I understand you are frustrated, but your commented is unnecessary snarky. Very few documentations out there are perfect, it's a very hard job, and I know for a fact that GitHub's documentation team is doing an incredible job with the resources they have available.

If you want to help out, the idea was mentioned that we should parse the payload tables instead of the examples, see https://github.com/octokit/webhooks.js/issues/94. This should resolve most of the problems that people run into, and updating the payloads tables if they are incorrect or lacking information will be an easy fix, the support team will very much appreciate people reporting problems with it.

Thanks <3

SnakyBeaky commented 5 years ago

@gr2m sorry if my comment seemed offensive, it was not my intention.

I'm actually working in a project using GitHub webhooks and I came across the same issue using the official API documentation and octokit.net (using C#, not js/ts).

I haven't verified all events yet but looks like most of them fit the "payload table" in the documentation, except PushEvent, which differs a lot from the table.

ENikS commented 4 years ago

if you look at GitHub docs for LabelEvent you will see action, label, and changes but the definition missing changes;

Something else must be broken I guess.

oscard0m commented 4 years ago

if you look at GitHub docs for LabelEvent you will see action, label, and changes but the definition missing changes;

Something else must be broken I guess.

Seems to be the same issue as pointed before regarding the mismatch between the tables and the examples on a given event. If you check the example for LabelEvent the missing field changes is not there (and it's how it is scraped right now)

gr2m commented 4 years ago

The changes field exist now:

https://github.com/octokit/webhooks.js/blob/1804f39a7bff8c0ff2b6bf196ba3b21b803e53d4/index.d.ts#L2893-L2899

Regarding the original issue from @acazacu: WebhookPayloadCheckSuite now includes an optional key for "installation", but is still missing an "organization" key.

https://github.com/octokit/webhooks.js/blob/1804f39a7bff8c0ff2b6bf196ba3b21b803e53d4/index.d.ts#L4134-L4140

This issue needs to be resolved in https://github.com/octokit/webhooks, either by adding more example payloads, or by parsing the payload tables on https://developer.github.com/v3/activity/events/types/, as discussed here: https://github.com/octokit/webhooks.js/issues/94. In that particular case, the Events API Payload table is not complete, but I can ask the docs team to complete it

ENikS commented 4 years ago

If you are going to ask them, could they also include field installation ?

gr2m commented 4 years ago

note that with the latest release Webhook.WebhookPayloadCheckSuite is now EventPayloads.WebhookPayloadCheckSuite.

The missing organization type will be addressed by https://github.com/octokit/webhooks/issues/136. You can contribute example webhook event payloads directly to the repository, which will with the initial bootstrapping of the JSON Schema and future validation of it

wolfy1339 commented 4 years ago

I have sent in a PR that fixes this specific case octokit/webhooks#184

github-actions[bot] commented 4 years ago

:tada: This issue has been resolved in version 7.12.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket: