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

Custom Webhook Templates #1089

Open
bkcsoft opened this issue Feb 28, 2017 · 39 comments · May be fixed by #19307
Open

Custom Webhook Templates #1089

bkcsoft opened this issue Feb 28, 2017 · 39 comments · May be fixed by #19307
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality.

Comments

@bkcsoft
Copy link
Member

bkcsoft commented Feb 28, 2017

Description

We should allow for custom webhooks. And while we're at it, migrate current webhooks into the new format (This is make it easy for people to remove slack-support (or rename to Mattermost) link to issue ).

I see this being interated in 5 steps (maybe less depending on size of PRs)

  1. Add a new webhook-type for custom payloads, this one reads {name}_payload.tmpl and logo {name}.png from options/webhooks/ and custom/webhooks/.
  2. Add UI for actually enabling them :trollface:
  3. Support for "Metadata", as in Name, and custom fields (for config-page). Should be in a file...
    Could also add support for URL-based icon (to reuse gitea-logo)
  4. Support for sending custom Headers
  5. Migrates all existing Slack & Gitea webhooks to the new format 🎉

The way I see it we'd end up with this:

/options/webhooks/
                 /gitea.png
                 /gitea_payload.tmpl
                 /gitea_headers.tmpl
                 /gitea_metadata.yml
                 /slack.png
                 /slack_payload.tmpl
                 /slack_headers.tmpl
                 /slack_metadata.yml
/custom/webhooks/
                /foobar.png
                /foobar_payload.tmpl
                /foobar_headers.tmpl
                /foobar_metadata.yml

Screenshots

Will try to make mockup screenshots

@bkcsoft bkcsoft added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality. labels Feb 28, 2017
@lunny lunny added this to the 1.x.x milestone Mar 1, 2017
@lunny lunny mentioned this issue Aug 29, 2017
6 tasks
@lunny
Copy link
Member

lunny commented May 18, 2018

How to resolve multiple languages issue?

@lunny lunny mentioned this issue May 21, 2018
7 tasks
@bkcsoft
Copy link
Member Author

bkcsoft commented Jun 17, 2018

Multiple language issue? You mean for the list? Do they need to be localized? AFAIK "Slack" would be "Slack" in all languages no?

@lunny
Copy link
Member

lunny commented Jun 18, 2018

OK. They still could customize their locale files.

@bkcsoft
Copy link
Member Author

bkcsoft commented Jul 4, 2018

The best part about having webhooks be templates, is that the templates would provide its own translations. So for any built-in templates Gitea provides, we would have separate translation-files for them in Crowd-In 🙂

@pinpox
Copy link

pinpox commented Apr 12, 2019

Any updates on this? I'd offer to help and submit a PR if someone can guide me a bit. Haven't hacked much on gitea yet, but really would like to use this feature

@lunny
Copy link
Member

lunny commented Apr 13, 2019

You could save all the webhook messages on template files and then read and execute template before send webhooks.

@alexanderadam
Copy link

alexanderadam commented Sep 27, 2019

