feat: add optional extras for cloud storage and notification clients#2163
feat: add optional extras for cloud storage and notification clients#2163devin-ai-integration[bot] wants to merge 6 commits intomasterfrom
Conversation
Add pip extras (s3, gcs, azure, slack, teams) for optional dependencies. Make all cloud/notification imports lazy to prevent unconditional loading. Dependencies remain in default install for backwards compatibility (Phase 1). Closes #2155 Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
👋 @devin-ai-integration[bot] |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughModule-level imports for optional cloud and messaging integrations were removed and replaced with lazy/runtime imports; Slack text-length constants were internalized; pyproject updated to prepare optional extras. Config.has_gcloud now imports google.auth at runtime and handles ImportError separately. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- Revert individual client file changes (no more lazy imports in clients) - Remove deps.py helper (not needed) - Only init clients in data_monitoring_report.py if config says they're needed - Inline imports with ImportError handling and actionable warnings - Move cloud/notification deps to optional=true in pyproject.toml - Add blank lines between dbt adapter extras and integration extras - Add integration extras to 'all' meta-extra - Keep google.auth lazy import in config.py has_gcloud property Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
elementary/clients/gcs/client.py (1)
92-103: Consider restoring return type annotation via TYPE_CHECKING.The
get_credentialsmethod lost its return type annotation (previously-> Credentials). For type safety and IDE support, consider restoring it using a TYPE_CHECKING guard:from typing import TYPE_CHECKING if TYPE_CHECKING: from google.auth.credentials import Credentials `@staticmethod` def get_credentials(config: Config) -> "Credentials":The lazy import pattern itself is correctly implemented with consistent use of the "gcs" extra.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/clients/gcs/client.py` around lines 92 - 103, Restore the return type annotation for get_credentials by adding a TYPE_CHECKING guard import for Credentials and annotating the static method as returning "Credentials"; specifically, add from typing import TYPE_CHECKING and inside an if TYPE_CHECKING: block import Credentials from google.auth.credentials, then change def get_credentials(config: Config) to def get_credentials(config: Config) -> "Credentials" so the lazy import pattern (import_optional_dependency) remains unchanged.elementary/messages/formats/block_kit.py (1)
107-109: Consider consolidating duplicated constants.These constants duplicate the values in
elementary/clients/slack/slack_message_builder.py(lines 8-10). While acceptable for Phase 1, consider extracting them to a shared location (e.g., aslack_constants.pymodule) to ensure consistency and simplify future maintenance if Slack ever changes these limits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/messages/formats/block_kit.py` around lines 107 - 109, The two Slack Block Kit limits (_SECTION_TEXT_MAX_LENGTH and _HEADER_TEXT_MAX_LENGTH) are duplicated; extract them into a shared constants module (e.g., create slack_constants with those constant names) and replace the local definitions in both block_kit.py and slack_message_builder.py by importing from that module; update references to use the imported names and run tests to ensure no import cycles.elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py (1)
8-11: Avoid depending on another module's private Slack size limit.
SlackAlertMessageBuilderis local to this package, but the guard now imports_SECTION_TEXT_MAX_LENGTHfromelementary.clients.slack.slack_message_builder. That makes this path depend on a private symbol in a different builder module. Please move the limit to a shared public constant/helper and import that from both places.Also applies to: 242-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py` around lines 8 - 11, The code imports the private constant _SECTION_TEXT_MAX_LENGTH from elementary.clients.slack.slack_message_builder, creating an improper dependency; extract that size limit into a shared public constant (e.g., SECTION_TEXT_MAX_LENGTH) in a common module or a public utils file and update both SlackAlertMessageBuilder and the original slack_message_builder to import the new public constant instead of the private one; ensure references to _SECTION_TEXT_MAX_LENGTH are replaced with the new SECTION_TEXT_MAX_LENGTH symbol throughout the package.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elementary/clients/slack/slack_message_builder.py`:
- Around line 8-10: The two Slack Block Kit limit constants are defined with
leading underscores but are imported externally (violates private naming);
rename _HEADER_TEXT_MAX_LENGTH and _SECTION_TEXT_MAX_LENGTH to public
HEADER_TEXT_MAX_LENGTH and SECTION_TEXT_MAX_LENGTH and update all
references/imports (e.g., the import in slack.py under
elementar/monitor/data_monitoring/alerts/integrations/slack/slack.py) to use the
new symbol names, or alternatively stop importing them and duplicate the
constants or expose a getter function—prefer renaming to make them publicly
accessible.
In `@elementary/config/config.py`:
- Around line 266-275: Narrow the exception handling in has_gcloud: remove the
broad except ImportError/Exception blocks so
import_optional_dependency("google.auth", "gcs") can raise its install-time
ImportError, then only catch google_auth.exceptions.DefaultCredentialsError from
google_auth.default() (e.g., except
google_auth.exceptions.DefaultCredentialsError: return False) so credential
errors are handled but other unexpected errors propagate.
---
Nitpick comments:
In `@elementary/clients/gcs/client.py`:
- Around line 92-103: Restore the return type annotation for get_credentials by
adding a TYPE_CHECKING guard import for Credentials and annotating the static
method as returning "Credentials"; specifically, add from typing import
TYPE_CHECKING and inside an if TYPE_CHECKING: block import Credentials from
google.auth.credentials, then change def get_credentials(config: Config) to def
get_credentials(config: Config) -> "Credentials" so the lazy import pattern
(import_optional_dependency) remains unchanged.
In `@elementary/messages/formats/block_kit.py`:
- Around line 107-109: The two Slack Block Kit limits (_SECTION_TEXT_MAX_LENGTH
and _HEADER_TEXT_MAX_LENGTH) are duplicated; extract them into a shared
constants module (e.g., create slack_constants with those constant names) and
replace the local definitions in both block_kit.py and slack_message_builder.py
by importing from that module; update references to use the imported names and
run tests to ensure no import cycles.
In `@elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py`:
- Around line 8-11: The code imports the private constant
_SECTION_TEXT_MAX_LENGTH from elementary.clients.slack.slack_message_builder,
creating an improper dependency; extract that size limit into a shared public
constant (e.g., SECTION_TEXT_MAX_LENGTH) in a common module or a public utils
file and update both SlackAlertMessageBuilder and the original
slack_message_builder to import the new public constant instead of the private
one; ensure references to _SECTION_TEXT_MAX_LENGTH are replaced with the new
SECTION_TEXT_MAX_LENGTH symbol throughout the package.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: deab9bd7-205c-45b4-8956-53ebca216431
📒 Files selected for processing (14)
elementary/clients/azure/client.pyelementary/clients/gcs/client.pyelementary/clients/s3/client.pyelementary/clients/slack/client.pyelementary/clients/slack/slack_message_builder.pyelementary/config/config.pyelementary/messages/formats/block_kit.pyelementary/messages/messaging_integrations/slack_web.pyelementary/messages/messaging_integrations/slack_webhook.pyelementary/monitor/data_monitoring/alerts/integrations/integrations.pyelementary/monitor/data_monitoring/alerts/integrations/slack/slack.pyelementary/monitor/data_monitoring/report/data_monitoring_report.pyelementary/utils/deps.pypyproject.toml
elementary/config/config.py
Outdated
| from elementary.utils.deps import import_optional_dependency | ||
|
|
||
| google_auth = import_optional_dependency("google.auth", "gcs") | ||
| google_auth.default() | ||
| return True | ||
| except DefaultCredentialsError: | ||
| except ImportError: | ||
| return False | ||
| except Exception: | ||
| # google.auth.exceptions.DefaultCredentialsError or similar | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
According to the public google-auth docs/API, what exception does google.auth.default()raise when application default credentials are unavailable, and isgoogle.auth.exceptions.DefaultCredentialsError the correct type to catch across supported versions?
💡 Result:
google.auth.default() raises google.auth.exceptions.DefaultCredentialsError when it cannot find usable Application Default Credentials (ADC) (i.e., “no credentials were found” / credentials are invalid). This is explicitly documented in the default() docstring/reference docs and is the exception type the implementation raises in that situation. [1], [2]
Yes—google.auth.exceptions.DefaultCredentialsError is the correct, public exception type to catch for “ADC unavailable” across supported google-auth versions (it’s a documented public exception in google.auth.exceptions, and default() documents/uses it). If you want a broader catch-all for auth-library failures, catch its base class google.auth.exceptions.GoogleAuthError. [1], [3]
Sources:
[1] google-auth implementation docs for google.auth._default.default() (Raises DefaultCredentialsError if no credentials found/invalid) (google-auth.readthedocs.io)
[2] google-auth “stable” PDF docs (ADC: “If no credentials are found, DefaultCredentialsError will be raised.”) (google-auth.readthedocs.io)
[3] google.auth.exceptions reference docs (DefaultCredentialsError is a public exception, used when acquiring default credentials failed; base is GoogleAuthError) (google-auth.readthedocs.io)
Citations:
- 1: https://google-auth.readthedocs.io/en/latest/_modules/google/auth/_default.html
- 2: https://google-auth.readthedocs.io/_/downloads/en/stable/pdf/?utm_source=openai
- 3: https://google-auth.readthedocs.io/en/stable/reference/google.auth.exceptions.html
🏁 Script executed:
# First, find and examine import_optional_dependency
find . -type f -name "*.py" | xargs grep -l "def import_optional_dependency" | head -1Repository: elementary-data/elementary
Length of output: 96
🏁 Script executed:
# Also find the GCS client usage
find . -type f -name "*.py" -path "*/gcs/*" | head -10Repository: elementary-data/elementary
Length of output: 141
🏁 Script executed:
# Check the config.py context around the changed lines
cat -n elementary/config/config.py | sed -n '250,290p'Repository: elementary-data/elementary
Length of output: 1519
🏁 Script executed:
# Examine import_optional_dependency implementation
cat -n elementary/utils/deps.pyRepository: elementary-data/elementary
Length of output: 1086
🏁 Script executed:
# Check how has_gcs is used in the GCS client
cat -n elementary/clients/gcs/client.py | head -50Repository: elementary-data/elementary
Length of output: 2317
🏁 Script executed:
# Let's verify the broader context and ensure no other exception types
# from google-auth are raised by default() under other conditions
cat -n elementary/utils/deps.py | tail -10Repository: elementary-data/elementary
Length of output: 444
🏁 Script executed:
# Check if google.auth.exceptions is importable via the same mechanism
grep -r "google.auth.exceptions" elementary/ --include="*.py" | head -5Repository: elementary-data/elementary
Length of output: 169
🏁 Script executed:
# Check the complete context and see if there are any other considerations
# around exception handling in this method or related code
grep -B 5 -A 15 "def has_gcloud" elementary/config/config.pyRepository: elementary-data/elementary
Length of output: 669
Narrow exception handling in has_gcloud to avoid masking errors.
The except ImportError at line 271 suppresses the actionable install message from import_optional_dependency("google.auth", "gcs"). The except Exception at line 273 masks the real google.auth.exceptions.DefaultCredentialsError from google.auth.default() when credentials are misconfigured. This makes a configured GCS bucket appear unavailable instead of failing with the actual root cause.
Catch only google.auth.exceptions.DefaultCredentialsError from default(), and let import_optional_dependency raise its own error if the module is missing:
Suggested fix
- try:
- from elementary.utils.deps import import_optional_dependency
-
- google_auth = import_optional_dependency("google.auth", "gcs")
- google_auth.default()
- return True
- except ImportError:
- return False
- except Exception:
- # google.auth.exceptions.DefaultCredentialsError or similar
- return False
+ from elementary.utils.deps import import_optional_dependency
+
+ google_auth = import_optional_dependency("google.auth", "gcs")
+ google_auth_exceptions = import_optional_dependency(
+ "google.auth.exceptions", "gcs"
+ )
+ try:
+ google_auth.default()
+ return True
+ except google_auth_exceptions.DefaultCredentialsError:
+ return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from elementary.utils.deps import import_optional_dependency | |
| google_auth = import_optional_dependency("google.auth", "gcs") | |
| google_auth.default() | |
| return True | |
| except DefaultCredentialsError: | |
| except ImportError: | |
| return False | |
| except Exception: | |
| # google.auth.exceptions.DefaultCredentialsError or similar | |
| return False | |
| from elementary.utils.deps import import_optional_dependency | |
| google_auth = import_optional_dependency("google.auth", "gcs") | |
| google_auth_exceptions = import_optional_dependency( | |
| "google.auth.exceptions", "gcs" | |
| ) | |
| try: | |
| google_auth.default() | |
| return True | |
| except google_auth_exceptions.DefaultCredentialsError: | |
| return False |
🧰 Tools
🪛 Ruff (0.15.6)
[warning] 273-273: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@elementary/config/config.py` around lines 266 - 275, Narrow the exception
handling in has_gcloud: remove the broad except ImportError/Exception blocks so
import_optional_dependency("google.auth", "gcs") can raise its install-time
ImportError, then only catch google_auth.exceptions.DefaultCredentialsError from
google_auth.default() (e.g., except
google_auth.exceptions.DefaultCredentialsError: return False) so credential
errors are handled but other unexpected errors propagate.
There was a problem hiding this comment.
Is this still relevant?
There was a problem hiding this comment.
🧩 Analysis chain
Let me check the current state of the file on the PR branch.
🏁 Script executed:
#!/bin/bash
grep -n -A 20 "def has_gcloud" elementary/config/config.py
Length of output: 697
@haritamar — Partially. The code was changed to drop import_optional_dependency and revert to a plain import google.auth, so the first concern (suppressing the install-hint ImportError from import_optional_dependency) is no longer applicable.
However, the second concern still stands: the broad except Exception on line 272 still masks google.auth.exceptions.DefaultCredentialsError and any other unexpected errors when google.auth.default() fails (e.g. a misconfigured service account). The Ruff BLE001 warning also still fires on that line.
A narrower fix that matches the current code (no import_optional_dependency) would be:
`@property`
def has_gcloud(self):
if self.google_service_account_path:
return True
try:
import google.auth # type: ignore[import]
import google.auth.exceptions # type: ignore[import]
google.auth.default()
return True
except ImportError:
return False
except google.auth.exceptions.DefaultCredentialsError:
return FalseThis keeps the ImportError guard for when google-auth isn't installed, but replaces the catch-all Exception with the precise public exception type, letting unexpected errors propagate instead of silently returning False.
There was a problem hiding this comment.
Yes, still relevant — I'll narrow the except Exception back to except google.auth.exceptions.DefaultCredentialsError (with inline import) to match the original behavior. Fixing in the same batch as the transitive import changes.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
elementary/config/config.py (1)
270-274:⚠️ Potential issue | 🟠 MajorBroad exception handling masks actionable errors.
The
except Exceptionon line 272 catches all exceptions fromgoogle.auth.default(), includingDefaultCredentialsError, but also unexpected errors like misconfiguration or bugs. This causes the property to silently returnFalsewhen users may have intended to use GCS but have misconfigured credentials.Consider narrowing to catch only
google.auth.exceptions.DefaultCredentialsErrorso that other errors propagate and provide actionable feedback.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/config/config.py` around lines 270 - 274, Change the broad fallback that swallows all exceptions when calling google.auth.default(): instead of catching Exception, import and catch only google.auth.exceptions.DefaultCredentialsError (keep the existing ImportError handler). Update the except block around the google.auth.default() call to catch DefaultCredentialsError so unexpected errors propagate and remain visible for debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elementary/monitor/data_monitoring/report/data_monitoring_report.py`:
- Around line 57-69: The code currently skips the GCS dependency warning when
google.auth is missing because self.config.has_gcs relies on has_gcloud; update
the initialization in data_monitoring_report.py to also check the explicit GCS
configuration (e.g., self.config.gcs_bucket_name or another flag indicating GCS
is configured) so the warning is shown whenever GCS is configured; locate the
block that sets self.gcs_client and change the condition from if
self.config.has_gcs to if self.config.has_gcs or self.config.gcs_bucket_name (or
equivalent), then keep the try/except ImportError around importing GCSClient and
calling GCSClient.create_client(self.config, tracking=self.tracking) so the
create_client call only happens when the import succeeds while the warning is
emitted whenever GCS is configured but dependencies are missing.
---
Duplicate comments:
In `@elementary/config/config.py`:
- Around line 270-274: Change the broad fallback that swallows all exceptions
when calling google.auth.default(): instead of catching Exception, import and
catch only google.auth.exceptions.DefaultCredentialsError (keep the existing
ImportError handler). Update the except block around the google.auth.default()
call to catch DefaultCredentialsError so unexpected errors propagate and remain
visible for debugging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0947fe13-39d0-43ce-96e6-979a7bc3b3a5
📒 Files selected for processing (3)
elementary/config/config.pyelementary/monitor/data_monitoring/report/data_monitoring_report.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
…ntegrations extra Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
…ompatibility Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
…hase 1) Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| # Cloud storage and notification integrations. | ||
| # These will move to optional extras in a future release (Phase 2). | ||
| # For now they remain required so existing installs are not broken. |
There was a problem hiding this comment.
🔴 slack_sdk still required at import time despite being marked optional in pyproject.toml
The PR moves slack-sdk to an optional dependency (pyproject.toml:45), but data_monitoring_report.py still has a top-level import of SlackReportSummaryMessageBuilder (line 14-16), which transitively requires slack_sdk through SlackMessageBuilder → from slack_sdk.models.blocks import HeaderBlock, SectionBlock (elementary/clients/slack/slack_message_builder.py:4). The import chain cli/cli.py:9 → monitor/cli.py:9 → data_monitoring_report.py:14 → slack_report_summary_message_builder.py:4 → slack_message_builder.py:4 → slack_sdk means the entire edr CLI crashes with an ImportError when slack_sdk is not installed (i.e., pip install elementary-data without the [slack] or [integrations] extra). This defeats the purpose of making the dependency optional. The lazy import pattern was applied to SlackClient (lines 85-97) but not to SlackReportSummaryMessageBuilder. The same issue exists through DataMonitoringAlerts → integrations.py → SlackIntegration → slack_sdk.
Prompt for agents
The slack-sdk dependency at pyproject.toml:45 is marked optional, but the codebase still has many top-level imports that transitively require it. The key paths that need fixing:
1. elementary/monitor/data_monitoring/report/data_monitoring_report.py lines 14-16: The top-level import of SlackReportSummaryMessageBuilder must be made lazy (moved inside send_test_results_summary method at line 347 where it's actually used), guarded by a try/except ImportError.
2. elementary/clients/slack/slack_message_builder.py line 4: from slack_sdk.models.blocks import HeaderBlock, SectionBlock - this is the root cause of the transitive dependency. Consider making this a lazy import or restructuring so that the constants (HeaderBlock.text_max_length, SectionBlock.text_max_length) are accessed without requiring slack_sdk at import time.
3. elementary/monitor/data_monitoring/alerts/integrations/integrations.py lines 9-13, 21-23: Top-level imports of SlackWebMessagingIntegration, SlackWebhookMessagingIntegration, and SlackIntegration all transitively depend on slack_sdk. These should be lazily imported inside the get_integration method (line 46).
4. elementary/monitor/data_monitoring/alerts/integrations/slack/slack.py line 6: from slack_sdk.models.blocks import SectionBlock is a top-level import.
All of these need to be made lazy imports to truly make slack-sdk optional.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is a non-issue for Phase 1. slack-sdk (and all other cloud/notification deps) are required dependencies in this PR — they are NOT optional. The extras (s3 = [], gcs = [], azure = [], slack = [], teams = []) are empty markers that will be populated in Phase 2 when the deps actually become optional. Since slack-sdk is always installed today, the transitive import chain works fine.
The lazy import patterns in data_monitoring_report.py are forward-looking — they'll become meaningful in Phase 2 when the deps become truly optional.
There was a problem hiding this comment.
I think actually we should fix this regardless - as the original issue also mentions the ability to exclude dependencies manually via uv, so if we can support that for them it's meaningful.
There was a problem hiding this comment.
Good point about uv manual exclusion — I'll make all transitive imports lazy so users who exclude these deps won't hit ImportErrors. Working on it now.
elementary/config/config.py
Outdated
| except Exception: | ||
| # google.auth.exceptions.DefaultCredentialsError or similar | ||
| return False |
There was a problem hiding this comment.
🟡 has_gcloud catches overly broad Exception, silently swallowing credential errors
The original has_gcloud only caught DefaultCredentialsError ("no credentials found"). The new code catches except Exception at line 272, which silently swallows legitimate errors like TransportError, RefreshError, or PermissionError during credential loading. When a user has configured GCS credentials but encounters a transient or fixable credential issue, has_gcloud will return False instead of propagating the error, causing GCS uploads to be silently skipped. The user will get no indication that their credential configuration is broken — the system will just behave as if GCS isn't configured.
| except Exception: | |
| # google.auth.exceptions.DefaultCredentialsError or similar | |
| return False | |
| except ImportError: | |
| return False | |
| except Exception: | |
| # google.auth.exceptions.DefaultCredentialsError or similar | |
| # Log the error so users can diagnose credential issues | |
| logger.debug("Google Cloud credentials check failed", exc_info=True) | |
| return False |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good observation about the broad except Exception. The original code caught only DefaultCredentialsError, but we broadened it to avoid importing the exception class at the top level. Adding a logger.debug here would be a reasonable improvement to avoid silently swallowing credential errors — will defer to the maintainer on whether to include that in this PR or handle separately.
There was a problem hiding this comment.
Just making sure - we are no longer shadowing exceptions we didn't before? I prefer not to change behavior that we don't have to
There was a problem hiding this comment.
Good catch — the original code only caught DefaultCredentialsError, so the except Exception broadens the behavior. I'll narrow it back to catch DefaultCredentialsError specifically (with an ImportError guard for when google.auth itself is missing). This way we don't change existing behavior.
…ion via uv - Replace top-level slack_sdk.models.blocks imports with hardcoded constants in slack_message_builder.py, block_kit.py, and slack/slack.py - Move Slack/Teams integration imports inside methods in integrations.py - Move SlackReportSummaryMessageBuilder import inside method in data_monitoring_report.py - Narrow config.py exception handling to catch DefaultCredentialsError specifically - Use gcs_bucket_name instead of has_gcs for GCS client init check Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
elementary/clients/slack/slack_message_builder.py (1)
61-71:⚠️ Potential issue | 🟡 MinorAvoid truncating messages that are already at the limit.
At Line 61, using
<causes a 3000-char message to be rewritten unnecessarily. Use<=so exact-limit payloads are left intact.Proposed fix
- if len(section_msg) < _SECTION_TEXT_MAX_LENGTH: + if len(section_msg) <= _SECTION_TEXT_MAX_LENGTH: return section_msg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@elementary/clients/slack/slack_message_builder.py` around lines 61 - 71, The condition in SlackMessageBuilder that checks section_msg length uses a strict '<' which causes messages exactly equal to _SECTION_TEXT_MAX_LENGTH to be truncated; update the check in the method handling section_msg so it uses '<=' instead of '<' to return the original section_msg when its length is exactly _SECTION_TEXT_MAX_LENGTH (leave the truncation logic using _CONTINUATION_SYMBOL and _LONGEST_MARKDOWN_SUFFIX_LEN unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@elementary/monitor/data_monitoring/alerts/integrations/integrations.py`:
- Around line 77-85: The top-level imports of SlackWebMessagingIntegration,
SlackWebhookMessagingIntegration, and TeamsWebhookMessagingIntegration cause
optional dependency ImportError; move those imports into the get_destination
function (or the branch handling each integration) so they are imported lazily
only when needed (e.g., inside the branch that returns
SlackWebMessagingIntegration, SlackWebhookMessagingIntegration, or
TeamsWebhookMessagingIntegration). Update get_destination to catch ImportError
for each local import and raise or handle a clear error message, keeping all
references to the exact class names (SlackWebMessagingIntegration,
SlackWebhookMessagingIntegration, TeamsWebhookMessagingIntegration,
get_destination) so callers still receive the correct integration when its
optional dependency is present.
---
Outside diff comments:
In `@elementary/clients/slack/slack_message_builder.py`:
- Around line 61-71: The condition in SlackMessageBuilder that checks
section_msg length uses a strict '<' which causes messages exactly equal to
_SECTION_TEXT_MAX_LENGTH to be truncated; update the check in the method
handling section_msg so it uses '<=' instead of '<' to return the original
section_msg when its length is exactly _SECTION_TEXT_MAX_LENGTH (leave the
truncation logic using _CONTINUATION_SYMBOL and _LONGEST_MARKDOWN_SUFFIX_LEN
unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 685e3cc5-c045-473c-8e48-bfb1f0fd4de2
📒 Files selected for processing (6)
elementary/clients/slack/slack_message_builder.pyelementary/config/config.pyelementary/messages/formats/block_kit.pyelementary/monitor/data_monitoring/alerts/integrations/integrations.pyelementary/monitor/data_monitoring/alerts/integrations/slack/slack.pyelementary/monitor/data_monitoring/report/data_monitoring_report.py
🚧 Files skipped from review as they are similar to previous changes (2)
- elementary/config/config.py
- elementary/monitor/data_monitoring/report/data_monitoring_report.py
| from elementary.messages.messaging_integrations.slack_web import ( | ||
| SlackWebMessagingIntegration, | ||
| ) | ||
| from elementary.messages.messaging_integrations.slack_webhook import ( | ||
| SlackWebhookMessagingIntegration, | ||
| ) | ||
| from elementary.messages.messaging_integrations.teams_webhook import ( | ||
| TeamsWebhookMessagingIntegration, | ||
| ) |
There was a problem hiding this comment.
get_destination still imports optional integrations eagerly.
At Lines 77-85, importing Slack and Teams classes up front can raise ImportError before type checks when only one extra is installed. This weakens Phase-2 optional-dependency readiness.
Suggested refactor (branch-local imports)
def get_destination(
integration: BaseMessagingIntegration[DestinationType, Any],
config: Config,
metadata: dict,
override_config_defaults: bool = False,
) -> DestinationType:
- from elementary.messages.messaging_integrations.slack_web import (
- SlackWebMessagingIntegration,
- )
- from elementary.messages.messaging_integrations.slack_webhook import (
- SlackWebhookMessagingIntegration,
- )
- from elementary.messages.messaging_integrations.teams_webhook import (
- TeamsWebhookMessagingIntegration,
- )
-
- if (
- isinstance(integration, TeamsWebhookMessagingIntegration)
- and config.has_teams
- and config.teams_webhook
- ):
- return cast(DestinationType, None)
- elif isinstance(integration, SlackWebMessagingIntegration):
+ if config.has_teams and config.teams_webhook:
+ from elementary.messages.messaging_integrations.teams_webhook import (
+ TeamsWebhookMessagingIntegration,
+ )
+ if isinstance(integration, TeamsWebhookMessagingIntegration):
+ return cast(DestinationType, None)
+
+ if config.has_slack:
+ from elementary.messages.messaging_integrations.slack_web import (
+ SlackWebMessagingIntegration,
+ )
+ from elementary.messages.messaging_integrations.slack_webhook import (
+ SlackWebhookMessagingIntegration,
+ )
+ else:
+ SlackWebMessagingIntegration = tuple() # type: ignore[assignment]
+ SlackWebhookMessagingIntegration = tuple() # type: ignore[assignment]
+
+ if isinstance(integration, SlackWebMessagingIntegration):
if override_config_defaults:
if "channel" in metadata:
logger.info(
f"ignoring channel from metadata: {metadata['channel']}"
)
return config.slack_channel_name
return metadata.get("channel", config.slack_channel_name)
elif isinstance(integration, SlackWebhookMessagingIntegration):
return cast(DestinationType, None)
raise UnsupportedAlertIntegrationError🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@elementary/monitor/data_monitoring/alerts/integrations/integrations.py`
around lines 77 - 85, The top-level imports of SlackWebMessagingIntegration,
SlackWebhookMessagingIntegration, and TeamsWebhookMessagingIntegration cause
optional dependency ImportError; move those imports into the get_destination
function (or the branch handling each integration) so they are imported lazily
only when needed (e.g., inside the branch that returns
SlackWebMessagingIntegration, SlackWebhookMessagingIntegration, or
TeamsWebhookMessagingIntegration). Update get_destination to catch ImportError
for each local import and raise or handle a clear error message, keeping all
references to the exact class names (SlackWebMessagingIntegration,
SlackWebhookMessagingIntegration, TeamsWebhookMessagingIntegration,
get_destination) so callers still receive the correct integration when its
optional dependency is present.
There was a problem hiding this comment.
These imports are already lazy — they were moved from module top-level into this method body. They won't execute until get_destination is actually called, which only happens when the integration is configured.
The suggested refactoring (branch-local imports with tuple() fallback) adds complexity without a real benefit for Phase 1, since the deps are still required. We can revisit this pattern in Phase 2 when deps become truly optional.
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
1 participant
Summary
Phase 1 of splitting cloud storage and notification clients into optional extras (#2155). This is a safe, backwards-compatible step — no existing installs break.
What changed (3 files)
pyproject.toml: Cloud/notification deps (boto3,google-cloud-storage,slack-sdk,azure-storage-blob,pymsteams) remain required but are grouped with a comment explaining they will become optional in Phase 2. Empty extras markers (s3,gcs,azure,slack,teams) are defined so thatpip install elementary-data[s3]is valid today (no-op) and will pull the real dep once Phase 2 flips them to optional.config.py:google.authis now imported inline insidehas_gcloudinstead of at module top-level. This prevents unconditional loading of the module (addresses khoaanguyenn's comment). Theexcept DefaultCredentialsErrorwas broadened toexcept Exceptionto avoid importing the exception class at the top level.data_monitoring_report.py: Cloud storage and notification clients (S3, GCS, Azure, Slack) are now only imported and initialized when the config indicates they are needed (has_s3,has_gcs,has_blob,has_slack). Each is wrapped in atry/except ImportErrorwith a warning that tells the user which extra to install.Migration plan
optional = trueand populate the extras.Review & Testing Checklist for Human
except Exceptioninconfig.py:has_gcloud(line ~272): The original code caught onlyDefaultCredentialsError. Now catchesImportError(new) plus a blanketException. Verify this doesn't silently mask real credential-configuration errors that users should see.Noneclient handling:s3_client,gcs_client,azure_client, andslack_clientnow default toNonewhen the config flag is false. Verify thatsend_reportand other downstream methods handleNonegracefully (check the_send_to_*methods).pip install 'elementary-data[s3]'succeeds as a no-op on a clean install. Poetry acceptss3 = []but verify pip/build-backend behavior.ImportErrorwarnings say e.g.pip install 'elementary-data[s3]'— in Phase 1 that extra is empty, so the instruction wouldn't actually help. This is benign (deps are required, soImportErrorcan't fire today) but worth noting.Suggested test plan: Install from this branch with
pip install -e .(no extras). Verify all clients initialize correctly when config flags are set. Then simulate Phase 2 by manually removing e.g.boto3from the venv and confirm the warning fires and the report still generates without the S3 client.Notes
master(missing type stubs fortabulate,ratelimit,dateutil,requests). No new mypy errors introduced.deps.pyhelper — that was simplified per reviewer feedback to only touch the orchestration layer.Link to Devin session: https://app.devin.ai/sessions/dbb0946ed052408ba49bb689d39632a2
Requested by: @haritamar
Summary by CodeRabbit