-
Notifications
You must be signed in to change notification settings - Fork 270
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
Add OpenAPI 3.1 webhook support #1135
Conversation
d48a2a5
to
1a5c039
Compare
Some things to consider, now that I look at this again:
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #1135 +/- ##
==========================================
- Coverage 98.59% 98.58% -0.01%
==========================================
Files 71 72 +1
Lines 8669 8766 +97
==========================================
+ Hits 8547 8642 +95
- Misses 122 124 +2 ☔ View full report in Codecov by Sentry. |
Hi @tfranzel, I hope you had a great start to the year! When you have some time, would you mind looking at this changeset and letting me know what you think of it? Is there anything I can do to help make it easier to review? |
hi @federicobond, sry for the delay. thanks, hope you had an equally good start. Excellent PR btw. So generally I like the PR. At first I was unsure about adding
yes I agree, but not because of the JSON-serialization. Importing too much stuff in I will look at it in detail in the upcoming days though. |
39379c6
to
79c3432
Compare
Makes sense. I just pushed a modification to show how it would look for webhooks to be defined in settings as a list of import paths for each individual event. |
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.
some comments about rearranging/simplifying stuff. The core itself I really like.
Just want to thank you both for continuing to develop features for this package. 🙏 |
a6e211e
to
0a97683
Compare
@tfranzel I think I have addressed all your comments. Added docs too. |
0a97683
to
111ef9c
Compare
I keep wondering if there is a declarative approach that can be built on top of this, where you decorate your serializer declarations and they get automatically registered as webhooks. |
One thing missing is versioning support. We could add an optional |
111ef9c
to
25aacb7
Compare
It is currently analogous to I removed the type (I asked for) again, because it was pesky due to django's lazy string. Not important enough to waste too much time there. Regarding Anyways, thanks @federicobond for this excellent PR. much appreciated! |
Thanks @tfranzel! On the topic of versioning (correct me if I'm wrong) the difference is that callbacks inherit the version from the operation they belong to, but webhooks are defined globally so they cannot do that. This is a feature I will need eventually, so I may open a pull request so we can discuss it separately. |
That is indeed correct, I have not thought about it that deeply. We can certainly pick this up again if you want, but I think this was a nice round package with a good scope. Extending it will be easier to reason about in a separate PR. |
Sure! This is a great start. |
Hi! This is an initial implementation of support for declarative OpenAPI 3.1 webhooks using a new OpenApiWebhook class. Let me know if this approach sounds good and what else (besides docs) would be missing for a production implementation.
The current implementation requires webhooks to be declared manually in the SPECTACULAR_SETTINGS configuration, I did not want to commit to a particular way of discovering webhooks at the moment.
References #665