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

Migrate webhook endpoint to type=http #1019

Open
xmo-odoo opened this issue Dec 13, 2024 · 0 comments
Open

Migrate webhook endpoint to type=http #1019

xmo-odoo opened this issue Dec 13, 2024 · 0 comments
Labels

Comments

@xmo-odoo
Copy link
Collaborator

Now that the mergebot is in 17.0, type=json shouldn't be necessary to receive json content types, and it would allow returning richer responses to github.

So go back to check the webhooks handler doc and improve the thing.

@github-project-automation github-project-automation bot moved this to ideas in Mergebot Dec 13, 2024
@xmo-odoo xmo-odoo moved this from ideas to accepted in Mergebot Dec 13, 2024
@xmo-odoo xmo-odoo moved this from accepted to done in Mergebot Dec 18, 2024
xmo-odoo added a commit that referenced this issue Dec 19, 2024
The old controller system required `type='json'` which only did
JSON-RPC and prevented returning proper responses.

As of 17.0 this is not the case anymore, `type='http'` controllers can
get `content-type: application/json` requests just fine and return
whatever they want. So change that:

- `type='json'`.
- Return `Response` objects with nice status codes (and text
  mimetypes, as otherwise werkzeug defaults to html).
- Update ping to bypass normal flow as otherwise it requires
  authentication and event sources which is annoying (it might be a
  good idea to have both in order to check for configuration, however
  it's not possible to just send a ping via the webhook UI on github
  so not sure how useful that would be).
- Add some typing and improve response messages while at it.

Note: some "internal" errors (e.g. ignoring event actions) are
reported as successes because as far as I can tell webhooks only
support success (2xy) / failure (4xy, 5xy) and an event that's ignored
is not really *failed* per se.

Some things are reported as failures even though they are on the edge
because that can be useful to see what's happening e.g. comment too
large or unable to lock rows.

Fixes #1019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: done
Development

No branches or pull requests

1 participant