sadpandajoe commented on code in PR #39914:
URL: https://github.com/apache/superset/pull/39914#discussion_r3508035318
##########
superset/reports/notifications/slack.py:
##########
@@ -53,7 +53,11 @@
logger = logging.getLogger(__name__)
-# TODO: Deprecated: Remove this class in Superset 6.0.0
+# Deprecated: Slack v1 will be removed in the next major release. The Slack
+# `files.upload` endpoint was retired in 2025, so file-bearing sends already
+# fail at the API level; only text-only `chat_postMessage` sends still work
+# here. Recipients with the `channels:read` scope are auto-upgraded to
+# SlackV2 on first send via `update_report_schedule_slack_v2`.
Review Comment:
Good catch — fixed in e975e18aba. The v2 probe (`should_use_v2_api`) lists
both public and private channel types, so it requires `channels:read` *and*
`groups:read`; the comment now names both scopes to match the deprecation
message and the probe's log line.
##########
superset/reports/notifications/slackv2.py:
##########
@@ -77,7 +77,18 @@ def _get_inline_files(
return ("pdf", [self._content.pdf])
return (None, [])
- @backoff.on_exception(backoff.expo, SlackApiError, factor=10, base=2,
max_tries=5)
+ # Retry on NotificationUnprocessableException (the wrapper that send()
+ # raises for transient Slack failures: SlackApiError, connection errors,
+ # and the SlackClientError catch-all). Retrying on SlackApiError directly
+ # would never fire because the try/except below converts it before the
+ # decorator can see it. Mirrors the pattern in webhook.py.
+ @backoff.on_exception(
+ backoff.expo,
+ NotificationUnprocessableException,
+ factor=10,
+ base=2,
+ max_tries=5,
+ )
@statsd_gauge("reports.slack.send")
def send(self) -> None:
Review Comment:
The v2 `send()` isn't wrapped in `@backoff` — only the per-call
`_call_slack_api` helper is, so a mid-loop failure retries just that one
channel's call, not channels that already succeeded (more granular than the v1
path, whose whole `send()` retries). A full re-send only happens via the
pre-existing reports/Celery task retry, which applies to every notification
type and is unchanged by this PR; `files_upload_v2` has no idempotency key, so
exactly-once delivery isn't achievable here. Leaving as-is for this deprecation
PR.
##########
superset/utils/slack.py:
##########
@@ -224,21 +256,75 @@ def get_channels_with_search(
return channels
+_SCOPE_MISSING_ERROR_CODES = frozenset(
+ {"missing_scope", "not_allowed_token_type", "no_permission"}
+)
+
+
def should_use_v2_api() -> bool:
if not feature_flag_manager.is_feature_enabled("ALERT_REPORT_SLACK_V2"):
+ _emit_v1_flag_off_deprecation()
return False
try:
client = get_slack_client()
+ extra_params = {"types": _SLACK_CONVERSATION_TYPES}
team_id = get_team_id()
- client.conversations_list(**({"team_id": team_id} if team_id else {}))
+ if team_id:
+ extra_params["team_id"] = team_id
+ client.conversations_list(
+ limit=1,
+ exclude_archived=True,
+ **extra_params,
+ )
logger.info("Slack API v2 is available")
return True
- except SlackApiError:
- # use the v1 api but warn with a deprecation message
+ except SlackApiError as ex:
+ # Only the scope-missing branch is a v1-deprecation signal; other
+ # SlackApiError codes (invalid_auth, ratelimited, server errors, etc.)
+ # are unrelated probe failures and should not be reported as a missing
+ # scope. We still fall back to v1 in both cases so a transient probe
+ # failure doesn't break sends — operators get an actionable log either
+ # way.
+ # `response` is normally a SlackResponse whose payload lives in
`.data`,
+ # but the SDK (and our tests) can also hand back a plain dict. Read the
+ # error code in either shape so the scope-missing branch isn't missed.
+ response = getattr(ex, "response", None)
+ data = getattr(response, "data", None)
+ if not isinstance(data, dict):
+ data = response if isinstance(response, dict) else {}
+ error_code = data.get("error", "")
+ if error_code in _SCOPE_MISSING_ERROR_CODES:
+ # The DeprecationWarning fires once per process, but the actionable
+ # log line fires every send so operators see it in their report
logs.
+ _emit_v1_scope_missing_deprecation()
+ logger.warning(
+ "Slack bot is missing the `channels:read` and `groups:read` "
+ "scopes; falling back to the deprecated "
+ "v1 API. %s",
+ _SLACK_V1_DEPRECATION_MESSAGE,
+ )
+ else:
+ logger.warning(
+ "Slack v2 probe failed with error %r; falling back to the "
+ "deprecated v1 API for this send. Investigate the underlying "
Review Comment:
This is intentional and documented in the surrounding comment: a non-scope
probe error falls back to v1 so a transient failure doesn't hard-abort the
send, and it logs an actionable "investigate the underlying Slack API error"
message distinct from the missing-scope path. Rate-limit errors are already
retried by the SDK's `RateLimitErrorRetryHandler` before they'd surface here,
and raising instead of falling back would fail the send just the same. No
change.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
