-
Notifications
You must be signed in to change notification settings - Fork 18
feat: webhook事件补充 --story=129476203 #582
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
base: master
Are you sure you want to change the base?
Conversation
# Reviewed, transaction id: 71465
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.
代码审查
此 PR 为任务操作和模板发布事件添加了 webhook 广播功能。发现以下关键问题需要处理:
🚨 关键问题
bkflow/task/views.py:206-213
interface_client.broadcast_task_events() 调用没有异常处理。如果 webhook 广播失败,可能导致整个操作失败或错误被静默忽略,影响业务流程。
建议:
try:
interface_client.broadcast_task_events(data={...})
except Exception as e:
logger.error(f"Broadcast event failed: {e}")
# 不应阻塞主流程bkflow/apigw/views/operate_task.py:50-55
event_broadcast_signal.send() 直接发送信号,而 bkflow/task/views.py:206 使用 InterfaceModuleClient().broadcast_task_events()。两种不同的机制可能导致事件处理不一致。
建议统一使用同一种广播机制。
bkflow/constants.py:35
OPERATE_EVENT 字典定义在模块顶层且没有类型提示,与下方的枚举类风格不一致,且字符串值(如 "task_paused")需要与 WebhookEventType 枚举同步维护。
建议:将映射关系放入枚举类或使用枚举值构建字典。
✨ 总结
- 需要为外部 API 调用添加异常处理
- 统一事件广播机制(signal vs API client)
- 改进常量定义的可维护性
# Reviewed, transaction id: 71494
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.
代码审查总结
此次审查针对任务操作和模板发布事件的 webhook 广播功能进行了复核。之前提出的关键问题仍需处理:
🚨 待解决问题
- 错误处理缺失 (
bkflow/task/views.py:206-213) - webhook 广播调用缺少异常处理 - 广播机制不一致 - 两处使用不同的广播方式(signal vs API client)
- 常量定义不规范 - OPERATE_EVENT 字典维护性较差
✅ 改进点
- 测试覆盖完善,已为相关操作添加 mock
- 事件类型枚举完整,覆盖新增的所有操作场景
建议优先处理错误处理和机制统一问题,以确保系统稳定性。
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.
代码审查复核
已检查当前提交,之前提出的关键问题仍未解决:
🚨 待处理问题
bkflow/task/views.py:206-213
bkflow/apigw/views/operate_task.py:50 vs bkflow/task/views.py:206
🔒 机制不一致:两处使用不同的广播方式(event_broadcast_signal.send() vs InterfaceModuleClient),可能导致事件处理行为不同。建议统一机制。
bkflow/constants.py:133-138
✨ 维护性问题:OPERATE_EVENT_MAP 使用字符串 .value 映射,需手动保持与枚举同步。建议直接使用枚举值或生成映射。
✅ 已完善
- 测试覆盖良好(mock 已添加)
- 事件类型完整
建议优先处理错误处理和机制统一,确保稳定性。
# Reviewed, transaction id: 71508
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.
代码审查总结
本次审查识别出 3 个关键问题需要关注:
主要发现
-
⚠️ 事件广播缺少错误处理 - 在 和 中,webhook 事件广播调用没有 try-catch,可能导致操作成功但事件发送失败时用户不可见 -
⚠️ 操作顺序问题 - 在 中,webhook 事件是在return result之前发送,但如果事件发送失败会抛异常影响正常响应 -
⚠️ 测试覆盖不足 - 测试仅 mock 了 broadcast 方法但未验证调用参数和失败场景
建议
参考 bkflow/task/celery/tasks.py:95-108 中的模式,建议对 webhook 广播添加异常捕获和日志记录,确保事件发送失败不影响主业务流程。
✨ 新增的 webhook 事件类型定义清晰,映射关系设计合理
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.
代码审查总结
本次审查识别出 3 个关键问题需要关注:
主要发现
-
⚠️ 事件广播缺少错误处理 - 在 bkflow/apigw/views/operate_task.py 和 bkflow/task/views.py 中,webhook 事件广播调用没有 try-catch,可能导致操作成功但事件发送失败时用户不可见 -
⚠️ 操作顺序问题 - 在 bkflow/apigw/views/operate_task.py:51 中,webhook 事件是在 return result 之前发送,但如果事件发送失败会抛异常影响正常响应 -
⚠️ 测试覆盖不足 - 测试仅 mock 了 broadcast 方法但未验证调用参数和失败场景
建议
参考 bkflow/task/celery/tasks.py:95-108 中的模式,建议对 webhook 广播添加异常捕获和日志记录,确保事件发送失败不影响主业务流程。
✨ 新增的 webhook 事件类型定义清晰,映射关系设计合理
| - code: task_resumed | ||
| name: 任务恢复 | ||
| - code: task_revoked | ||
| name: 任务撤销 |
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.
还需要补充一下webhook接口的文档,这类文档的需求可以试着让AI来生成,比如让AI根据你分支的改动,总结并完善某份文档
Reviewed, transaction id: 71465