Custom Webhook Templates #410

@lunny commented on GitHub (May 18, 2018):

How to resolve multiple languages issue?

@lunny commented on GitHub (May 18, 2018): How to resolve multiple languages issue?

GiteaMirror commented

@bkcsoft commented on GitHub (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?

@bkcsoft commented on GitHub (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?

GiteaMirror commented

@lunny commented on GitHub (Jun 18, 2018):

OK. They still could customize their locale files.

@lunny commented on GitHub (Jun 18, 2018): OK. They still could customize their locale files.

GiteaMirror commented

@bkcsoft commented on GitHub (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 🙂

@bkcsoft commented on GitHub (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 :slightly_smiling_face:

GiteaMirror commented

@pinpox commented on GitHub (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

@pinpox commented on GitHub (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

GiteaMirror commented

@lunny commented on GitHub (Apr 13, 2019):

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

@lunny commented on GitHub (Apr 13, 2019): You could save all the webhook messages on template files and then read and execute template before send webhooks.

GiteaMirror commented

@alexanderadam commented on GitHub (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.

@alexanderadam commented on GitHub (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](https://github.com/go-gitea/gitea/blob/c6fb7fe27c16c4e43d4d8dbe4d2ff4b3c4c52a29/models/webhook.go#L486-L525). **PS:** Webhooks that doesn't come with an HTTP interface (i.e. #7549 or #5469) won't be solved by this naturally.

GiteaMirror commented

@guillep2k commented on GitHub (Sep 27, 2019):

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.

@guillep2k commented on GitHub (Sep 27, 2019): > > > 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.

GiteaMirror commented

@alexanderadam commented on GitHub (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.

@alexanderadam commented on GitHub (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 ```bash $ command $project $commit_hash $prev_commit_hash $project_tags static_param foo ``` would be nice.

GiteaMirror commented

@tacotexmex commented on GitHub (Oct 11, 2019):

Badly needed for my Matrix integration going forward.

@tacotexmex commented on GitHub (Oct 11, 2019): Badly needed for my Matrix integration going forward.

GiteaMirror commented

@lunny commented on GitHub (Oct 18, 2019):

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

@lunny commented on GitHub (Oct 18, 2019): @guillep2k @alexanderadam But this has been implemented via git hooks? You can edit git hooks on repository settings UI.

GiteaMirror commented

@alexanderadam commented on GitHub (Oct 18, 2019):

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. 😉

@alexanderadam commented on GitHub (Oct 18, 2019): > 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. :wink:

GiteaMirror commented

@guillep2k commented on GitHub (Oct 22, 2019):

@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.

@guillep2k commented on GitHub (Oct 22, 2019): > > > @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.

GiteaMirror commented

@fawdlstty commented on GitHub (Nov 6, 2020):

How are the plans for this feature going?

@fawdlstty commented on GitHub (Nov 6, 2020): How are the plans for this feature going?

GiteaMirror commented

@WattsC-90 commented on GitHub (Nov 19, 2020):

this is a great feature if it can come in soon! want to integrate gitea with existing issue management tool without having to write a custom service in the middle to swap from one to the other! :)

@WattsC-90 commented on GitHub (Nov 19, 2020): this is a great feature if it can come in soon! want to integrate gitea with existing issue management tool without having to write a custom service in the middle to swap from one to the other! :)

GiteaMirror commented

@lunny commented on GitHub (Nov 19, 2020):

Different events should have different templates, so we can define a yml with multiple sections.

/options/webhooks/
                 /gitea/logo.png
                 /gitea/payload.yml
/custom/webhooks/
                /foobar/logo.png
                /foobar/payload.yml

name: gitea
headers:
metadata:
    channel: {{.Channel}}
events:
    create: 
        title: The {{refType}} {{refName}} has been created.
    deleted:
        title: ssssss
       ......

@lunny commented on GitHub (Nov 19, 2020): Different events should have different templates, so we can define a yml with multiple sections. ``` /options/webhooks/ /gitea/logo.png /gitea/payload.yml /custom/webhooks/ /foobar/logo.png /foobar/payload.yml ``` ```yml name: gitea headers: metadata: channel: {{.Channel}} events: create: title: The {{refType}} {{refName}} has been created. deleted: title: ssssss ...... ```

GiteaMirror commented

@londiniym commented on GitHub (Nov 17, 2021):

How are the plans for this feature going?

@londiniym commented on GitHub (Nov 17, 2021): How are the plans for this feature going?

GiteaMirror commented

@nwmcsween commented on GitHub (Dec 31, 2021):

Add a bounty to get this moving faster, see: https://www.bountysource.com/issues/42577911-custom-webhook-templates

I've added $40USD to this

@nwmcsween commented on GitHub (Dec 31, 2021): Add a bounty to get this moving faster, see: https://www.bountysource.com/issues/42577911-custom-webhook-templates I've added $40USD to this

GiteaMirror commented

@oliverpool commented on GitHub (Jul 7, 2022):

@jolheiser le me reply here to https://github.com/go-gitea/gitea/pull/19307#issuecomment-1177731366, 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).

https://github.com/go-gitea/gitea/issues/1089#issuecomment-730341962 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 https://github.com/go-gitea/gitea/pull/19307#issuecomment-1177141658 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 https://github.com/go-gitea/gitea/pull/19307#issuecomment-1126666676 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 commented on GitHub (Jul 7, 2022): @jolheiser le me reply here to https://github.com/go-gitea/gitea/pull/19307#issuecomment-1177731366, 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](https://docs.gitea.io/en-us/webhooks/#event-information) 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). https://github.com/go-gitea/gitea/issues/1089#issuecomment-730341962 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 https://github.com/go-gitea/gitea/pull/19307#issuecomment-1177141658 I propose [bloblang](https://www.benthos.dev/docs/guides/bloblang/about), 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 https://github.com/go-gitea/gitea/pull/19307#issuecomment-1126666676 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?

GiteaMirror commented

@oliverpool commented on GitHub (Jul 7, 2022):

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

@oliverpool commented on GitHub (Jul 7, 2022): (if `bloblang` seems like a good idea, I would be willing to draft a PR to integrate it)

GiteaMirror commented

@jolheiser commented on GitHub (Jul 7, 2022):

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.

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

To answer the above with my own opinions:

I don't think we should. As I've mentioned, bridge services are already entirely possible (minus UI), but they haven't caught on.
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.
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"?

@jolheiser commented on GitHub (Jul 7, 2022): 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"?

GiteaMirror commented

@oliverpool commented on GitHub (Jul 8, 2022):

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.

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").

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 🤗 :)

@oliverpool commented on GitHub (Jul 8, 2022): > 1. Are we going to confine custom webhooks to HTTP? I would say yes. The goal of a **web**hook 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](https://github.com/go-gitea/gitea/pull/20267)). If the "custom webhook" allows this, I don't need a bridge. _The usecase I actually want to do, is trigger a sourcehut build._ --- > 2. 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"). --- > 3. 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 :hugs: :)

GiteaMirror commented

@jolheiser commented on GitHub (Jul 8, 2022):

@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.

@jolheiser commented on GitHub (Jul 8, 2022): @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.

GiteaMirror commented

@oliverpool commented on GitHub (Jul 26, 2022):

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
}

@oliverpool commented on GitHub (Jul 26, 2022): Thinking more about this the `transformation service` should always output an object, with a `payload` key. Something like: ```json { "payload":{ "hash": "123456", "message": "en event happened" }, "content_type": "application/json" // other keys can be added later. "headers" for instance } ```

GiteaMirror commented

@Flashdown commented on GitHub (Jan 26, 2023):

For those who can't wait for this one to become ready and merged like me (https://github.com/go-gitea/gitea/pull/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

@Flashdown commented on GitHub (Jan 26, 2023): For those who can't wait for this one to become ready and merged like me (https://github.com/go-gitea/gitea/pull/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

GiteaMirror commented

@oliverpool commented on GitHub (Mar 14, 2023):

@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:

webhook.PrepareWebhook is called, with the webhook model, the event type and the payload
After some checks (event active, branch...), webhook.payloadCreator is called
The result of webhook.payloadCreator (which calls convertPayloader) is saved as webhook_model.HookTask
the webhook_model.HookTask is added to the queue
when popped from the queue, the webhook.Deliver is called, with the webhook model and the HookTask
Based on both models (webhook and hooktask), a request is created and sent
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

webhook.PrepareWebhook is called, with the webhook model, the event type and the payload
After some checks (event active, branch), handler, err := webhook.NewXXXHookHandler(meta) is called
hookRequest, body, err := createPayload(handler, p api.Payloader) is called and saved as webhook_model.HookTask
the webhook_model.HookTask is added to the queue
when popped from the queue, webhook.Deliver simply unmarshalls the request
The request is sent
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?

@oliverpool commented on GitHub (Mar 14, 2023): @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` 5. the `webhook_model.HookTask` is added to the queue 6. when popped from the queue, `webhook.Deliver` simply unmarshalls the request 7. The request is sent 8. 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?

GiteaMirror commented

@jolheiser commented on GitHub (Mar 15, 2023):

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.

@jolheiser commented on GitHub (Mar 15, 2023): 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.

GiteaMirror commented

https://github.com/argoproj/argo-events/issues/1431#issuecomment-1844979706

@weironz commented on GitHub (Dec 7, 2023):

Does Gitea Webhook currently not support docking with argo events？

@weironz commented on GitHub (Dec 7, 2023): Does Gitea Webhook currently not support docking with argo events？ https://github.com/argoproj/argo-events/issues/1431#issuecomment-1844979706

GiteaMirror commented

@oliverpool commented on GitHub (Jan 25, 2024):

@jolheiser (and anyone following here on this issue) I drafted a refactoring of the webhook_task database storage (proposed in https://github.com/go-gitea/gitea/issues/1089#issuecomment-1467775828) 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)

@oliverpool commented on GitHub (Jan 25, 2024): @jolheiser (and anyone following here on this issue) I drafted a refactoring of the webhook_task database storage (proposed in https://github.com/go-gitea/gitea/issues/1089#issuecomment-1467775828) 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)

GiteaMirror commented

@lunny commented on GitHub (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.

@lunny commented on GitHub (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.

GiteaMirror commented

@oliverpool commented on GitHub (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.

@oliverpool commented on GitHub (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.

GiteaMirror commented

@lunny commented on GitHub (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.

@lunny commented on GitHub (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.

GiteaMirror commented

@oliverpool commented on GitHub (Jan 25, 2024):

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 commented on GitHub (Jan 25, 2024): > 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).

GiteaMirror commented

@oliverpool commented on GitHub (Jan 29, 2024):

@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 commented on GitHub (Jan 29, 2024): @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.

GiteaMirror commented

@oliverpool commented on GitHub (Jan 30, 2024):

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

Do you know if this old (June 2020) comment of yours is still up-to-date? https://github.com/go-gitea/gitea/issues/11940#issuecomment-645713996

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 commented on GitHub (Jan 30, 2024): @lunny regarding the specific logic added in `services/webhook/payloader.go` by #12046 (which got refactored as-is in #12310): https://github.com/go-gitea/gitea/blob/b51bd7f1d62cd623601185162d84ac29adb25366/modules/webhook/payloader.go#L37-L42 Do you know if this old (June 2020) comment of yours is still up-to-date? https://github.com/go-gitea/gitea/issues/11940#issuecomment-645713996 > 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: ```go case models.HookEventIssueComment, models.HookEventPullRequestComment: return s.IssueComment(p.(*api.IssueCommentPayload)) ``` What do you think?

GiteaMirror commented

@oliverpool commented on GitHub (Feb 6, 2024):

@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 commented on GitHub (Feb 6, 2024): @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.

GiteaMirror commented

@oliverpool commented on GitHub (Feb 12, 2024):

PR has been opened here: #29145

@oliverpool commented on GitHub (Feb 12, 2024): PR has been opened here: #29145

GiteaMirror commented