villebro commented on a change in pull request #13436:
URL: https://github.com/apache/superset/pull/13436#discussion_r588745647
##########
File path: superset/reports/commands/execute.py
##########
@@ -172,9 +174,14 @@ def _get_screenshot(self) -> ScreenshotData:
)
image_url = self._get_url(user_friendly=True)
user = self._get_screenshot_user()
- image_data = screenshot.compute_and_cache(
- user=user, cache=thumbnail_cache, force=True,
- )
+ try:
+ image_data = screenshot.get_screenshot(user=user)
+ except SoftTimeLimitExceeded as ex:
+ raise ReportScheduleScreenshotTimeout()
+ except Exception as ex:
+ raise ReportScheduleScreenshotFailedError(
+ f"Failed taking a screenshot {str(ex)}"
+ )
Review comment:
I'm stealing codecov's comment here, but could we add a test for this
broad exception case, too?
##########
File path: superset/reports/commands/alert.py
##########
@@ -48,6 +51,19 @@ def __init__(self, report_schedule: ReportSchedule):
self._result: Optional[float] = None
def run(self) -> bool:
+ """
+ Executes an alert SQL query and validates it.
+ Will set the report_schedule.last_value or last_value_row_json
+ with the query result
+
+ :return: bool, if the alert triggered or not
+ :raises: AlertQueryError,
+ AlertQueryInvalidTypeError,
+ AlertQueryMultipleColumnsError,
+ AlertQueryMultipleRowsError,
+ AlertQueryTimeout,
+ AlertValidatorConfigError
Review comment:
nit: while there are many conventions out there, I believe this is the
one we mostly use in Superset:
```
:raises Exception1: reason1
:raises Exception2: reason2
```
##########
File path: superset/reports/commands/alert.py
##########
@@ -112,9 +128,12 @@ def _is_validator_operator(self) -> bool:
self._report_schedule.validator_type ==
ReportScheduleValidatorType.OPERATOR
)
- def validate(self) -> None:
+ def _execute_query(self) -> pd.DataFrame:
"""
- Validate the query result as a Pandas DataFrame
+ Executes the actual alert SQL query template
+
+ :return: A pandas dataframe
+ :raises: AlertQueryTimeout, AlertQueryError
Review comment:
same here
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
