You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/06/01 13:14:47 UTC
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request #9810: feat: superset report slack integration
dpgaspar commented on a change in pull request #9810:
URL: https://github.com/apache/incubator-superset/pull/9810#discussion_r433210752
##########
File path: setup.py
##########
@@ -102,6 +102,7 @@ def get_git_sha():
"retry>=0.9.2",
"selenium>=3.141.0",
"simplejson>=3.15.0",
+ "slackclient>=2.5.0",
Review comment:
Can we bump to the latest 2.6.2, and what do you think about making this requirement optional (extras)
##########
File path: requirements.txt
##########
@@ -83,6 +83,7 @@ six==1.14.0 # via bleach, cryptography, flask-jwt-extended, flask-
sqlalchemy-utils==0.36.4 # via apache-superset (setup.py), flask-appbuilder
sqlalchemy==1.3.16 # via alembic, apache-superset (setup.py), flask-sqlalchemy, marshmallow-sqlalchemy, sqlalchemy-utils
sqlparse==0.3.1 # via apache-superset (setup.py)
+slackclient==2.5.0 # via apache-superset (setup.py)
Review comment:
Can we bump to the latest 2.6.2
##########
File path: superset/tasks/schedules.py
##########
@@ -307,20 +369,26 @@ def _get_slice_data(schedule: SliceEmailSchedule) -> EmailContent:
link=slice_url_user_friendly,
)
- elif schedule.delivery_type == EmailDeliveryType.attachment:
+ elif delivery_type == EmailDeliveryType.attachment:
data = {__("%(name)s.csv", name=slc.slice_name): content}
body = __(
'<b><a href="%(url)s">Explore in Superset</a></b><p></p>',
name=slc.slice_name,
url=slice_url_user_friendly,
)
- return EmailContent(body, data, None)
+ # how to: https://api.slack.com/reference/surfaces/formatting
+ slack_message = f"""
+ *{slc.slice_name}*\n
+ <{slice_url_user_friendly}|Explore in Superset>
Review comment:
We could use babel to translate "Explore in Superset"
##########
File path: superset/views/schedules.py
##########
@@ -83,6 +83,11 @@ class EmailScheduleView(
description="List of recipients to send test email to. "
"If empty, we send it to the original recipients",
),
+ "test_slack_channel": StringField(
+ "Test Slack Channel",
+ default=None,
+ description="A slack channel to send test message to.",
Review comment:
Also missing translations:
``` python
from flask_babel import lazy_gettext as _
"test_slack_channel": StringField(
_("Test Slack Channel"),
default=None,
description=_("A slack channel to send test message to."),
...
....
##########
File path: superset/tasks/schedules.py
##########
@@ -62,55 +63,94 @@
WEBDRIVER_BASEURL = config["WEBDRIVER_BASEURL"]
WEBDRIVER_BASEURL_USER_FRIENDLY = config["WEBDRIVER_BASEURL_USER_FRIENDLY"]
-EmailContent = namedtuple("EmailContent", ["body", "data", "images"])
+ReportContent = namedtuple(
+ "EmailContent",
+ [
+ "body", # email body
+ "data", # attachments
+ "images", # embedded images for the email
+ "slack_message", # html not supported, only markdown
+ # attachments for the slack message, embedding not supported
+ "slack_attachment",
+ ],
+)
-def _get_recipients(
- schedule: Union[DashboardEmailSchedule, SliceEmailSchedule]
+def _get_email_to_and_bcc(
+ recipients: str, deliver_as_group: bool
) -> Iterator[Tuple[str, str]]:
bcc = config["EMAIL_REPORT_BCC_ADDRESS"]
- if schedule.deliver_as_group:
- to = schedule.recipients
+ if deliver_as_group:
+ to = recipients
yield (to, bcc)
else:
- for to in get_email_address_list(schedule.recipients):
+ for to in get_email_address_list(recipients):
yield (to, bcc)
-def _deliver_email(
- schedule: Union[DashboardEmailSchedule, SliceEmailSchedule],
+def _deliver_slack(slack_channel: str, subject: str, body: str, file: bytes,) -> None:
+ try:
+ client = WebClient(token=config["SLACK_API_TOKEN"], proxy=config["SLACK_PROXY"])
+ response = client.files_upload(
+ channels=slack_channel, file=file, initial_comment=body, title=subject
+ )
+ logger.info(f"Sent the report to the slack {slack_channel}")
+ assert response["file"], str(response) # the uploaded file
+ except SlackApiError as ex:
+ if ex.response["error"] == "ratelimited":
+ delay = int(ex.response.headers["Retry-After"])
+ logger.info(f"Rate limited. Retrying in {delay} seconds")
+ time.sleep(delay)
+ client.files_upload(
Review comment:
The second retry can fail again, also I wonder how will this behave on high concurrency. May be over optimisation but we could consider a simple loop count or a simple form of exponential backoff
##########
File path: superset/views/schedules.py
##########
@@ -83,6 +83,11 @@ class EmailScheduleView(
description="List of recipients to send test email to. "
"If empty, we send it to the original recipients",
),
+ "test_slack_channel": StringField(
+ "Test Slack Channel",
+ default=None,
+ description="A slack channel to send test message to.",
Review comment:
`description="A slack channel to send a test message to.",`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org