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 10 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}
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
|
Can we make the CI install the s3 and slack extras? Even though they do nothing now |
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
|
@haritamar Done — CI now installs with |
|
Thanks! I see that we pass gcs and azure params as well so maybe these too (though I see gcs is commented out actually for some reason) |
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
|
@haritamar Good call — updated to install all five extras: |
|
Why teams actually? I don't see that we pass teams params in the workflow. Did I miss something? |
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
|
@haritamar You're right, no Teams params in the workflow. Removed it — now just |
|
And maybe the teams extra should be named msteams |
Co-Authored-By: Itamar Hartstein <haritamar@gmail.com>
|
@haritamar Good idea — renamed |
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
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,msteams) 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, preventing unconditional loading (addresses khoaanguyenn's comment).ImportErroris caught separately (returnsFalsewhengoogle.authis missing), and the originalDefaultCredentialsErrorcatch is preserved.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. Each is wrapped in atry/except ImportErrorwith a warning that tells the user which extra to install.SlackReportSummaryMessageBuilderis also lazily imported insidesend_test_results_summary.slack_message_builder.py/block_kit.py/slack/slack.py: Replaced top-levelfrom slack_sdk.models.blocks import HeaderBlock, SectionBlockwith hardcoded constants (_HEADER_TEXT_MAX_LENGTH = 150,_SECTION_TEXT_MAX_LENGTH = 3000). This breaks the transitive import chain so modules can be imported withoutslack_sdkinstalled.integrations.py: MovedSlackWebMessagingIntegration,SlackWebhookMessagingIntegration,TeamsWebhookMessagingIntegration, andSlackIntegrationimports from module top-level into the methods that use them (get_integration,get_destination).test-warehouse.yml: CI now installs with.[s3,gcs,azure,slack]extras to verify they resolve without errors.Migration plan
optional = trueand populate the extras.Review & Testing Checklist for Human
_SECTION_TEXT_MAX_LENGTH = 3000and_HEADER_TEXT_MAX_LENGTH = 150are duplicated inslack_message_builder.py,block_kit.py, andslack/slack.py. Verify these matchslack_sdk.models.blocks.SectionBlock.text_max_length/HeaderBlock.text_max_length. If slack_sdk ever changes them, these will drift silently.get_destinationimports are unconditional: All three integration types are imported at the top ofget_destination()regardless of which integration is configured. In Phase 2 (when deps become optional), callingget_destinationwithoutslack_sdkwould fail even for a Teams-only setup. This is fine for Phase 1 but should be addressed before Phase 3.gcs_bucket_nameinstead ofhas_gcs: The GCS block indata_monitoring_report.pychecksself.config.gcs_bucket_namerather thanself.config.has_gcs. This is intentional —has_gcsdepends onhas_gcloudwhich swallowsImportError, so the warning would never fire. VerifyGCSClient.create_clientstill returnsNonegracefully when credentials are missing but the bucket name is set.Noneclient handling:s3_client,gcs_client,azure_client, andslack_clientdefault toNonewhen the config flag is false or import fails. Verify thatsend_reportand_send_to_*methods handleNonegracefully.pip install 'elementary-data[msteams]'and the other extras succeed as no-ops on a clean install.Suggested test plan: Install from this branch with
pip install -e .(no extras) and verify all clients initialize when config flags are set. Then simulate Phase 2 bypip uninstall boto3and confirm the S3 warning fires and the report still generates without the S3 client.Notes
teamsextra was renamed tomsteamsper reviewer feedback.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