-
Notifications
You must be signed in to change notification settings - Fork 777
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
send webhook when task or workflow run is canceled #1374
send webhook when task or workflow run is canceled #1374
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to ac4a400 in 25 minutes and 27 seconds
More details
- Looked at
154
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. skyvern/forge/agent.py:1546
- Draft comment:
The change to acceptlast_step
asStep | None
inexecute_task_webhook
is appropriate and consistent with its usage. - Reason this comment was not posted:
Confidence changes required:0%
The change inexecute_task_webhook
method to acceptlast_step
asStep | None
is appropriate, as it allows for flexibility when the last step might not be available. This change is consistent with the usage in the codebase.
2. skyvern/forge/sdk/workflow/service.py:888
- Draft comment:
The change to acceptapi_key
asstr | None
inexecute_workflow_webhook
is appropriate and consistent with its usage. - Reason this comment was not posted:
Confidence changes required:0%
The change inexecute_workflow_webhook
method to acceptapi_key
asstr | None
is appropriate, as it allows for cases where the API key might not be provided. This change is consistent with the usage in the codebase.
3. skyvern/forge/sdk/routes/agent_protocol.py:287
- Draft comment:
The addition ofx_api_key
as a header incancel_task
is appropriate for passing API keys for authentication. - Reason this comment was not posted:
Confidence changes required:0%
The change incancel_task
to includex_api_key
as a header is consistent with the need to pass API keys for authentication and is used correctly in the function.
4. skyvern/forge/sdk/routes/agent_protocol.py:308
- Draft comment:
The addition ofx_api_key
as a header incancel_workflow_run
is appropriate for passing API keys for authentication. - Reason this comment was not posted:
Confidence changes required:0%
The change incancel_workflow_run
to includex_api_key
as a header is consistent with the need to pass API keys for authentication and is used correctly in the function.
Workflow ID: wflow_mRaU7xEO2B9PxSqw
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -292,16 +293,28 @@ async def cancel_task( | |||
status_code=status.HTTP_404_NOT_FOUND, | |||
detail=f"Task not found {task_id}", | |||
) | |||
await app.agent.update_task(task_obj, status=TaskStatus.canceled) | |||
task = await app.agent.update_task(task_obj, status=TaskStatus.canceled) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shall we just only send the webhook when task/workflow cancellation is finished? any specific reason why we must send the webhook in the API call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if user call multiple times to the cancle endpoint. he will receive multiple times the webhook without any task/workflow being cancelled 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
users can also cancel tasks that are stuck. our worker won't really send a webhook if we don't send a webhook to them in the API endpoint.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it's okay that we send two cancel task webhook.
workflow_run = await app.DATABASE.get_workflow_run(workflow_run_id=workflow_run_id) | ||
if not workflow_run: | ||
raise HTTPException( | ||
status_code=status.HTTP_404_NOT_FOUND, | ||
detail=f"Workflow run not found {workflow_run_id}", | ||
) | ||
await app.WORKFLOW_SERVICE.mark_workflow_run_as_canceled(workflow_run_id) | ||
await app.WORKFLOW_SERVICE.execute_workflow_webhook(workflow_run, api_key=x_api_key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question as task
Important
Send webhooks when tasks or workflow runs are canceled by updating
execute_step()
,execute_workflow()
, and related functions to handle webhook execution.need_call_webhook=True
inexecute_step()
inagent.py
andexecute_workflow()
inservice.py
.cancel_task()
andcancel_workflow_run()
inagent_protocol.py
to includex_api_key
for webhook execution.execute_task_webhook()
inagent.py
to acceptlast_step
asStep | None
.execute_workflow_webhook()
inservice.py
to handle webhook execution for workflow runs.service.py
for webhook execution.This description was created by for ac4a400. It will automatically update as commits are pushed.