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

[TaskProcessing] Add support for webhooks #46579

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

julien-nc
Copy link
Member

@julien-nc julien-nc commented Jul 17, 2024

  • 2 new columns/attributes for tasks: webhookUri and webhookMethod
  • When a task finishes (is canceled, fails or succeeds), we try to "run" the webhook
  • If the method is "HTTP:GET", "HTTP:POST"... we make a simple HTTP request
  • If the method is "AppAPI:APP_ID:HTTP_METHOD" we directly send a request to the exApp
  • The request body contains the task ID and status

Questions:

  • Do we want to combine so much information in the method column/attr?
  • What do we want to include in the request body/headers?

@julien-nc julien-nc force-pushed the feat/taskprocessing/webhooks branch from d32cbc3 to eae5fcb Compare July 17, 2024 13:27
@julien-nc julien-nc marked this pull request as ready for review July 17, 2024 13:33
Copy link
Member

@bigcat88 bigcat88 left a comment

Choose a reason for hiding this comment

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

LGTM

@provokateurin
Copy link
Member

I'm not sure I understand the purpose of these changes. Is this part of the new webhook stuff or specific to task processing api? If it is the former then it seems wrong to me to implement it in the task processing api? Maybe I'm missing something though.

@julien-nc julien-nc force-pushed the feat/taskprocessing/webhooks branch 2 times, most recently from e107dfc to 5fac324 Compare July 19, 2024 12:13
@julien-nc
Copy link
Member Author

It seems like the 3rd party repo is not up to date in master. The Block merging with outdated 3rdparty action will be fixed when someone pushes the new subproject commit hash.

I don't really get why the OpenAPI action is failing though.

@provokateurin
Copy link
Member

The OpenAPI failure is random, it seems to happen once in a while without an apparent reason.

@julien-nc
Copy link
Member Author

@provokateurin If we think about the workflow engine, it would indeed be possible in a flow to:

  • schedule a task, get its ID
  • run an custom approval script to block the flow execution
  • in the approval script, register a webhook for the "task finished" event with a filter on the task ID, setting the target URI to the resume endpoint of the approval script
  • after the approval step is resumed, run a script that unregisters the webhook because we don't need it anymore

But it is way more simple to:

  • run an approval script
  • schedule a task in the approval script and tell the task processing API to call the resume endpoint when the task has finished

Having the webhook_listeners app does not prevent us from implementing webhook-like features in other places where it's convenient. That's the reasoning behind this PR.

@julien-nc julien-nc force-pushed the feat/taskprocessing/webhooks branch from 5fac324 to bb393a7 Compare July 19, 2024 23:58
…e task processing API

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@julien-nc julien-nc force-pushed the feat/taskprocessing/webhooks branch from bb393a7 to 78c09b5 Compare July 22, 2024 09:36
@julien-nc julien-nc merged commit 98f2fb3 into master Jul 22, 2024
171 checks passed
@julien-nc julien-nc deleted the feat/taskprocessing/webhooks branch July 22, 2024 10:52
@blizzz blizzz mentioned this pull request Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants