Skip to content

fix: add event examples with installation key that don't have them yet #195

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Oct 14, 2020
Merged

fix: add event examples with installation key that don't have them yet #195

merged 7 commits into from
Oct 14, 2020

Conversation

oscard0m
Copy link
Member

@oscard0m oscard0m commented Oct 13, 2020 •
edited
Loading

📝 Summary

Add installation optional paramater to Webhook Payloads missing it.

⛱ Motivation and Context

octokit/webhooks.js#266
Fix #198

@oscard0m oscard0m added the Type: Bug Something isn't working as documented label Oct 13, 2020
@oscard0m oscard0m requested a review from gr2m October 13, 2020 23:22
@oscard0m oscard0m self-assigned this Oct 13, 2020
@gr2m gr2m self-assigned this Oct 14, 2020
gr2m
gr2m requested changes Oct 14, 2020
View reviewed changes
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

remove the example payloads for installation and installation_repositories that don't have the installation key. Also the installation always has a node_id which is a string besides the id key

...amples/api.github.com/installation_repositories.added.payload.without-installation-prop.json Outdated
"type": "User",
"site_admin": false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The installation_repositories event has always the installation key.

Copy link
Member Author

@oscard0m oscard0m Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I doubted about this one because, according to docs, it says

The GitHub App installation. Webhook payloads contain the installation property when the event is configured for and sent to a GitHub App.

https://docs.github.com/en/free-pro-team@latest/developers/webhooks-and-events/webhook-events-and-payloads#installation_repositories

If it's like this, I'm going to PR to Github Docs :)

Copy link
Contributor

Choose a reason for hiding this comment

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

go PR the docs, I'm very sure that "installation" is always present for these two particular events. They will let us know if otherwise. You can reference this PR

payload-examples/api.github.com/installation.created.payload.without-installation-prop.json Outdated
"type": "User",
"site_admin": false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The installation event has always an installation key

Copy link
Member Author

@oscard0m oscard0m Oct 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I doubted about this one because, according to docs, it says

The GitHub App installation. Webhook payloads contain the installation property when the event is configured for and sent to a GitHub App.

https://docs.github.com/en/free-pro-team@latest/developers/webhooks-and-events/webhook-events-and-payloads#installation

If it's like this, I'm going to PR to Github Docs :)

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case the "installation" object is the key part of the payload, it has many properties beyond just id and node_id. I'm sure that it's always present

@oscard0m oscard0m mentioned this pull request Oct 14, 2020
Closed
@oscard0m
Copy link
Member Author

oscard0m commented Oct 14, 2020 •
edited
Loading

Removed installation and installation_repositories examples without 'installation' property until github/docs#516 is clarified to not block octokit/webhooks.js#266

Once Github Docs provides feedback, a separate PR would be opened in case it's necessary to tackle installation? prop.

@oscard0m oscard0m requested a review from gr2m October 14, 2020 05:50
@gr2m gr2m changed the title fix(index.json): add installation optional prop to Payloads without it fix: add event examples with installation key that don't have them yet Oct 14, 2020
@gr2m gr2m merged commit 2ce9972 into master Oct 14, 2020
@gr2m gr2m deleted the add-optional-installation-property-to-webhook-payloads branch October 14, 2020 05:54
@gr2m
Copy link
Contributor

gr2m commented Oct 14, 2020

Great work!

@github-actions
Copy link
Contributor

🎉 This PR is included in version 3.18.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@oscard0m
Copy link
Member Author

Removed installation and installation_repositories examples without 'installation' property until github/docs#516 is clarified to not block octokit/webhooks.js#266

Once Github Docs provides feedback, a separate PR would be opened in case it's necessary to tackle installation? prop.

Confirmed by Github Docs team. The property is there 100% so no need to do a followup PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gr2m gr2m gr2m approved these changes

@wolfy1339 wolfy1339 wolfy1339 approved these changes

Assignees

@gr2m gr2m

@oscard0m oscard0m

Labels
Type: Bug Something isn't working as documented
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for optional parameter installation for Webhook Payloads missing it
3 participants
@oscard0m @gr2m @wolfy1339