-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
feat: Add new report, and alert type: webhook #30044
base: master
Are you sure you want to change the base?
Conversation
- new report type - new notification logic - update NotificationMethod.tsx with webhook - update config with WEBHOOK_SECRET
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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #30044 +/- ##
===========================================
+ Coverage 60.48% 70.84% +10.35%
===========================================
Files 1931 1990 +59
Lines 76236 80378 +4142
Branches 8568 9154 +586
===========================================
+ Hits 46114 56940 +10826
+ Misses 28017 21216 -6801
- Partials 2105 2222 +117
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Really cool idea @kistoth90! |
- add license template to webhook.py - revert label rename
I love the idea here. Approving the next CI run... ping me if it needs to be kicked again. |
At this point I don't know what, or how should I fix the errors:
I would appreciate some assistance about, becauseI'm stuck. |
""" | ||
Retrieves the secret token for HMAC signature from the application configuration. | ||
""" | ||
with app.app_context(): |
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.
do we need to create a new app_context here?
return hmac.new( | ||
secret.encode("utf-8"), | ||
json.dumps(data).encode("utf-8"), | ||
digestmod="sha256", |
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.
digestmod could be a new configuration key
return json.loads(self._recipient.recipient_config_json)["target"] | ||
|
||
def _generate_signature( | ||
self, data: Dict[str, Optional[str | None | int]] |
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.
we still support python 3.9, in order to use the new union type annotation we need to import:
from __future__ import annotations
return None | ||
|
||
def _get_header( | ||
self, data: Dict[str, Optional[str | None | int]] |
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 using from __future__ import annotations
let's change this to:
def _get_header(
self, data: dict[str, Optional[str | None | int]]
...
), | ||
"content_format": self._content.header_data.get("notification_format"), | ||
"metadata": json.loads(self._recipient.recipient_config_json).get( | ||
"JSONData" |
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.
WDYT about creating a TypeDict to make this payload structure more explicit
timeout=10, | ||
) | ||
response.raise_for_status() | ||
logger.info("Report webhook has been successfully sent to {host}") |
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.
use:
logger.info("Report webhook has been successfully sent to %s", host)
instead
For the Pre-commit issue, have you tried running ruff and black locally to remove the formatting blocks? |
- add __future__ package - remove app_context from _get_secre_token() - move digestmod to config as constant - remove metadata from payload - fix logger.info message
Hi @fisjac, Yepp, the ruff, and Blacklist passed as well! I made some changes by the guidance of dpgaspar. Let's hope the best! 🤞 |
Closing and reopening to try to get tests to kick off. |
SUMMARY
I add webhook as a new selectable report type.
By the use of webhook the user can post csv/png/pdf to any endpoint, with some informative parameters as:
For security reasons I made a new optional parameter: WEBHOOK_SECRET. It's a secret string between the Superset and the webhook endpoint. In the case where WEBHOOK_SECRET is filled, Superset adds an "X-Webhook-Signature" parameter to the header of the post call, which hashes the json data to be sent. The receiving party can verify that the party sending the webhook is the real sender by hashing the received json data and comparing it with the sending party's "X-Webhook-Signature" parameter. If the two parameters do not match, the receiving party may reject the call because the sender is not the supposed host.
On the report log, we can get feedback about the successful, and not successful requests as well.
I'm happy to share my code with you guys!
TESTING INSTRUCTIONS
By the result you will get the selectend chart/dashboard file on your webhook endpoint with some informative parameters as:
ADDITIONAL INFORMATION