sadpandajoe commented on code in PR #39914:
URL: https://github.com/apache/superset/pull/39914#discussion_r3508330291
##########
superset/reports/notifications/slackv2.py:
##########
@@ -48,6 +48,55 @@
logger = logging.getLogger(__name__)
+_TRANSIENT_SLACK_API_ERROR_CODES = frozenset(
+ {
+ "fatal_error",
+ "internal_error",
+ "ratelimited",
+ "request_timeout",
+ "rollup_error",
+ "service_unavailable",
+ "timeout",
+ }
+)
Review Comment:
Skipping: `_TRANSIENT_SLACK_API_ERROR_CODES` is a module-level
`frozenset({...})` literal that mypy already infers as `frozenset[str]`, and it
is the only module-level constant in this file, so there is no
annotated-constant convention to match. mypy, ruff, and ruff-format all pass
as-is; adding an explicit annotation here isn't required and would diverge from
the surrounding unannotated style.
##########
superset/utils/slack.py:
##########
@@ -34,12 +36,42 @@
logger = logging.getLogger(__name__)
+_SLACK_V1_DEPRECATION_MESSAGE = (
+ "Slack v1 (the legacy `Slack` recipient type and `files.upload` API) is "
+ "deprecated and will be removed in the next major release. Slack retired "
+ "the `files.upload` endpoint in 2025, so v1 file uploads no longer work; "
+ "only text-only `chat_postMessage` sends still succeed. Grant your Slack "
+ "bot the `channels:read` and `groups:read` scopes so existing v1 "
+ "recipients can be auto-upgraded to SlackV2 on "
+ "their next send."
+)
Review Comment:
`_SLACK_V1_DEPRECATION_MESSAGE` is a module-level string literal whose type
mypy already infers as `str`, and this module deliberately leaves all similar
constants unannotated (`_SLACK_CONVERSATION_TYPES`,
`_SCOPE_MISSING_ERROR_CODES`, and even `logger`). Adding an explicit `: str`
only here would diverge from the file's convention; mypy and ruff already pass,
so no change.
##########
superset/utils/slack.py:
##########
@@ -34,12 +36,42 @@
logger = logging.getLogger(__name__)
+_SLACK_V1_DEPRECATION_MESSAGE = (
+ "Slack v1 (the legacy `Slack` recipient type and `files.upload` API) is "
+ "deprecated and will be removed in the next major release. Slack retired "
+ "the `files.upload` endpoint in 2025, so v1 file uploads no longer work; "
+ "only text-only `chat_postMessage` sends still succeed. Grant your Slack "
+ "bot the `channels:read` and `groups:read` scopes so existing v1 "
+ "recipients can be auto-upgraded to SlackV2 on "
+ "their next send."
+)
+
+
+# functools.cache gives us a process-lifetime, thread-safe one-shot guard
+# without the read-then-write race that bare module globals would have under
+# multi-threaded WSGI workers. The cached return value (None) is irrelevant —
+# we only care that the body executes at most once per process.
[email protected]
+def _emit_v1_flag_off_deprecation() -> None:
+ warnings.warn(_SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning,
stacklevel=3)
+ logger.warning(
+ "ALERT_REPORT_SLACK_V2 is disabled; %s", _SLACK_V1_DEPRECATION_MESSAGE
+ )
+
+
[email protected]
+def _emit_v1_scope_missing_deprecation() -> None:
+ warnings.warn(_SLACK_V1_DEPRECATION_MESSAGE, DeprecationWarning,
stacklevel=3)
+
class SlackChannelTypes(StrEnum):
PUBLIC = "public_channel"
PRIVATE = "private_channel"
+_SLACK_CONVERSATION_TYPES = ",".join(SlackChannelTypes)
Review Comment:
`_SLACK_CONVERSATION_TYPES = ",".join(SlackChannelTypes)` is a derived
module-level constant whose type mypy already infers as `str`, matching how the
other constants in this file (`_SLACK_V1_DEPRECATION_MESSAGE`,
`_SCOPE_MISSING_ERROR_CODES`) are left unannotated. Annotating just this one
would break local consistency; mypy/ruff pass, so no change.
##########
superset/utils/slack.py:
##########
@@ -117,7 +149,7 @@ def _get_channels(
client = get_slack_client()
channel_schema = SlackChannelSchema()
channels: list[SlackChannelSchema] = []
- extra_params = {"types": ",".join(SlackChannelTypes)}
+ extra_params = {"types": _SLACK_CONVERSATION_TYPES}
Review Comment:
`extra_params` in `_get_channels` is a local dict; mypy infers `dict[str,
str]` (both `_SLACK_CONVERSATION_TYPES` and the later `team_id` assignment on
line 154 are strings) and type-checks the `**extra_params` call cleanly. A
local-variable annotation isn't required and none of the sibling locals carry
one, so skipping.
--
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]