This feature would solve a bunch of issues at once (i.e. #2139, #5252, #5267, #5496, #5548, #7488, #7548, #7788, #7973, #8473, #8963, #9000, #9134, #9485, #9504, #9746, #10288, #10418, #10719, #12712, #13063, #13592).

But what's more important: the maintenance probably becomes easier because people could simply add PRs for the templates (or have local custom ones) instead of having a growing hard coded list of webhook services that live in the Go source.

PS: Webhooks that doesn't come with an HTTP interface (i.e. #7549 or #5469) won't be solved by this naturally.

@guillep2k
Copy link
Member

PS: Webhooks that doesn't come with an HTTP interface (i.e. #7549 or #5469) won't be solved by this naturally.

We could allow the use of external commands for that, like we do for external renderers. That would cover a lot of ground.

@alexanderadam
Copy link

alexanderadam commented Sep 27, 2019

We could allow the use of external commands for that, like we do for external renderers. That would cover a lot of ground

Absolutely! That makes sense. I guess via template / GUI as well?
Something like

$ command $project $commit_hash $prev_commit_hash $project_tags static_param foo

would be nice.

@tacotexmex
Copy link

Badly needed for my Matrix integration going forward.

@lunny
Copy link
Member

lunny commented Oct 18, 2019

@guillep2k @alexanderadam But this has been implemented via git hooks? You can edit git hooks on repository settings UI.

@alexanderadam
Copy link

But this has been implemented via git hooks? You can edit git hooks on repository settings UI.

You are just referring on the external command integration, right?
That's true, I didn't think of that. Thank you!
The major topic of this issue is still open however. 😉

@guillep2k
Copy link
Member

@guillep2k @alexanderadam But this has been implemented via git hooks? You can edit git hooks on repository settings UI.

@lunny I don't know about that. AFAIU git hooks will run under Gitea's user, so they are only available to the site admin or selected users? What I was thinking was something any repo owner can chose from a closed-list, pre-defined by the admin in custom/otherhooks like we do with labels or gitignore.

@oliverpool
Copy link
Contributor

@jolheiser le me reply here to #19307 (comment), since this seems a better place to discuss about it

As far as I understand, it seems already agreed that gitea should offer a way for instance administrators to offer "custom webhook templates".

Looking at the current implementation and the issues for new webhook, it seems that what is actually needed is the possibility to "shape" the canonical JSON along some metadata to adapt to the different services. For instance matrix, Slack and telegram output a smaller JSON with a "text" field summarizing the changes.

So it seems that what is needed is way to transform a JSON document into another JSON document at runtime.


The current implementation is in Go, so it doesn't allow customization at runtime (requires a new compilation).


This issue mentions "templates" and *_payload.tmpl but does not show any example of such a template. I don't think that Go templates are the best for outputting JSON data (formatting and escaping is not trivial).

#1089 (comment) showed a *_payload.yml template with some string replacement (looking like a go template). So it would mean: executing as a go template and then parsing as YAML (which implies that the go templating must ensure to keep the yaml valid, with proper escaping).


In #19307 (comment) I propose bloblang, which is a document mapper, written in Go. It could read some "mapping" files and run them before delivering the payload.
Pros:

  • it has been designed for document mapping, so its interface is quite nice (input and output well defined)
  • it can integrate custom functions (to inject metadata, translation or ease the formatting)

Cons:

  • it is a new language to learn

Another possibility would be javascript #19307 (comment) proposed https://github.com/dop251/goja
Pros:

  • well-known language
  • it can integrate custom functions (to inject metadata, translation or ease the formatting)

Cons:

  • depending on the JS engine, it might not behave as expected (console.log not working for instance)
  • it will need strong convention regarding the code (how is the metadata and canonical json provided, how should the result be sent back)

What do you think of this overview?
Did you have anything else in mind for the *_payload.yml format?

@oliverpool
Copy link
Contributor

(if bloblang seems like a good idea, I would be willing to draft a PR to integrate it)

@jolheiser
Copy link
Member

I agree that describing payloads in YAML is going to be a horrible UX, which is why my current WIP PR didn't go that route.


The current implementation of that PR essentially allows defining a limited UI and passing the payload+form data to another bridge service. More or less a normal Gitea webhook + some limited form data.

As noted in that same PR, some other maintainers want to have it all in the DB, which has stalled the PR because I also think that's bad UX.

Regarding js vs bloblang, unless there's a real strong reason to use bloblang I would just as soon use JS. More people are going to be used to it (even if they don't like it), and considering there may be more complex logic depending on the form data (plus adding headers, etc etc) it seems like it may be a better fit.


It seems there are conflicting opinions on the very basics of how this should work currently.

My original PR included the ability to run custom scripts rather than requiring another running service, but it was scrapped in favor of bridge services (even though those are currently possible and yet not widely used)


In order to continue this, I think we need to get a clear answer on what is going to be allowed before we begin to discuss implementations.

  1. Are we going to confine custom webhooks to HTTP? This would mean every "custom webhook" would require another running bridge service.
  2. If not, how do we allow people to write transformation services? bloblang? JS? yaegi?
  3. Where do we store the config files/images? In the database? On disk?

To answer the above with my own opinions:

  1. I don't think we should. As I've mentioned, bridge services are already entirely possible (minus UI), but they haven't caught on.
  2. I think that some form of script-like way should be allowed. For smaller instances, running another service sounds silly imho
    I don't have a huge preference on how it's done, but I still don't think running a local command would be a dramatic amount of overhead. At least not any more than embedding an x-lang engine into the runtime.
  3. I think these should live on disk. imo it's the easier way to allow for installation, as well as development
    Having it in the DB, I'm not entirely sure what an install process would look like. "Here, copy/paste this YAML into your page"?

@oliverpool
Copy link
Contributor

  1. Are we going to confine custom webhooks to HTTP?

I would say yes. The goal of a webhook is to communicate with another service, very unlikely running on the same machine. If you can control what runs on the machine, then exposing a service via a local port is not hard to do (however this is only accessible to instance administrators, which represent a minority of the gitea users).

This would mean every "custom webhook" would require another running bridge service.

I don't understand what you mean. If I want to post a message to mattermost/matrix/discord every time a PR is opened, I only need to shape the payload (and add some Authorization headers). If the "custom webhook" allows this, I don't need a bridge.

The usecase I actually want to do, is trigger a sourcehut build.


  1. If not, how do we allow people to write transformation services? bloblang? JS? yaegi?

