-
Notifications
You must be signed in to change notification settings - Fork 435
feat: Test webhook from backend #5354
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: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
api/environments/views.py
Outdated
T = TypeVar('T', bound=Model) | ||
|
||
@runtime_checkable | ||
class HasObjects(Protocol[T]): | ||
objects: Manager[T] | ||
|
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.
Curious to have your opinion on this typing and cast @khvn26 before using it more (it's to tackle this environments/views.py:291: error: "type[T]" has no attribute "objects" [attr-defined]
)
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 understand why it's done here, but I don't think this is necessarily a good solution.
I'd try following the official way to deal with this first by using the ._default_manager
attribute.
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.
Ah perfect thanks I will look into it
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.
Came up with some comments/questions.
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 believe the recent changes are a massive improvement, good job 👍
There's still a couple comments I'd like you to address, though:
-
I'm not particularly fond of using static sample data, though, as it can get stale easily. Ideally, we should be able to reuse existing webhook tasks in the check.
-
Skimming through the code, I've found (currently unused?)
trigger_sample_webhook
utility function andTriggerSampleWebhookMixin
that we need to either refactor and integrate into your work, or get rid of.
for more information, see https://pre-commit.ci
@khvn26 made the changes. To answer your questions:
|
for more information, see https://pre-commit.ci
…webhook-from-backend
for more information, see https://pre-commit.ci
Thanks for submitting a PR! Please check the boxes below:
docs/
if required so people know about the feature!Changes
The goal of this PR is to move the webhook testing logic from the frontend (with custom method to mimic backend signature) to the backend
Frontend payload exemple (audit log)
WebhookViewSet
withtest
endpointscope
payload object (on top of existing org/env pk from url)Webhook returned error status: 400
)How did you test this code?
example code:
pip install flask flask-cors
python server.py