I would restrict "people" to "gitea-dev" and "instance-administrator".

Another possibility would be wasm.

  • pro: the "people" can write in any language they want
  • con: it is not hackable (you need the source and a compiler to modifiy a given webhook)

I think that it should be a language restricted enough (I fear, that if JS is allowed, the "people" will expect to be able to perform "fetch" requests directly from the script - which could be interesting, but not really a "webhook" anymore, more like a "hook").


  1. Where do we store the config files/images?

Actually I think we should take a step back and ask who should be allowed to customize what for the webhooks.

Currently:

  • the gitea-dev can add/modifiy/remove webhooks and expose some configuration (via Go code, which isn't very well contained - for instance matrix-logic appears in the deliver.go file)
  • the instance administrator can't adjust anything
  • the end-user can adjust some configuration regarding those webhooks

What workflows do we want to allow/ease?

Here is my take:

  • gitea-dev should be able to add/modify/remove webhooks easily (a PR for a new webhook should change less than 3 files - including tests)
  • the instance administrator should be able to add webhooks easily (without re-compiling), which exposes some config to the end-user
  • the end-user can "only" adjust some configuration regarding both webhooks types

I would refrain (at least for now) to allow the end-user to create a custom webhook.

so I agree with @jolheiser that the config files/images should live on disk (I do agree with you on something 🤗 :)

@jolheiser
Copy link
Member

@oliverpool Are you on Discord or Matrix by chance? I think perhaps it may be more effective to line up there and then post a summary here once we've aligned our thoughts.

@oliverpool
Copy link
Contributor

Thinking more about this the transformation service should always output an object, with a payload key. Something like:

{
    "payload":{
        "hash": "123456",
        "message": "en event happened"
    },
    "content_type": "application/json"
    // other keys can be added later. "headers" for instance
}

@Flashdown
Copy link

Flashdown commented Jan 26, 2023

For those who can't wait for this one to become ready and merged like me (#19307)

that would like to use Gitea Webhooks with Google Chat in the meanwhile, then I'd like to share with you that I've found a good workaround using a NodeJS proxy in a docker container that translates slack webhooks to google chat webhooks. It's working fine for me :)
-> https://github.com/chriseaton/slack-to-google-chat-webhook-proxy

@oliverpool
Copy link
Contributor

@jolheiser I recently looked at the webhook state and to allow moving forward, I would recommend splitting this issue in 2 steps:

  • refactor the webhook request generation logic (more below)
  • add custom/external/file-based webhook template (as a second step)

Currently to generate a webhook request, the following happens:

  1. webhook.PrepareWebhook is called, with the webhook model, the event type and the payload
  2. After some checks (event active, branch...), webhook.payloadCreator is called
  3. The result of webhook.payloadCreator (which calls convertPayloader) is saved as webhook_model.HookTask
  4. the webhook_model.HookTask is added to the queue
  5. when popped from the queue, the webhook.Deliver is called, with the webhook model and the HookTask
  6. Based on both models (webhook and hooktask), a request is created and sent
  7. The response is saved

I think a couple of things could be refactored:

2. webhook.payloadCreator

For all webhooks, it calls GetXXXPayload, which calls convertPayloader which calls the right method on the webhook model.

Instead of GetXXXPayload, a NewXXXHookHandler could be created, which returns an interface.
convertPayloader would then be called first calls the right method on the previous interface (if the method is missing, it means the event is not supported by the hook, or it could call a default method?).

Moreover the p api.Payloader and event webhook_module.HookEventType arguments of convertPayloader seem redundant: a type switch could probably happen on p (and dismiss the event argument).

3. & 5.

Currently the actual request is created at step 5. I think it could be created at step 3. and simply save in the database the whole request (which is done on step 6. anyway):

  • URL
  • Method
  • Body
  • Headers

So instead of func (m *MatrixPayload) IssueComment(p *api.IssueCommentPayload) (api.Payloader, error), it would be something like func (m *MatrixPayload) IssueComment(uuid string, p *api.IssueCommentPayload) (webhook_model.HookRequest, []byte, error) (uuid is the hook_id, needed for idempotency).

End result

  1. webhook.PrepareWebhook is called, with the webhook model, the event type and the payload
  2. After some checks (event active, branch), handler, err := webhook.NewXXXHookHandler(meta) is called
  3. hookRequest, body, err := createPayload(handler, p api.Payloader) is called and saved as webhook_model.HookTask
  4. the webhook_model.HookTask is added to the queue
  5. when popped from the queue, webhook.Deliver simply unmarshalls the request
  6. The request is sent
  7. The response is saved

I think generating the whole request upfront would make the logic much easier.
It may make adding new native hooks much easier.
It should help having custom webhooks, since it would formalize the output of the function: webhook_model.HookRequest, []byte, error (the input would still be an open question...).

Maybe this sounds to abstract and I should open a PR to show what I mean. What do you think?

@jolheiser
Copy link
Member

I would need to refresh myself a bit on the webhook code, but that sounds feasible at a glance.

I've been meaning to get back to this, but my TODO list never seems to decrease. 😔
If you'd like to work on the refactor PR that may be reasonable, I'd be happy to review.

@willzhang
Copy link

Does Gitea Webhook currently not support docking with argo events?

argoproj/argo-events#1431 (comment)

@oliverpool
Copy link
Contributor

@jolheiser (and anyone following here on this issue) I drafted a refactoring of the webhook_task database storage (proposed in #1089 (comment)) on codeberg: https://codeberg.org/forgejo/forgejo/pulls/2231

I plan to upstream this to gitea when this draft is complete.

I look forward to any feedback! (here, on matrix or on codeberg)

@lunny
Copy link
Member

lunny commented Jan 25, 2024

I still think we need a table to save the customized HookType. Since this table is necessary, some other informations can also be stored into this table. Those are meta information, I don't think storing them into hook_task is a good idea.

@oliverpool
Copy link
Contributor

@lunny my PR does not implement any customized HookType.

It is more of a step to simplify the hook interface, so that adding custom types (in later PRs) should be easier.

@lunny
Copy link
Member

lunny commented Jan 25, 2024

@lunny my PR does not implement any customized HookType.

It is more of a step to simplify the hook interface, so that adding custom types (in later PRs) should be easier.

I mean the migrations is unnecessary, we can put them into the hook_type table, especially for a big instance like codeberg.

@oliverpool
Copy link
Contributor

I mean the migrations is unnecessary, we can put them into the hook_type table, especially for a big instance like codeberg.

You are right, the impact of the migration must be checked, I will ask codeberg to run it on a copy of the database (it can likely be make faster by using parallelism).
To make it less work, I added a call to the cleanup_hook_task_table before the migration is run.

I think that this migration is worthwhile, because it enables a substantial cleanup of the webhook code. Defining a new webhook inside gitea (as Go files) will touch far fewer files after this PR (for instance the matrix hook currently has some logic inside the deliver.go file - #19307 has this as well).

The proposed PR is more of an intermediary step:

  • it does not enable custom hook types in any way
  • however it should make adding new hook types (as Go files) less involved (and hopefully add custom types easier as well).

@oliverpool
Copy link
Contributor

@lunny after considering the feedback that I received (inclusive yours, thank you for it!), I am proposing another approach: https://codeberg.org/forgejo/forgejo/pulls/2263 (saving the raw event as-is and doing the type-dependent logic right before sending).

With this new approach, the migration is much lighter (only adding a version field for backward compatibility) and the overall changes are fewer.

@oliverpool
Copy link
Contributor

@lunny regarding the specific logic added in services/webhook/payloader.go by #12046 (which got refactored as-is in #12310):

case models.HookEventIssueComment, models.HookEventPullRequestComment:
pl, ok := p.(*api.IssueCommentPayload)
if ok {
return s.IssueComment(pl)
}
return s.PullRequest(p.(*api.PullRequestPayload))

Do you know if this old (June 2020) comment of yours is still up-to-date? #11940 (comment)

That's because for HookEventPullRequestComment event, some places use IssueCommentPayload and others use PullRequestPayload

Looking at the current code:

  • modules/actions/workflows.go calls return matchIssueCommentEvent(commit, payload.(*api.IssueCommentPayload), evt) for all HookEvent***Comment
  • services/webhook/notifier.go all new Webhooks for HookEvent***Comment are of type *api.IssueCommentPayload

So I guess that this can be simplified to:

	case models.HookEventIssueComment, models.HookEventPullRequestComment:
		return s.IssueComment(p.(*api.IssueCommentPayload))

What do you think?

@oliverpool
Copy link
Contributor

@jolheiser I have a branch ready, for which I plan to open a PR here soon: https://codeberg.org/forgejo/forgejo/pulls/2263

Following the discussion with @lunny and other feedback, instead of saving the request to be delivered in the database, it saves the raw event and creates the webhook-dependent request right before delivering it.

Hence the webhook-specific logic is exists within Deliver, which should simplify the addition of other/custom webhook types.

Feel free to comment on codeberg, or wait for the PR here.

@oliverpool
Copy link
Contributor

PR has been opened here: #29145

@jessesanford
Copy link

Now that #29145 has been merged, is there a logical path forward for this?

@OdinVex
Copy link

OdinVex commented Sep 14, 2024

I would have thought custom would've been the first type to be supported because using variable replacement and a template you could support so very many right away...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging a pull request may close this issue.