New custom slack sink, slack_sink_preview#1789
Conversation
WalkthroughThis update adds a "preview" mode for Slack notifications in Robusta, enabling advanced message templating with Jinja2 and Slack Block Kit JSON. It introduces new classes, configuration options, template loading, and rendering logic, along with manual and automated tests to verify functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Robusta
participant SlackSink
participant SlackSinkPreview
participant SlackSender
participant Slack API
User->>Robusta: Configure sink (slack_sink or slack_sink_preview)
Robusta->>SlackSink: Initialize (is_preview=False)
Robusta->>SlackSinkPreview: Initialize (is_preview=True)
SlackSinkPreview->>SlackSender: Initialize with is_preview=True
SlackSink->>SlackSender: Initialize with is_preview=False
User->>Robusta: Trigger alert (Finding)
Robusta->>SlackSink/SlackSinkPreview: Send finding
alt Preview Mode
SlackSinkPreview->>SlackSender: send_finding_to_slack (is_preview=True)
SlackSender->>template_loader: Render template (default or custom)
template_loader-->>SlackSender: Slack blocks
SlackSender->>Slack API: Post message (with blocks and attachments)
else Standard Mode
SlackSink->>SlackSender: send_finding_to_slack (is_preview=False)
SlackSender->>Slack API: Post message (legacy formatting)
end
Slack API-->>User: Slack message delivered
sequenceDiagram
participant User
participant Robusta
participant SlackSender
participant template_loader
User->>Robusta: Define custom template in slack_sink_preview config
Robusta->>SlackSender: Send finding with custom template
SlackSender->>template_loader: render_custom_template_to_blocks(custom_template, context)
template_loader-->>SlackSender: Slack blocks (parsed from JSON)
SlackSender->>Slack API: Post message with custom blocks
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/robusta/core/sinks/slack/templates/README.md (1)
1-73: Comprehensive documentation for new templating systemThe README provides excellent documentation for the Slack message templates. It clearly explains how templates work, available options, customization methods, and template variables. The example configuration is particularly helpful for users wanting to customize their templates.
Minor suggestion: In line 9, consider adding "the" before "Slack Block Kit" for improved grammar.
- Templates are separated by double newlines (`\n\n`) to indicate separate blocks. Each block must be valid JSON that conforms to the [Slack Block Kit](https://api.slack.com/block-kit) format. + Templates are separated by double newlines (`\n\n`) to indicate separate blocks. Each block must be valid JSON that conforms to the [Slack Block Kit](https://api.slack.com/block-kit) format.🧰 Tools
🪛 LanguageTool
[uncategorized] ~9-~9: Possible missing article found.
Context: ...ate separate blocks. Each block must be valid JSON that conforms to the [Slack Block ...(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~13-~13: Loose punctuation mark.
Context: .... ## Available Templates -header.j2: The header section of alert notificatio...(UNLIKELY_OPENING_PUNCTUATION)
docs/configuration/sinks/slack.rst (1)
184-258: Duplicate content with "Template Styles and Customization" section.The content in this "Message Templating" section appears to be duplicated in the following "Template Styles and Customization" section (lines 259-354). Consider consolidating these sections to avoid redundancy.
src/robusta/core/sinks/slack/templates/template_loader.py (1)
1-6: Remove unused imports.The
OptionalandTupletypes are imported but not used in the code.-from typing import Dict, Any, Optional, List, Tuple +from typing import Dict, Any, List🧰 Tools
🪛 Ruff (0.8.2)
4-4:
typing.Optionalimported but unusedRemove unused import
(F401)
4-4:
typing.Tupleimported but unusedRemove unused import
(F401)
test_slack_integration.py (2)
8-13: Remove unused standard-library & typing imports
json,List,Dict,Any, andOptionalare never referenced. Keeping dead imports hurts readability and may hide real linter warnings.-import json -from typing import List, Dict, Any, Optional +# (no additional imports needed here)🧰 Tools
🪛 Ruff (0.8.2)
11-11:
jsonimported but unusedRemove unused import:
json(F401)
12-12:
typing.Listimported but unusedRemove unused import
(F401)
12-12:
typing.Dictimported but unusedRemove unused import
(F401)
12-12:
typing.Anyimported but unusedRemove unused import
(F401)
12-12:
typing.Optionalimported but unusedRemove unused import
(F401)
18-37: Prune unused Robusta symbols to silence Ruff F401
FindingStatus,CallbackBlock,CallbackChoice,ResourceInfo, andask_holmesare imported but never used after you converted interactive blocks to plain Markdown.-from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingStatus, FindingSubject, Link, EnrichmentType +from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingSubject, Link, EnrichmentType ... -from robusta.core.reporting.blocks import ( - MarkdownBlock, - HeaderBlock, - DividerBlock, - TableBlock, - ListBlock, - LinkProp, - LinksBlock, - CallbackBlock, - CallbackChoice, - FileBlock -) +from robusta.core.reporting.blocks import ( + MarkdownBlock, + HeaderBlock, + DividerBlock, + TableBlock, + ListBlock, + LinkProp, + LinksBlock, + FileBlock, +) ... -from robusta.core.model.base_params import ResourceInfo -from robusta.core.playbooks.internal.ai_integration import ask_holmes +# removed unused importsThis keeps the test harness tidy and stops CI noise.
🧰 Tools
🪛 Ruff (0.8.2)
19-19:
robusta.core.reporting.base.FindingStatusimported but unusedRemove unused import:
robusta.core.reporting.base.FindingStatus(F401)
30-30:
robusta.core.reporting.blocks.CallbackBlockimported but unusedRemove unused import
(F401)
31-31:
robusta.core.reporting.blocks.CallbackChoiceimported but unusedRemove unused import
(F401)
34-34:
robusta.core.model.base_params.ResourceInfoimported but unusedRemove unused import:
robusta.core.model.base_params.ResourceInfo(F401)
36-36:
robusta.core.playbooks.internal.ai_integration.ask_holmesimported but unusedRemove unused import:
robusta.core.playbooks.internal.ai_integration.ask_holmes(F401)
src/robusta/integrations/slack/sender.py (2)
4-4: Drop unusedosimport
osis never referenced in this module – delete to satisfy Ruff F401.-import os🧰 Tools
🪛 Ruff (0.8.2)
4-4:
osimported but unusedRemove unused import:
os(F401)
512-520: Robustness: JSON block splitting by blank lines is fragile
template.render(...).split("\n\n")assumes block JSONs are separated by exactly one blank
line and that no JSON object itself contains two consecutive newlines (pretty-printed or
markdown). A safer approach is to wrap the template in a top-level list andjson.loads
once:- rendered_blocks = [] - for block_str in template.render(**template_context).strip().split("\n\n"): - if block_str.strip(): - block = json.loads(block_str) - rendered_blocks.append(block) - return rendered_blocks + blocks_json = "[" + template.render(**template_context) + "]" + return json.loads(blocks_json)This prevents silent failures when a template author formats JSON differently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/configuration/sinks/slack.rst(1 hunks)src/robusta/core/reporting/base.py(2 hunks)src/robusta/core/sinks/slack/slack_sink_params.py(3 hunks)src/robusta/core/sinks/slack/templates/README.md(1 hunks)src/robusta/core/sinks/slack/templates/__init__.py(1 hunks)src/robusta/core/sinks/slack/templates/header.j2(1 hunks)src/robusta/core/sinks/slack/templates/legacy.j2(1 hunks)src/robusta/core/sinks/slack/templates/template_loader.py(1 hunks)src/robusta/integrations/slack/sender.py(9 hunks)test_slack_integration.py(1 hunks)
🧰 Additional context used
🪛 LanguageTool
src/robusta/core/sinks/slack/templates/README.md
[uncategorized] ~9-~9: Possible missing article found.
Context: ...ate separate blocks. Each block must be valid JSON that conforms to the [Slack Block ...
(AI_HYDRA_LEO_MISSING_A)
[uncategorized] ~13-~13: Loose punctuation mark.
Context: .... ## Available Templates - header.j2: The header section of alert notificatio...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Ruff (0.8.2)
src/robusta/core/sinks/slack/templates/template_loader.py
4-4: typing.Optional imported but unused
Remove unused import
(F401)
4-4: typing.Tuple imported but unused
Remove unused import
(F401)
test_slack_integration.py
11-11: json imported but unused
Remove unused import: json
(F401)
12-12: typing.List imported but unused
Remove unused import
(F401)
12-12: typing.Dict imported but unused
Remove unused import
(F401)
12-12: typing.Any imported but unused
Remove unused import
(F401)
12-12: typing.Optional imported but unused
Remove unused import
(F401)
19-19: robusta.core.reporting.base.FindingStatus imported but unused
Remove unused import: robusta.core.reporting.base.FindingStatus
(F401)
30-30: robusta.core.reporting.blocks.CallbackBlock imported but unused
Remove unused import
(F401)
31-31: robusta.core.reporting.blocks.CallbackChoice imported but unused
Remove unused import
(F401)
34-34: robusta.core.model.base_params.ResourceInfo imported but unused
Remove unused import: robusta.core.model.base_params.ResourceInfo
(F401)
36-36: robusta.core.playbooks.internal.ai_integration.ask_holmes imported but unused
Remove unused import: robusta.core.playbooks.internal.ai_integration.ask_holmes
(F401)
src/robusta/integrations/slack/sender.py
4-4: os imported but unused
Remove unused import: os
(F401)
751-754: Use ternary operator kwargs = {"thread_ts": thread_ts} if thread_ts else {} instead of if-else-block
Replace if-else-block with kwargs = {"thread_ts": thread_ts} if thread_ts else {}
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (13)
src/robusta/core/sinks/slack/templates/__init__.py (1)
1-2: Package initialization looks goodA simple Python package marker with a clear comment describing its purpose. This follows the standard Python package structure and effectively establishes the new Slack templates package.
src/robusta/core/sinks/slack/slack_sink_params.py (2)
5-5: Import additions look goodAppropriate imports added to support the new template configuration options.
15-20: Good addition of template configuration optionsThe new parameters align well with the PR objectives to introduce Jinja2 templates and more flexible Slack message formatting. The options are well-documented with clear defaults and appropriate types:
send_svgandprefer_redirect_to_platformcontrol visual aspectstemplate_styleprovides an easy way to toggle between modern and legacy formatscustom_templatesenables complete customization for advanced usersThis implementation successfully supports both backward compatibility (via the "legacy" option) and new flexible formatting.
src/robusta/core/sinks/slack/templates/header.j2 (1)
1-36: Well-structured template implementing the new JIRA-style headerThis template successfully implements the more information-dense header style mentioned in the PR objectives. Key strengths:
- Clear organization with separate title and context blocks
- Intelligent conditional rendering for the investigation link
- Comprehensive metadata display with appropriate icons
- Proper use of Slack's Block Kit JSON structure
The template achieves the goal of providing users with richer details at a glance while maintaining a clean visual structure. The conditional logic for the optional resource section is particularly well implemented.
src/robusta/core/reporting/base.py (2)
153-159: Good serialization implementation.The
to_dictmethod provides a clean dictionary representation of the Enrichment class with all relevant attributes. This will make it easy to use enrichment data in templates.
416-442: Well-structured JSON serialization for Finding class.The comprehensive
to_jsonmethod correctly transforms the Finding object into a serializable dictionary, including all nested objects like enrichments and links. This serialization will be valuable for the new Slack templating system.src/robusta/core/sinks/slack/templates/legacy.j2 (1)
1-31: Well-organized legacy template with clear sectioning.The template effectively replicates the original Slack message format with appropriate sections for status/header, source, and description. The conditional logic based on finding source and status is cleanly implemented.
docs/configuration/sinks/slack.rst (1)
259-354: Comprehensive documentation for template customization.The template documentation provides clear examples and a well-structured reference of available variables. This will be valuable for users who want to customize their Slack notifications.
src/robusta/core/sinks/slack/templates/template_loader.py (4)
17-26: Good initialization with template caching.The template environment is properly configured with appropriate settings for file loading, autoescape, and whitespace handling. The template caching will improve performance by avoiding repeated disk reads.
28-46: Robust template loading with error handling.The
get_templatemethod appropriately checks the cache before loading from disk and includes solid error handling with a fallback template. This will prevent template loading failures from bringing down the entire application.
48-78: Well-implemented template rendering with proper error handling.The
render_to_blocksmethod correctly handles the rendering and parsing of template output, with appropriate error handling for both rendering and JSON parsing failures. The approach of splitting by newlines to separate multiple blocks is elegant.
81-82: Good use of singleton pattern.Creating a singleton instance of the template loader ensures efficient reuse across the application and avoids duplicating the template cache.
src/robusta/integrations/slack/sender.py (1)
760-770: Attachment created with only a divider when there are no enrichment blocksIf
attachment_slack_blocksis empty we still append a divider, sending an attachment that
contains a single unusable block. Slack allows it, but users see an odd horizontal line.Add the divider only when at least one attachment block exists:
-all_attachment_blocks = attachment_slack_blocks.copy() if attachment_slack_blocks else [] -# Always add a divider at the end -all_attachment_blocks.append({"type": "divider"}) +all_attachment_blocks = attachment_slack_blocks.copy() +if all_attachment_blocks: + all_attachment_blocks.append({"type": "divider"})
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/robusta/integrations/slack/sender.py (2)
431-527: Well-structured Jinja2 template integration for flexible Slack messagesThe refactoring of
__create_finding_headerto use Jinja2 templates is well-implemented. It provides:
- Template selection based on user preference
- Support for custom templates
- Comprehensive context data for templates
- Good error handling with fallback to file-based templates
The structure of preparing context data separately from rendering is clean and maintainable.
Consider moving the emoji mapping logic to a separate helper function for better maintainability:
- # Choose emoji based on kind - if subject_kind.lower() == "pod": - resource_emoji = ":ship:" - elif subject_kind.lower() == "deployment": - resource_emoji = ":package:" - elif subject_kind.lower() == "node": - resource_emoji = ":computer:" - elif subject_kind.lower() == "service": - resource_emoji = ":link:" - elif subject_kind.lower() == "job": - resource_emoji = ":clock1:" - elif subject_kind.lower() == "statefulset": - resource_emoji = ":chains:" + resource_emoji = self._get_resource_emoji(subject_kind.lower()) # Then add this helper method to the class: def _get_resource_emoji(self, kind: str) -> str: emoji_map = { "pod": ":ship:", "deployment": ":package:", "node": ":computer:", "service": ":link:", "job": ":clock1:", "statefulset": ":chains:" } return emoji_map.get(kind, ":package:") # Default emoji
724-804: Thorough refactoring of message sending logicThe code now properly handles:
- Header blocks, main blocks, and attachment blocks separately
- File blocks and their conversion
- Error message integration from file uploads
- Detailed logging for debugging
- A consistent attachment structure with proper coloring
The refactoring significantly improves message organization and readability in Slack.
- if thread_ts: - kwargs = {"thread_ts": thread_ts} - else: - kwargs = {} + kwargs = {"thread_ts": thread_ts} if thread_ts else {}🧰 Tools
🪛 Ruff (0.8.2)
751-754: Use ternary operator
kwargs = {"thread_ts": thread_ts} if thread_ts else {}instead ofif-else-blockReplace
if-else-block withkwargs = {"thread_ts": thread_ts} if thread_ts else {}(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/robusta/integrations/slack/sender.py(9 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/robusta/integrations/slack/sender.py
4-4: os imported but unused
Remove unused import: os
(F401)
751-754: Use ternary operator kwargs = {"thread_ts": thread_ts} if thread_ts else {} instead of if-else-block
Replace if-else-block with kwargs = {"thread_ts": thread_ts} if thread_ts else {}
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (6)
src/robusta/integrations/slack/sender.py (6)
660-661: Great improvement: New header blocks systemThe introduction of dedicated
header_blocksallows for more structured and visually appealing messages in Slack.Also applies to: 679-682
684-688: Good UX improvement: Description moved after titleMoving the description immediately after the title creates a more logical reading flow for users.
698-719: Well-organized file block handlingSeparating file blocks from other blocks and handling them appropriately improves the message structure.
772-781: Helpful debugging: Comprehensive logging before sendingThe detailed logging of all blocks before sending to Slack will be invaluable for debugging template issues.
843-843: Consistent terminology: "Source" changed to "Cluster"Renaming from "Source" to "Cluster" maintains consistency with the terminology used in templates.
Also applies to: 851-851, 866-866
347-347: LGTM: Helpful comment about attachment creationThe comment clarifies the purpose of creating a single attachment with consistent coloring.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/robusta/integrations/slack/sender.py (1)
639-792: 🛠️ Refactor suggestionRefactor duplicated message composition logic
The
send_finding_to_slackmethod has been extensively reworked to support the new template-based header system, but contains duplicated logic from__send_blocks_to_slack. Consider refactoring to eliminate this duplication.Additionally, the ternary operator could be used for kwargs assignment as suggested by the static analyzer.
# Replace lines 739-742 with a ternary operator - if thread_ts: - kwargs = {"thread_ts": thread_ts} - else: - kwargs = {} + kwargs = {"thread_ts": thread_ts} if thread_ts else {}More broadly, consider extracting the block processing logic into a helper method that both
send_finding_to_slackand__send_blocks_to_slackcan use, eliminating the duplication noted in the comment on line 710.🧰 Tools
🪛 Ruff (0.8.2)
739-742: Use ternary operator
kwargs = {"thread_ts": thread_ts} if thread_ts else {}instead ofif-else-blockReplace
if-else-block withkwargs = {"thread_ts": thread_ts} if thread_ts else {}(SIM108)
♻️ Duplicate comments (1)
src/robusta/integrations/slack/sender.py (1)
2-4: Remove unused importThe
osmodule is imported but not used anywhere in the file.import copy import json -import os import logging import ssl🧰 Tools
🪛 Ruff (0.8.2)
4-4:
osimported but unusedRemove unused import:
os(F401)
🧹 Nitpick comments (6)
src/robusta/core/sinks/slack/templates/template_loader.py (3)
13-19: Consider making the regex pattern a module-level constantThe regular expression for finding string literals is well-crafted for handling newlines in JSON, but it's recompiled every time the function is called.
+# Regex for matching JSON string literals, handling escaped quotes and multiline strings +STRING_LITERAL_RE = re.compile(r'("(?:\\.|[^"\\])*")', flags=re.DOTALL) def escape_raw_newlines_in_json_strings(raw_json: str) -> str: def fix_string(match): s = match.group(1) return s.replace("\n", "\\n") # Escape raw newlines in strings - STRING_LITERAL_RE = re.compile(r'("(?:\\.|[^"\\])*")', flags=re.DOTALL) return STRING_LITERAL_RE.sub(fix_string, raw_json)
75-76: Remove duplicate line initializationThere's a duplicate initialization of the
blockslist.# Split by newlines to get multiple blocks and parse each as JSON blocks = [] - blocks = [] for block_str in rendered.strip().split("\n\n"):
95-121: Consider refactoring to reduce code duplicationThe
render_custom_or_file_template_to_blocksmethod duplicates logic found inrender_to_blocksfor processing JSON blocks. Consider refactoring to extract common functionality.def render_custom_or_file_template_to_blocks(self, template_name: str, context: Dict[str, Any], custom_template: str = None) -> List[Dict[str, Any]]: """ Render a custom Jinja template string (if provided) or a file-based template to Slack blocks. Args: template_name: The name of the file-based template (e.g., "header.j2") context: Dictionary of variables to pass to the template custom_template: Optional Jinja template string to use instead of file-based template Returns: List of Slack block objects (dictionaries) """ if custom_template: try: template = Template(custom_template) - rendered_blocks = [] - for block_str in template.render(**context).strip().split("\n\n"): - if block_str.strip(): - block_str_fixed = escape_raw_newlines_in_json_strings(block_str) - block = json.loads(block_str_fixed) - rendered_blocks.append(block) - return rendered_blocks + rendered = template.render(**context) + return self._process_rendered_template(rendered) except Exception as e: logging.error(f"Error rendering custom template: {e}") # Fall back to file-based template # Use file-based template return self.render_to_blocks(template_name, context) + def _process_rendered_template(self, rendered: str) -> List[Dict[str, Any]]: + """ + Process a rendered template string into Slack blocks. + + Args: + rendered: The rendered template string + + Returns: + List of Slack block objects (dictionaries) + """ + blocks = [] + for block_str in rendered.strip().split("\n\n"): + if not block_str.strip(): + continue + + try: + block_str_fixed = escape_raw_newlines_in_json_strings(block_str) + block = json.loads(block_str_fixed) + blocks.append(block) + + except json.JSONDecodeError as e: + logging.exception(f"Error parsing JSON from template output: {e}") + logging.warning(f"Problematic JSON (repr): {repr(block_str)}") + + return blocksThis refactoring would also require updating
render_to_blocksto use the new helper method.src/robusta/integrations/slack/sender.py (3)
19-21: Unused import and comment clarityThe
Templateclass is imported but never used directly in this file. Also, the comment could be more specific about why Jinja2 is "always present".-# Since we're assuming Jinja2 is always present, we can import directly -from jinja2 import Template +# Import template loader for Slack message templating from robusta.core.sinks.slack.templates.template_loader import template_loader🧰 Tools
🪛 Ruff (0.8.2)
20-20:
jinja2.Templateimported but unusedRemove unused import:
jinja2.Template(F401)
431-514: Well-structured template-based header generationThe header generation logic is now template-driven with good support for multiple styles and custom templates. The context preparation is thorough, providing rich data for the templates.
A few suggestions to improve the code:
- Extract the resource text/emoji logic to a separate method
- Consider making the emoji mappings a dictionary lookup
def __create_finding_header( self, finding: Finding, status: FindingStatus, platform_enabled: bool, include_investigate_link: bool, sink_params = None ) -> List[SlackBlock]: title = finding.title.removeprefix("[RESOLVED] ") title, mention = self.extract_mentions(title) sev = finding.severity # Select appropriate template based on user preference template_name = "header.j2" # default template if sink_params and hasattr(sink_params, "template_style") and sink_params.template_style == "legacy": template_name = "legacy.j2" # Check if the user has provided a custom template custom_template = None if sink_params and sink_params.custom_templates and template_name in sink_params.custom_templates: custom_template = sink_params.custom_templates[template_name] # Prepare data for template status_text = "Firing" if status == FindingStatus.FIRING else "Resolved" status_emoji = "⚠️" if status == FindingStatus.FIRING else "✅" investigate_uri = finding.get_investigate_uri(self.account_id, self.cluster_name) if platform_enabled else "" # Get alert type information if finding.source == FindingSource.PROMETHEUS: alert_type = "Alert" elif finding.source == FindingSource.KUBERNETES_API_SERVER: alert_type = "K8s Event" else: alert_type = "Notification" + # Get resource information + resource_text, resource_emoji = self.__get_resource_info(finding.subject) - # Prepare resource text and emoji if available - resource_text = "" - resource_emoji = ":package:" - - if finding.subject: - subject_kind = finding.subject.subject_type.value - subject_namespace = finding.subject.namespace - subject_name = finding.subject.name - - if subject_kind and subject_name: - # Choose emoji based on kind - if subject_kind.lower() == "pod": - resource_emoji = ":ship:" - elif subject_kind.lower() == "deployment": - resource_emoji = ":package:" - elif subject_kind.lower() == "node": - resource_emoji = ":computer:" - elif subject_kind.lower() == "service": - resource_emoji = ":link:" - elif subject_kind.lower() == "job": - resource_emoji = ":clock1:" - elif subject_kind.lower() == "statefulset": - resource_emoji = ":chains:" - - # Format as Kind/Namespace/Name - if subject_namespace: - resource_text = f"{subject_kind}/{subject_namespace}/{subject_name}" - else: - resource_text = f"{subject_kind}/{subject_name}" # Prepare template context template_context = { "title": title, "status_text": status_text, "status_emoji": status_emoji, "severity": sev.name.capitalize(), "severity_emoji": sev.to_emoji(), "alert_type": alert_type, "cluster_name": self.cluster_name, "platform_enabled": platform_enabled, "include_investigate_link": include_investigate_link, "investigate_uri": investigate_uri, "resource_text": resource_text, "resource_emoji": resource_emoji, "finding": finding } # Use the new template loader method for both custom and file-based templates return template_loader.render_custom_or_file_template_to_blocks( template_name, template_context, custom_template ) +def __get_resource_info(self, subject): + """Extract resource information from subject""" + resource_text = "" + resource_emoji = ":package:" # default emoji + + if not subject: + return resource_text, resource_emoji + + subject_kind = subject.subject_type.value + subject_namespace = subject.namespace + subject_name = subject.name + + if not (subject_kind and subject_name): + return resource_text, resource_emoji + + # Emoji mapping by resource kind + emoji_map = { + "pod": ":ship:", + "deployment": ":package:", + "node": ":computer:", + "service": ":link:", + "job": ":clock1:", + "statefulset": ":chains:" + } + + # Get emoji for resource kind + resource_emoji = emoji_map.get(subject_kind.lower(), ":package:") + + # Format as Kind/Namespace/Name + if subject_namespace: + resource_text = f"{subject_kind}/{subject_namespace}/{subject_name}" + else: + resource_text = f"{subject_kind}/{subject_name}" + + return resource_text, resource_emoji
760-769: Consider moving detailed logging to debug level onlyThe detailed JSON logging of all blocks is useful for debugging but could generate a lot of log volume in production. Consider adding a condition to only log at this level when a debug flag is enabled.
# Detailed logging of blocks before sending to Slack - logging.debug( + if logging.getLogger().isEnabledFor(logging.DEBUG): + logging.debug( f"SENDING TO SLACK - FULL BLOCKS DETAIL:\n" f"CHANNEL: {slack_channel}\n" f"TITLE: {finding.title}\n" f"HEADER BLOCKS: {json.dumps(header_blocks, indent=2)}\n" f"MAIN BLOCKS: {json.dumps(output_blocks, indent=2)}\n" f"ALL BLOCKS: {json.dumps(all_blocks, indent=2)}\n" f"ATTACHMENT BLOCKS: {json.dumps(all_attachment_blocks, indent=2)}\n" f"ATTACHMENTS: {json.dumps(attachments, indent=2)}\n" - ) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/robusta/core/sinks/slack/templates/template_loader.py(1 hunks)src/robusta/integrations/slack/sender.py(9 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/robusta/integrations/slack/sender.py
4-4: os imported but unused
Remove unused import: os
(F401)
20-20: jinja2.Template imported but unused
Remove unused import: jinja2.Template
(F401)
739-742: Use ternary operator kwargs = {"thread_ts": thread_ts} if thread_ts else {} instead of if-else-block
Replace if-else-block with kwargs = {"thread_ts": thread_ts} if thread_ts else {}
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (5)
src/robusta/core/sinks/slack/templates/template_loader.py (4)
1-11: Good use of type hints and modularity for the template systemThe imports and type hints are well-structured for this new module. Defining the template directory path relative to the current file is a good approach for reliability.
22-37: Good environment configuration with cachingThe
SlackTemplateLoaderclass is well-designed with appropriate configuration for Jinja2, enabling proper template loading and rendering. The template caching implementation will improve performance.
38-57: Solid error handling for template loadingThe
get_templatemethod has good error handling with appropriate logging and fallback for template loading failures.
123-124: Appropriate use of the singleton patternThe singleton pattern is well-applied here, providing a consistent instance for template loading throughout the application.
src/robusta/integrations/slack/sender.py (1)
831-832: Good terminology update from "Source" to "Cluster"Renaming the label from "Source:" to "Cluster:" improves clarity and aligns with the current terminology.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
src/robusta/integrations/slack/sender.py (1)
720-727:⚠️ Potential issueLost error feedback from
prepare_slack_textIf file uploads fail,
prepare_slack_textreturns anerror_msg, but
the new path drops it, so users never see the warning.message, error_msg = self.prepare_slack_text( finding.title, max_log_file_limit_kb=sink_params.max_log_file_limit_kb, files=file_blocks ) - +# Surface upload errors to the user +if error_msg: + other_blocks.append(MarkdownBlock(error_msg))🧰 Tools
🪛 Ruff (0.8.2)
721-721: Trailing comma missing
Add trailing comma
(COM812)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/robusta/core/sinks/slack/slack_sink_params.py(2 hunks)src/robusta/core/sinks/slack/templates/template_loader.py(1 hunks)src/robusta/integrations/slack/sender.py(9 hunks)tests/test_slack_integration.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_slack_integration.py
1-1: The file is executable but no shebang is present
(EXE002)
16-16: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
24-24: Trailing comma missing
Add trailing comma
(COM812)
25-25: Trailing comma missing
Add trailing comma
(COM812)
32-32: Trailing comma missing
Add trailing comma
(COM812)
33-33: Trailing comma missing
Add trailing comma
(COM812)
40-40: Trailing comma missing
Add trailing comma
(COM812)
41-41: Trailing comma missing
Add trailing comma
(COM812)
43-43: Missing return type annotation for public function test_slack_template_styles
Add return type annotation: None
(ANN201)
43-43: Missing type annotation for function argument template_style
(ANN001)
43-43: Missing type annotation for function argument template_name
(ANN001)
43-43: Missing type annotation for function argument expected_phrases
(ANN001)
45-45: Trailing comma missing
Add trailing comma
(COM812)
52-52: Trailing comma missing
Add trailing comma
(COM812)
74-74: Boolean positional value in function call
(FBT003)
src/robusta/core/sinks/slack/slack_sink_params.py
5-5: typing.Dict is deprecated, use dict instead
(UP035)
5-5: typing.Literal imported but unused
Remove unused import: typing.Literal
(F401)
22-22: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
src/robusta/core/sinks/slack/templates/template_loader.py
5-5: typing.Dict is deprecated, use dict instead
(UP035)
5-5: typing.List is deprecated, use list instead
(UP035)
14-14: Missing return type annotation for private function fix_string
(ANN202)
14-14: Missing type annotation for function argument match
(ANN001)
27-27: Missing return type annotation for special method __init__
Add return type annotation: None
(ANN204)
36-36: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
51-51: Do not catch blind exception: Exception
(BLE001)
52-52: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
52-52: Logging statement uses f-string
(G004)
58-58: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
58-58: Use list instead of List for type annotation
Replace with list
(UP006)
58-58: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
87-87: Logging statement uses f-string
(G004)
87-87: Redundant exception object included in logging.exception call
(TRY401)
88-88: Logging statement uses f-string
(G004)
88-88: Use explicit conversion flag
Replace with conversion flag
(RUF010)
90-90: Consider moving this statement to an else block
(TRY300)
91-91: Do not catch blind exception: Exception
(BLE001)
92-92: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
92-92: Logging statement uses f-string
(G004)
95-95: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
95-95: Use list instead of List for type annotation
Replace with list
(UP006)
95-95: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
107-107: Do not catch blind exception: Exception
(BLE001)
108-108: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
108-108: Logging statement uses f-string
(G004)
111-111: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
111-111: Use list instead of List for type annotation
Replace with list
(UP006)
111-111: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
src/robusta/integrations/slack/sender.py
4-4: os imported but unused
Remove unused import: os
(F401)
20-20: jinja2.Template imported but unused
Remove unused import: jinja2.Template
(F401)
432-432: Boolean-typed positional argument in function definition
(FBT001)
432-432: Boolean-typed positional argument in function definition
(FBT001)
433-433: Missing type annotation for function argument sink_params
(ANN001)
433-433: Trailing comma missing
Add trailing comma
(COM812)
434-434: Use list instead of List for type annotation
Replace with list
(UP006)
499-499: Trailing comma missing
Add trailing comma
(COM812)
511-511: Unnecessary else after return statement
Remove unnecessary else
(RET505)
646-646: Use list instead of List for type annotation
Replace with list
(UP006)
721-721: Trailing comma missing
Add trailing comma
(COM812)
737-740: Use ternary operator kwargs = {"thread_ts": thread_ts} if thread_ts else {} instead of if-else-block
Replace if-else-block with kwargs = {"thread_ts": thread_ts} if thread_ts else {}
(SIM108)
754-754: Trailing comma missing
Add trailing comma
(COM812)
759-766: Logging statement uses f-string
(G004)
766-766: Trailing comma missing
Add trailing comma
(COM812)
785-785: Do not catch blind exception: Exception
(BLE001)
786-788: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
787-787: Logging statement uses f-string
(G004)
787-787: Trailing comma missing
Add trailing comma
(COM812)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (2)
src/robusta/integrations/slack/sender.py (2)
432-513: Header rendering relies on buggy loader API
template_loader.render_custom_template_to_blocks/render_file_template_to_blocks
are currently broken (see template_loader.py comments). Once fixed,
__create_finding_headerwill work as intended. No changes required
here, but keep this dependency in mind when validating end-to-end.🧰 Tools
🪛 Ruff (0.8.2)
432-432: Boolean-typed positional argument in function definition
(FBT001)
432-432: Boolean-typed positional argument in function definition
(FBT001)
433-433: Missing type annotation for function argument
sink_params(ANN001)
433-433: Trailing comma missing
Add trailing comma
(COM812)
434-434: Use
listinstead ofListfor type annotationReplace with
list(UP006)
499-499: Trailing comma missing
Add trailing comma
(COM812)
511-511: Unnecessary
elseafterreturnstatementRemove unnecessary
else(RET505)
4-5: 🧹 Nitpick (assertive)Remove unused imports
Neither
osnorjinja2.Templateis referenced after refactor.-import os -from jinja2 import TemplateLikely an incorrect or invalid review comment.
🧰 Tools
🪛 Ruff (0.8.2)
4-4:
osimported but unusedRemove unused import:
os(F401)
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
src/robusta/core/sinks/slack/preview/__init__.py (1)
1-3: 🛠️ Refactor suggestionDefine
__all__to specify exported symbolsThe static analysis correctly flags that imported classes are not used within this file. Since this is an
__init__.pyfile that's meant to expose these classes at the package level, you should explicitly define which symbols are exported by adding an__all__declaration.from robusta.core.sinks.slack.preview.slack_sink_preview import SlackSinkPreview from robusta.core.sinks.slack.preview.slack_sink_params_preview import SlackSinkPreviewConfigWrapper, SlackSinkPreviewParams +__all__ = ["SlackSinkPreview", "SlackSinkPreviewConfigWrapper", "SlackSinkPreviewParams"]🧰 Tools
🪛 Ruff (0.8.2)
1-1:
robusta.core.sinks.slack.preview.slack_sink_preview.SlackSinkPreviewimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
robusta.core.sinks.slack.preview.slack_sink_params_preview.SlackSinkPreviewConfigWrapperimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
2-2:
robusta.core.sinks.slack.preview.slack_sink_params_preview.SlackSinkPreviewParamsimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
src/robusta/integrations/slack/sender.py (1)
264-301:⚠️ Potential issueMutable default argument & lost error surfacing
files: List[FileBlock] = []is a classic mutable-default gotcha – the same list is shared
across invocations.When
error_msgis returned it is no longer surfaced to the user (the caller no longer
appends a block).- def prepare_slack_text(self, message: str, max_log_file_limit_kb: int, files: List[FileBlock] = []): + def prepare_slack_text( + self, + message: str, + max_log_file_limit_kb: int, + files: Optional[List[FileBlock]] = None, + ): ... - if files: + if files:In
send_finding_to_slack, re-introduce the original behaviour:message, error_msg = self.prepare_slack_text(...) -if error_msg: - # previously lost – add it back - other_blocks.append(MarkdownBlock(error_msg)) +if error_msg: + other_blocks.append(MarkdownBlock(error_msg))🧰 Tools
🪛 Ruff (0.8.2)
264-264: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (5)
src/robusta/core/sinks/slack/preview/slack_sink_preview_params.py (1)
18-28: Edge-case: emptytemplate_namestring bypasses override
get_effective_template_name()checksif self.template_name:.
If the caller setstemplate_name=""(empty string), this evaluates toFalseand silently falls back to the style-based default, which is probably surprising.- if self.template_name: + if self.template_name is not None:A subsequent explicit empty string should raise/ log an error instead of being ignored.
Consider validating the field (e.g. with a Pydantic validator) to disallow an empty string.tests/manual_tests/test_slack_integration_manual.py (2)
6-8:sys.pathmanipulation risks masking the real packageAdding
<tests>/manual_tests/srcin front of the path can shadow the installedrobustapackage or other top-level modules, leading to confusing import behaviour.Prefer:
PROJECT_ROOT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "..")) sys.path.insert(0, PROJECT_ROOT)or – even better – rely on a proper virtual-env installation for manual tests.
9-29: Remove unused imports to keep the file lint-cleanThe following imports are flagged by Ruff as unused:
FindingStatus,CallbackBlock,CallbackChoice,ResourceInfo,ask_holmes.Deleting them (or prefixing with
_) will keep CI green.-from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingStatus, FindingSubject, Link, \ - EnrichmentType +from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingSubject, Link, \ + EnrichmentType ... -from robusta.core.reporting.blocks import ( - MarkdownBlock, - HeaderBlock, - DividerBlock, - TableBlock, - ListBlock, - LinkProp, - LinksBlock, - CallbackBlock, - CallbackChoice, - FileBlock -) +from robusta.core.reporting.blocks import ( + MarkdownBlock, + HeaderBlock, + DividerBlock, + TableBlock, + ListBlock, + LinkProp, + LinksBlock, + FileBlock +)🧰 Tools
🪛 Ruff (0.8.2)
10-10:
robusta.core.reporting.base.FindingStatusimported but unusedRemove unused import:
robusta.core.reporting.base.FindingStatus(F401)
22-22:
robusta.core.reporting.blocks.CallbackBlockimported but unusedRemove unused import
(F401)
23-23:
robusta.core.reporting.blocks.CallbackChoiceimported but unusedRemove unused import
(F401)
26-26:
robusta.core.model.base_params.ResourceInfoimported but unusedRemove unused import:
robusta.core.model.base_params.ResourceInfo(F401)
28-28:
robusta.core.playbooks.internal.ai_integration.ask_holmesimported but unusedRemove unused import:
robusta.core.playbooks.internal.ai_integration.ask_holmes(F401)
src/robusta/integrations/slack/sender.py (2)
2-5: Clean up unused imports
osandjinja2.Templateare no longer referenced in the file.-import json -import os -from jinja2 import Template +import jsonRemoving them eliminates Ruff F401 warnings and keeps the dependency set minimal.
🧰 Tools
🪛 Ruff (0.8.2)
4-4:
osimported but unusedRemove unused import:
os(F401)
668-676:to_slackreceivesBaseBlocks but header is pre-rendered
header_blocksis already a list of Slack-formatted dicts, whileoutput_blocks
expects theBaseBlockform. Mixing the two representations works here but is brittle.
Consider converting all blocks through a single pathway (e.g. make
__create_finding_headerreturnHeaderBlocks and reuse__to_slack) to ensure a
uniform contract and simplify testing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/robusta/core/sinks/sink_factory.py(2 hunks)src/robusta/core/sinks/slack/preview/__init__.py(1 hunks)src/robusta/core/sinks/slack/preview/slack_sink_preview.py(1 hunks)src/robusta/core/sinks/slack/preview/slack_sink_preview_params.py(1 hunks)src/robusta/core/sinks/slack/slack_sink.py(1 hunks)src/robusta/core/sinks/slack/templates/template_loader.py(1 hunks)src/robusta/integrations/slack/sender.py(11 hunks)tests/manual_tests/test_slack_integration_manual.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/robusta/core/sinks/slack/templates/template_loader.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/manual_tests/test_slack_integration_manual.py
10-10: robusta.core.reporting.base.FindingStatus imported but unused
Remove unused import: robusta.core.reporting.base.FindingStatus
(F401)
22-22: robusta.core.reporting.blocks.CallbackBlock imported but unused
Remove unused import
(F401)
23-23: robusta.core.reporting.blocks.CallbackChoice imported but unused
Remove unused import
(F401)
26-26: robusta.core.model.base_params.ResourceInfo imported but unused
Remove unused import: robusta.core.model.base_params.ResourceInfo
(F401)
28-28: robusta.core.playbooks.internal.ai_integration.ask_holmes imported but unused
Remove unused import: robusta.core.playbooks.internal.ai_integration.ask_holmes
(F401)
src/robusta/integrations/slack/sender.py
4-4: os imported but unused
Remove unused import: os
(F401)
20-20: jinja2.Template imported but unused
Remove unused import: jinja2.Template
(F401)
739-742: Use ternary operator kwargs = {"thread_ts": thread_ts} if thread_ts else {} instead of if-else-block
Replace if-else-block with kwargs = {"thread_ts": thread_ts} if thread_ts else {}
(SIM108)
src/robusta/core/sinks/slack/preview/__init__.py
1-1: robusta.core.sinks.slack.preview.slack_sink_preview.SlackSinkPreview imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: robusta.core.sinks.slack.preview.slack_sink_params_preview.SlackSinkPreviewConfigWrapper imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
2-2: robusta.core.sinks.slack.preview.slack_sink_params_preview.SlackSinkPreviewParams imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (3)
src/robusta/core/sinks/sink_factory.py (1)
25-25: LGTM!The SlackSinkPreview import and registration in the sink factory mapping looks good. This properly integrates the new preview sink into the factory's sink creation mechanism.
Also applies to: 39-39
src/robusta/core/sinks/slack/slack_sink.py (1)
13-22: LGTM!The changes to support dependency injection of a SlackSender are well-implemented. This pattern improves testability and allows for specialized behavior like preview mode.
src/robusta/integrations/slack/sender.py (1)
505-514: Runtime safety: relies onget_effective_template_nameexisting onsink_params
SlackSinkParamsis passed in, but onlySlackSinkPreviewParamsis guaranteed to
implementget_effective_template_name/get_custom_template.
IfSlackSinkParamswasn’t extended accordingly, this will raiseAttributeError
at runtime.Please verify that the base params were updated or guard the access:
if hasattr(sink_params, "get_effective_template_name"): template_name = sink_params.get_effective_template_name() else: template_name = "header.j2"
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/robusta/integrations/slack/sender.py (2)
826-829:⚠️ Potential issueHandle error message from prepare_slack_text.
Similar to the issue fixed in a previous commit, you need to handle the error message returned by
prepare_slack_text().- message, error_msg = self.prepare_slack_text( - finding.title, max_log_file_limit_kb=sink_params.max_log_file_limit_kb, files=file_blocks - ) + message, error_msg = self.prepare_slack_text( + finding.title, max_log_file_limit_kb=sink_params.max_log_file_limit_kb, files=file_blocks + ) + + # Surface possible file-upload errors to the user + if error_msg: + other_blocks.append(MarkdownBlock(error_msg))
852-861: 🛠️ Refactor suggestionAvoid sending an empty attachment with only a divider.
The code always adds a divider to
all_attachment_blockseven when there are no enrichment blocks, potentially resulting in an empty attachment with just a divider.- # Always add a divider at the end - all_attachment_blocks.append({"type": "divider"}) - - # Create a single attachment with the status color - attachments = [{ - "color": status.to_color_hex(), - "blocks": all_attachment_blocks - }] + # Only add attachments if we have blocks + if all_attachment_blocks: + # Add a divider at the end + all_attachment_blocks.append({"type": "divider"}) + + # Create a single attachment with the status color + attachments = [{ + "color": status.to_color_hex(), + "blocks": all_attachment_blocks + }] + else: + attachments = []
🧹 Nitpick comments (4)
src/robusta/integrations/slack/sender.py (4)
843-846: Use ternary operator for thread_ts handling.The static analysis tool suggests using a ternary operator for better readability.
- if thread_ts: - kwargs = {"thread_ts": thread_ts} - else: - kwargs = {} + kwargs = {"thread_ts": thread_ts} if thread_ts else {}🧰 Tools
🪛 Ruff (0.8.2)
843-846: Use ternary operator
kwargs = {"thread_ts": thread_ts} if thread_ts else {}instead ofif-else-blockReplace
if-else-block withkwargs = {"thread_ts": thread_ts} if thread_ts else {}(SIM108)
864-873: Consider conditionally enabling detailed JSON logging.The extensive JSON dumps in the debug logs could impact performance in production environments.
Consider adding a more granular log level check:
- logging.debug( - f"SENDING TO SLACK - FULL BLOCKS DETAIL:\n" - f"CHANNEL: {slack_channel}\n" - f"TITLE: {finding.title}\n" - f"HEADER BLOCKS: {json.dumps(header_blocks, indent=2)}\n" - f"MAIN BLOCKS: {json.dumps(output_blocks, indent=2)}\n" - f"ALL BLOCKS: {json.dumps(all_blocks, indent=2)}\n" - f"ATTACHMENT BLOCKS: {json.dumps(all_attachment_blocks, indent=2)}\n" - f"ATTACHMENTS: {json.dumps(attachments, indent=2)}\n" - ) + if logging.getLogger().isEnabledFor(logging.DEBUG): + # Only perform expensive JSON serialization if debug is enabled + logging.debug( + f"SENDING TO SLACK - FULL BLOCKS DETAIL:\n" + f"CHANNEL: {slack_channel}\n" + f"TITLE: {finding.title}\n" + f"HEADER BLOCKS: {json.dumps(header_blocks, indent=2)}\n" + f"MAIN BLOCKS: {json.dumps(output_blocks, indent=2)}\n" + f"ALL BLOCKS: {json.dumps(all_blocks, indent=2)}\n" + f"ATTACHMENT BLOCKS: {json.dumps(all_attachment_blocks, indent=2)}\n" + f"ATTACHMENTS: {json.dumps(attachments, indent=2)}\n" + )
462-473: Extract emoji mapping to a constant or configuration.The resource kind to emoji mapping is hardcoded in the method. Consider extracting it to a constant or configuration for better maintainability.
+# At the top of the file with other constants +RESOURCE_KIND_EMOJI_MAP = { + "pod": ":ship:", + "deployment": ":package:", + "node": ":computer:", + "service": ":link:", + "job": ":clock1:", + "statefulset": ":chains:", + # Add more mappings as needed +} + # Then in the method: - # Choose emoji based on kind - if subject_kind.lower() == "pod": - resource_emoji = ":ship:" - elif subject_kind.lower() == "deployment": - resource_emoji = ":package:" - elif subject_kind.lower() == "node": - resource_emoji = ":computer:" - elif subject_kind.lower() == "service": - resource_emoji = ":link:" - elif subject_kind.lower() == "job": - resource_emoji = ":clock1:" - elif subject_kind.lower() == "statefulset": - resource_emoji = ":chains:" + # Choose emoji based on kind + resource_emoji = RESOURCE_KIND_EMOJI_MAP.get(subject_kind.lower(), ":package:")
742-895: Consider refactoring to reduce code duplication.The
send_finding_to_slack_previewmethod duplicates a lot of logic fromsend_finding_to_slack. Consider extracting common functionality into helper methods.Common code between both methods could be refactored into shared helper functions for:
- Channel determination
- Holmes analysis handling
- Finding status determination
- Links creation
- Block transformation and sending
This would make both methods more maintainable and reduce the risk of inconsistent behavior.
🧰 Tools
🪛 Ruff (0.8.2)
843-846: Use ternary operator
kwargs = {"thread_ts": thread_ts} if thread_ts else {}instead ofif-else-blockReplace
if-else-block withkwargs = {"thread_ts": thread_ts} if thread_ts else {}(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/robusta/integrations/slack/sender.py(9 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/robusta/integrations/slack/sender.py
843-846: Use ternary operator kwargs = {"thread_ts": thread_ts} if thread_ts else {} instead of if-else-block
Replace if-else-block with kwargs = {"thread_ts": thread_ts} if thread_ts else {}
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (6)
src/robusta/integrations/slack/sender.py (6)
9-9: LGTM: Required imports for the new templating system.The new imports for
json, template loader, and preview parameters are necessary to support the new templating functionality.Also applies to: 16-16, 51-51
66-66: LGTM: Added preview mode flag to SlackSender.The
is_previewparameter and instance attribute enable the new templating system without breaking existing functionality.Also applies to: 87-87
427-509: LGTM: Well-structured header template generator.This method effectively extracts all relevant information from the finding and prepares it for template rendering. The implementation supports both built-in and custom templates, providing good flexibility for users.
665-672: LGTM: Clean integration of preview mode.The conditional delegation to the preview implementation is a clean way to introduce the new functionality without disrupting existing behavior.
934-934: LGTM: Updated terminology from "Source" to "Cluster".This change aligns with the terminology used in the new templates and documentation.
Also applies to: 942-942
891-895:Details
❌ Incorrect review comment
Handle exceptions more consistently in preview mode.
The error handling returns an empty string on exception, which might cause issues if the caller expects a valid timestamp.
🏁 Script executed:
#!/bin/bash # Check how the return value from send_finding_to_slack_preview is used # Look for usages of send_finding_to_slack echo "Checking usages of send_finding_to_slack:" rg "send_finding_to_slack\(" src/ # Look for checks on the return value echo "Checking for checks on the return value:" rg "= +.+send_finding_to_slack\(" src/Length of output: 567
🏁 Script executed:
#!/bin/bash # Display send_finding_to_slack_preview implementation and its exception handling sed -n '850,920p' src/robusta/integrations/slack/sender.pyLength of output: 3140
🏁 Script executed:
#!/bin/bash # Display context around send_finding_to_slack usage in SlackSink grep -Rn "send_finding_to_slack" -n src/robusta/core/sinks/slack/slack_sink.py | cut -d: -f1 | while read -r line; do start=$((line-5)) end=$((line+5)) echo "Context around line $line:" sed -n "${start},${end}p" src/robusta/core/sinks/slack/slack_sink.py echo "----" doneLength of output: 1606
🏁 Script executed:
#!/bin/bash # Show the full handle_notification_grouping method in SlackSink to inspect return value usage awk '/def handle_notification_grouping/,/def get_timeline_uri/' src/robusta/core/sinks/slack/slack_sink.pyLength of output: 3074
No action needed for send_finding_to_slack exception handling
Thesend_finding_to_slackmethod logs any exceptions and returns an empty string on failure, but we’ve confirmed that none of its callers inspect or rely on the returned timestamp. As such, there’s no current risk of downstream errors. If you later introduce a “preview” flow or other code paths that do depend on a valid Slack timestamp, you can revisit this exception‐handling strategy at that time.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_slack_preview.py (1)
1-2: Complete or clarify the file move for Slack template testsThe comment indicates this file is being moved to
tests/test_slack_templates.py, but that file doesn't exist yet. Either include the renamed file in this PR or update the PR description to clarify this is a multi-step migration.
🧹 Nitpick comments (3)
tests/test_slack_preview.py (3)
4-4: Remove unused importThe
pytestimport is not being used directly in the code. It's likely needed for the test runner but not explicitly referenced in this file.-import pytest🧰 Tools
🪛 Ruff (0.11.9)
4-4:
pytestimported but unusedRemove unused import:
pytest(F401)
16-66: Test structure looks solid, but assertions could be more robustThe test properly sets up the test environment, creates a finding with enrichments, and sends it to Slack. However, the assertions only check for string presence without verifying the message structure or format.
Consider enhancing the assertions to verify the structure of the Slack message:
slack_sender.send_finding_to_slack(finding, preview_params, platform_enabled=True) latest_message = slack_channel.get_complete_latest_message() - # Check for key elements in the preview message - assert "Test Preview Template" in latest_message - assert "This is a test preview block." in latest_message - assert "Additional Information" in latest_message - assert "Test point 1" in latest_message - assert "Test point 2" in latest_message + # Check for key elements in the preview message + assert len(latest_message) > 0, "No messages received" + first_message = latest_message[0] + + # Verify the message contains the expected blocks/attachments + assert "blocks" in first_message or "attachments" in first_message, "Message lacks blocks or attachments" + + # Check for key content elements + message_text = first_message.get("text", "") + assert "Test Preview Template" in message_text, "Title not found in message" + + # Check blocks or attachments contain expected content + all_content = str(first_message) # Convert entire message structure to string for content checks + assert "This is a test preview block." in all_content, "Preview block not found" + assert "Additional Information" in all_content, "Additional info not found" + assert "Test point 1" in all_content and "Test point 2" in all_content, "Bullet points not found"
67-126: The custom template test could use more structured assertionsSimilar to the previous test, the assertions only check for string presence. Additionally, the test doesn't verify that the custom template was actually applied correctly to the message structure.
Consider enhancing the assertions:
slack_sender.send_finding_to_slack(finding, preview_params, platform_enabled=True) latest_message = slack_channel.get_complete_latest_message() - # Check for custom template elements - assert "CUSTOM PREVIEW: Test Custom Preview Template" in latest_message - assert "Testing custom preview template rendering" in latest_message - assert "This is a test custom preview block." in latest_message - assert "Custom Content" in latest_message - assert "Custom point 1" in latest_message - assert "Custom point 2" in latest_message + # Check for custom template elements + assert len(latest_message) > 0, "No messages received" + first_message = latest_message[0] + + # Verify the message contains blocks that match our custom template format + all_content = str(first_message) + custom_title = "CUSTOM PREVIEW: Test Custom Preview Template" + custom_description = "Testing custom preview template rendering" + + assert custom_title in all_content, "Custom title format not found" + assert custom_description in all_content, "Custom description not found" + assert "This is a test custom preview block." in all_content, "Preview block not found" + assert "Custom Content" in all_content, "Custom section header not found" + assert "Custom point 1" in all_content and "Custom point 2" in all_content, "Custom bullet points not found" + + # Verify template was applied correctly + if "blocks" in first_message: + # Check if any block contains our custom formatted text + found_custom_template = False + for block in first_message["blocks"]: + if block.get("type") == "section" and "text" in block: + text_content = block["text"].get("text", "") + if custom_title in text_content and custom_description in text_content: + found_custom_template = True + break + assert found_custom_template, "Custom template format not applied correctly"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_slack_preview.py(1 hunks)tests/utils/slack_utils.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/test_slack_preview.py
4-4: pytest imported but unused
Remove unused import: pytest
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (2)
tests/utils/slack_utils.py (1)
25-29: Implementation is clean and follows the existing pattern.The new
get_complete_latest_messagemethod follows the same pattern as the existingget_latest_messagemethod, but returns the full messages list instead of just the text of the latest message. This facilitates more comprehensive testing of Slack message structure and content.tests/test_slack_preview.py (1)
112-114:Details
❌ Incorrect review comment
Consider setting template_name in SlackSinkPreviewParams
The custom template is defined and passed as
slack_custom_templates, but there's no parameter settingtemplate_nameto use this custom template. Either the test is relying on default behavior to use the custom template, or the parameter is missing.preview_params = SlackSinkPreviewParams( name="test_custom_preview", slack_channel=slack_channel.channel_name, api_key="", investigate_link=True, prefer_redirect_to_platform=False, max_log_file_limit_kb=1000, + template_name="custom.j2", slack_custom_templates={"custom.j2": custom_template} )Run the following to verify how template selection works:
🏁 Script executed:
#!/bin/bash # Check if SlackSinkPreviewParams has a template_name parameter and how it's used grep -A 20 "class SlackSinkPreviewParams" $(find src -name "slack_sink_preview_params.py") # Check in slack_sender.py how the template is selected grep -A 30 "def send_finding_to_slack" $(find src -name "sender.py" | grep "slack")Length of output: 2141
No change needed for template_name
The preview params already auto-select the single custom template when only one is provided. In SlackSinkPreviewParams.get_effective_template_name(), if slack_custom_templates has exactly one entry it returns that key—so your test will correctly use
"custom.j2"without explicitly setting template_name.Likely an incorrect or invalid review comment.
rename test preview pytest fix test fix test fix update slack_utils pytest changes attempting to debug slack test fixing pytest preview sink
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/manual_tests/test_slack_integration_manual.py (2)
41-41:⚠️ Potential issueRemove hardcoded token placeholder
The default placeholder token is truthy and will bypass the empty token check in the main function.
This could lead to unexpected errors when trying to connect to Slack with an invalid token. Instead:
-SLACK_TOKEN = os.environ.get("SLACK_TOKEN", "xoxb-your-actual-token-here") +SLACK_TOKEN = os.environ.get("SLACK_TOKEN")
428-431: 🛠️ Refactor suggestionImprove token validation
The current validation only checks if the token is empty, but doesn't detect placeholder tokens.
Add stronger validation to prevent attempting to connect with an invalid token:
def main(): - if not SLACK_TOKEN: + if not SLACK_TOKEN or SLACK_TOKEN == "xoxb-your-actual-token-here": logging.error("SLACK_TOKEN environment variable not set. Please set it to proceed.") sys.exit(1) + + # Additional validation for token format + if not (SLACK_TOKEN.startswith("xoxb-") or SLACK_TOKEN.startswith("xapp-")): + logging.warning("SLACK_TOKEN may not be valid - should start with 'xoxb-' or 'xapp-'")
🧹 Nitpick comments (4)
tests/test_slack_preview.py (2)
4-4: Remove unused importThe
pytestpackage is imported but not directly used in the code.While pytest imports are sometimes kept for discoverability, it's better to remove unused imports when they're flagged by linters:
-import pytest🧰 Tools
🪛 Ruff (0.11.9)
4-4:
pytestimported but unusedRemove unused import:
pytest(F401)
16-35:extract_text_from_blockscould handle more block typesThe function only extracts text from 'section' type blocks, but Slack messages often contain other block types with text content.
Consider enhancing the function to extract text from other block types like
header,context, andrich_textblocks:def extract_text_from_blocks(message): """Extract all text content from Slack message blocks and attachments""" text_parts = [] # Extract text from main blocks if 'blocks' in message: for block in message['blocks']: if block.get('type') == 'section' and 'text' in block: text_parts.append(block['text'].get('text', '')) + elif block.get('type') == 'header' and 'text' in block: + text_parts.append(block['text'].get('text', '')) + elif block.get('type') == 'context' and 'elements' in block: + for element in block['elements']: + if element.get('type') in ('mrkdwn', 'plain_text') and 'text' in element: + text_parts.append(element.get('text', '')) # Extract text from attachments if 'attachments' in message: for attachment in message['attachments']: if 'blocks' in attachment: for block in attachment['blocks']: if block.get('type') == 'section' and 'text' in block: text_parts.append(block['text'].get('text', '')) + elif block.get('type') == 'header' and 'text' in block: + text_parts.append(block['text'].get('text', '')) + elif block.get('type') == 'context' and 'elements' in block: + for element in block['elements']: + if element.get('type') in ('mrkdwn', 'plain_text') and 'text' in element: + text_parts.append(element.get('text', '')) return ' '.join(text_parts)tests/manual_tests/test_slack_integration_manual.py (2)
10-11: Clean up unused importsSeveral imported items are never used in the code.
Remove these unused imports to improve code clarity:
-from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingStatus, FindingSubject, Link, \ +from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingSubject, Link, \ EnrichmentType from robusta.core.reporting.consts import FindingSubjectType, FindingType from robusta.core.reporting.base import LinkType from robusta.core.reporting.blocks import ( MarkdownBlock, HeaderBlock, DividerBlock, TableBlock, ListBlock, LinkProp, LinksBlock, - CallbackBlock, - CallbackChoice, FileBlock ) -from robusta.core.model.base_params import ResourceInfo from robusta.core.sinks.slack.slack_sink_params import SlackSinkParams -from robusta.core.sinks.slack.preview.slack_sink_preview_params import SlackSinkPreviewParams, SlackTemplateStyle +from robusta.core.sinks.slack.preview.slack_sink_preview_params import SlackSinkPreviewParams -from robusta.core.playbooks.internal.ai_integration import ask_holmesAlso applies to: 22-23, 26-29
🧰 Tools
🪛 Ruff (0.11.9)
10-10:
robusta.core.reporting.base.FindingStatusimported but unusedRemove unused import:
robusta.core.reporting.base.FindingStatus(F401)
373-379: Improve code formatting for readabilityThe markdown block content runs very long and contains embedded HTML-like syntax.
Consider breaking the long markdown content into multiple lines for better readability:
finding.add_enrichment([ MarkdownBlock( - '📘 *Alert Explanation:* This pod is throttled due to its CPU limit. This can occur even when CPU usage is far below the limit. <https://github.com/robusta-dev/alert-explanations/wiki/CPUThrottlingHigh-(Prometheus-Alert)|Learn more.>'), + '📘 *Alert Explanation:* This pod is throttled due to its CPU limit. ' + 'This can occur even when CPU usage is far below the limit. ' + '<https://github.com/robusta-dev/alert-explanations/wiki/CPUThrottlingHigh-(Prometheus-Alert)|Learn more.>'), MarkdownBlock( - "🛠 *Robusta's Recommendation:* Remove this pod's CPU limit entirely. <https://home.robusta.dev/blog/stop-using-cpu-limits/|Using CPU limits is *not* a best practice.>"),table_block + "🛠 *Robusta's Recommendation:* Remove this pod's CPU limit entirely. " + "<https://home.robusta.dev/blog/stop-using-cpu-limits/|Using CPU limits is *not* a best practice.>"), + table_block ], annotations={"unfurl": False})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/manual_tests/test_slack_integration_manual.py(1 hunks)tests/test_slack_preview.py(1 hunks)tests/utils/slack_utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/utils/slack_utils.py
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/test_slack_preview.py
4-4: pytest imported but unused
Remove unused import: pytest
(F401)
tests/manual_tests/test_slack_integration_manual.py
10-10: robusta.core.reporting.base.FindingStatus imported but unused
Remove unused import: robusta.core.reporting.base.FindingStatus
(F401)
22-22: robusta.core.reporting.blocks.CallbackBlock imported but unused
Remove unused import
(F401)
23-23: robusta.core.reporting.blocks.CallbackChoice imported but unused
Remove unused import
(F401)
26-26: robusta.core.model.base_params.ResourceInfo imported but unused
Remove unused import: robusta.core.model.base_params.ResourceInfo
(F401)
28-28: robusta.core.sinks.slack.preview.slack_sink_preview_params.SlackTemplateStyle imported but unused
Remove unused import: robusta.core.sinks.slack.preview.slack_sink_preview_params.SlackTemplateStyle
(F401)
29-29: robusta.core.playbooks.internal.ai_integration.ask_holmes imported but unused
Remove unused import: robusta.core.playbooks.internal.ai_integration.ask_holmes
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (4)
tests/test_slack_preview.py (2)
36-87: Code looks goodThe test function
test_slack_preview_default_templateis well-structured and thoroughly tests the Slack preview functionality with a default template.
88-147: Code looks goodThe test function
test_slack_preview_custom_templateis well-structured and thoroughly tests the Slack preview functionality with a custom template.tests/manual_tests/test_slack_integration_manual.py (2)
54-111: Function implementation looks goodThe
create_test_findingfunction is well-structured and creates a comprehensive test finding with all necessary properties.
524-650: Test structure looks goodThe various test scenarios (standard sink, preview sink with default/custom template, channel override) are well-structured and cover different aspects of the Slack integration functionality.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/robusta/core/sinks/slack/preview/slack_sink_preview_params.py (1)
8-12: 🛠️ Refactor suggestionMissing template_style field referenced in past review comments
The class is missing a
template_stylefield that was mentioned in past review comments. This field would be used to determine which template to use (legacy vs default) based on user preferences.class SlackSinkPreviewParams(SlackSinkParams): #TODO: improve the SlackSinkPreviewParams so the slack_custom_templates can be defined once globally and # only a template name needs to be passed to each channel in the config slack_custom_templates: Optional[Dict[str, str]] = None # Template name -> custom template content + template_name: Optional[str] = Nonetests/manual_tests/test_slack_integration_manual.py (1)
41-47:⚠️ Potential issueFix token validation to prevent use of placeholder token
The script uses a placeholder Slack token as default, which might lead to attempted API usage with an invalid token. The current validation won't detect this placeholder.
-SLACK_TOKEN = os.environ.get("SLACK_TOKEN", "xoxb-your-actual-token-here") +SLACK_TOKEN = os.environ.get("SLACK_TOKEN") # The Slack channel to send messages to SLACK_CHANNEL = os.environ.get("SLACK_CHANNEL", "test-robusta") # Optional channel override for testing label-based routing SLACK_OVERRIDE_CHANNEL = os.environ.get("SLACK_OVERRIDE_CHANNEL", "") # Optional Slack user ID to mention in test messages (e.g., "U1234567890") SLACK_MENTION = os.environ.get("SLACK_MENTION", "")And then update the validation in the main function:
def main(): - if not SLACK_TOKEN: + if not SLACK_TOKEN or SLACK_TOKEN == "xoxb-your-actual-token-here": logging.error("SLACK_TOKEN environment variable not set. Please set it to proceed.") sys.exit(1) + + # Simple format validation + if not (SLACK_TOKEN.startswith("xoxb-") or SLACK_TOKEN.startswith("xapp-")): + logging.warning("SLACK_TOKEN may not be valid - should start with 'xoxb-' or 'xapp-'")
🧹 Nitpick comments (6)
src/robusta/core/sinks/slack/preview/slack_sink_preview_params.py (1)
13-17: Consider relaxing the "exactly one key-value pair" constraintThe validator restricts
slack_custom_templatesto contain exactly one key-value pair, which seems unnecessarily limiting. Consider allowing multiple templates to be defined, especially since the comment above suggests improving this in the future.You could modify the validator to support multiple templates or remove this constraint entirely:
@validator('slack_custom_templates') def check_one_item(cls, v): - if v is not None and len(v) != 1: - raise ValueError("slack_custom_templates must contain exactly one key-value pair") + if v is not None and len(v) < 1: + raise ValueError("slack_custom_templates must contain at least one key-value pair") return vtests/manual_tests/test_slack_integration_manual.py (1)
10-30: Remove unused importsSeveral imported modules and classes are not used in the code, as flagged by the static analysis.
from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingStatus, FindingSubject, Link, \ EnrichmentType -from robusta.core.reporting.consts import FindingSubjectType, FindingType +from robusta.core.reporting.consts import FindingSubjectType, FindingType from robusta.core.reporting.base import LinkType from robusta.core.reporting.blocks import ( MarkdownBlock, HeaderBlock, DividerBlock, TableBlock, ListBlock, LinkProp, LinksBlock, - CallbackBlock, - CallbackChoice, FileBlock ) -from robusta.core.model.base_params import ResourceInfo from robusta.core.sinks.slack.slack_sink_params import SlackSinkParams from robusta.core.sinks.slack.preview.slack_sink_preview_params import SlackSinkPreviewParams -from robusta.core.playbooks.internal.ai_integration import ask_holmes🧰 Tools
🪛 Ruff (0.11.9)
10-10:
robusta.core.reporting.base.FindingStatusimported but unusedRemove unused import:
robusta.core.reporting.base.FindingStatus(F401)
22-22:
robusta.core.reporting.blocks.CallbackBlockimported but unusedRemove unused import
(F401)
23-23:
robusta.core.reporting.blocks.CallbackChoiceimported but unusedRemove unused import
(F401)
26-26:
robusta.core.model.base_params.ResourceInfoimported but unusedRemove unused import:
robusta.core.model.base_params.ResourceInfo(F401)
29-29:
robusta.core.playbooks.internal.ai_integration.ask_holmesimported but unusedRemove unused import:
robusta.core.playbooks.internal.ai_integration.ask_holmes(F401)
src/robusta/integrations/slack/sender.py (4)
512-516: Unused variable: template_nameThe
template_namevariable is retrieved but never used, as it's immediately overwritten by the result ofsink_params.get_template_name().# Determine the template name to use -template_name = sink_params.get_template_name() if sink_params else "header.j2" +const template_name = sink_params.get_template_name() if sink_params else "header.j2" # Get the custom template for this template name, if any custom_template = sink_params.get_custom_template() if sink_params else None🧰 Tools
🪛 Ruff (0.11.9)
512-512: Local variable
template_nameis assigned to but never usedRemove assignment to unused variable
template_name(F841)
771-794: Remove unused return value in AI_ANALYSIS branchThe method returns an empty string after
self.send_holmes_analysis()which is never used by any caller.def __send_finding_to_slack_preview( self, finding: Finding, sink_params: SlackSinkParams, platform_enabled: bool, thread_ts: str = None, -) -> str: +) -> None: blocks: List[BaseBlock] = [] attachment_blocks: List[BaseBlock] = [] header_blocks: List[SlackBlock] = [] # JIRA-style header blocks slack_channel = ChannelTransformer.template( sink_params.channel_override, sink_params.slack_channel, self.cluster_name, finding.subject.labels, finding.subject.annotations, ) if finding.finding_type == FindingType.AI_ANALYSIS: # holmes analysis message needs special handling self.send_holmes_analysis(finding, slack_channel, platform_enabled, thread_ts) - return "" # [arik] Looks like the return value here is not used, needs to be removed + returnMake the same change in the
__send_finding_to_slackmethod as well.
869-873: Simplify thread_ts handling with ternary operatorThe if-else block to set
kwargsbased onthread_tscan be simplified using a ternary operator.-if thread_ts: - kwargs = {"thread_ts": thread_ts} -else: - kwargs = {} +kwargs = {"thread_ts": thread_ts} if thread_ts else {}🧰 Tools
🪛 Ruff (0.11.9)
869-872: Use ternary operator
kwargs = {"thread_ts": thread_ts} if thread_ts else {}instead ofif-else-blockReplace
if-else-block withkwargs = {"thread_ts": thread_ts} if thread_ts else {}(SIM108)
890-899: Consider reducing debug log verbosityThe detailed JSON logging could be very verbose and potentially expose sensitive information. Consider adding conditional logging or reducing the verbosity level.
logging.debug( f"SENDING TO SLACK - FULL BLOCKS DETAIL:\n" f"CHANNEL: {slack_channel}\n" f"TITLE: {finding.title}\n" - f"HEADER BLOCKS: {json.dumps(header_blocks, indent=2)}\n" - f"MAIN BLOCKS: {json.dumps(output_blocks, indent=2)}\n" - f"ALL BLOCKS: {json.dumps(all_blocks, indent=2)}\n" - f"ATTACHMENT BLOCKS: {json.dumps(all_attachment_blocks, indent=2)}\n" - f"ATTACHMENTS: {json.dumps(attachments, indent=2)}\n" + f"BLOCK COUNT: header={len(header_blocks)}, main={len(output_blocks)}, attachment={len(all_attachment_blocks)}\n" ) # For more detailed logging, consider using a higher log level if logging.getLogger().isEnabledFor(logging.TRACE): # assuming TRACE is defined logging.trace( f"HEADER BLOCKS: {json.dumps(header_blocks, indent=2)}\n" f"MAIN BLOCKS: {json.dumps(output_blocks, indent=2)}\n" f"ALL BLOCKS: {json.dumps(all_blocks, indent=2)}\n" f"ATTACHMENT BLOCKS: {json.dumps(all_attachment_blocks, indent=2)}\n" f"ATTACHMENTS: {json.dumps(attachments, indent=2)}\n" )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/robusta/core/sinks/slack/__init__.py(1 hunks)src/robusta/core/sinks/slack/preview/slack_sink_preview_params.py(1 hunks)src/robusta/core/sinks/slack/templates/template_loader.py(1 hunks)src/robusta/integrations/slack/sender.py(10 hunks)tests/manual_tests/test_slack_integration_manual.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/robusta/core/sinks/slack/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/robusta/core/sinks/slack/preview/slack_sink_preview_params.py (2)
src/robusta/core/sinks/sink_base_params.py (1)
SinkBaseParams(103-159)src/robusta/core/sinks/slack/slack_sink_params.py (1)
SlackSinkParams(9-26)
🪛 Ruff (0.11.9)
tests/manual_tests/test_slack_integration_manual.py
10-10: robusta.core.reporting.base.FindingStatus imported but unused
Remove unused import: robusta.core.reporting.base.FindingStatus
(F401)
22-22: robusta.core.reporting.blocks.CallbackBlock imported but unused
Remove unused import
(F401)
23-23: robusta.core.reporting.blocks.CallbackChoice imported but unused
Remove unused import
(F401)
26-26: robusta.core.model.base_params.ResourceInfo imported but unused
Remove unused import: robusta.core.model.base_params.ResourceInfo
(F401)
29-29: robusta.core.playbooks.internal.ai_integration.ask_holmes imported but unused
Remove unused import: robusta.core.playbooks.internal.ai_integration.ask_holmes
(F401)
src/robusta/integrations/slack/sender.py
512-512: Local variable template_name is assigned to but never used
Remove assignment to unused variable template_name
(F841)
869-872: Use ternary operator kwargs = {"thread_ts": thread_ts} if thread_ts else {} instead of if-else-block
Replace if-else-block with kwargs = {"thread_ts": thread_ts} if thread_ts else {}
(SIM108)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (4)
src/robusta/integrations/slack/sender.py (4)
95-103: LGTM! String sanitization for JSON compatibilityGood implementation of the string sanitization method that handles newlines properly for JSON compatibility in templates. This helps ensure templates render correctly in Slack.
434-458: Comprehensive template context data preparationThe header building implementation is thorough and well-structured, providing a rich set of data for templates including status, severity, alert type, and resource information with appropriate emoji selection.
817-834: Good approach to handling file blocks separatelyThe approach to separate file blocks from other enrichment blocks and handle them appropriately is well-designed. This prevents Slack API errors and ensures proper rendering of the content.
617-693: Efficient branching for preview vs standard modesThe implementation of
send_finding_to_slackwith a fallback mechanism is well-designed. It attempts the preview mode first and gracefully falls back to the standard mode if an error occurs. The exception handling and logging provide clear diagnostic information.🧰 Tools
🪛 Ruff (0.11.9)
643-646: Use ternary operator
kwargs = {"thread_ts": thread_ts} if thread_ts else {}instead ofif-else-blockReplace
if-else-block withkwargs = {"thread_ts": thread_ts} if thread_ts else {}(SIM108)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/robusta/integrations/slack/sender.py (1)
806-809: Avoid duplicate "Investigate" buttonsWhen both
platform_enabledandinclude_investigate_linkare true, this code will create a duplicate "Investigate" button, as the template already includes one.- links_block: LinksBlock = self.__create_links( - finding, platform_enabled, sink_params.investigate_link, sink_params.prefer_redirect_to_platform - ) - blocks.append(links_block) + # Only add a manual "Investigate" button if the template didn't already include it + if not (platform_enabled and sink_params.investigate_link): + links_block: LinksBlock = self.__create_links( + finding, + platform_enabled, + sink_params.investigate_link, + sink_params.prefer_redirect_to_platform, + ) + blocks.append(links_block)
🧹 Nitpick comments (4)
src/robusta/integrations/slack/sender.py (4)
9-9: Remove unused importThe
jsonmodule is imported but not used anywhere in the file.-import json🧰 Tools
🪛 Ruff (0.11.9)
9-9:
jsonimported but unusedRemove unused import:
json(F401)
512-512: Remove unused variable assignmentThe
template_namevariable is assigned but never used in the code.- template_name = sink_params.get_template_name() if sink_params else "header.j2"🧰 Tools
🪛 Ruff (0.11.9)
512-512: Local variable
template_nameis assigned to but never usedRemove assignment to unused variable
template_name(F841)
791-792: Remove unused return value as noted in TODO commentThe code includes a TODO comment from "arik" noting that the return value isn't used.
- return "" # [arik] Looks like the return value here is not used, needs to be removed + return None
771-852: Consider reducing code duplication between preview and non-preview methodsThe
__send_finding_to_slack_previewmethod shares significant code with__send_finding_to_slack. Consider extracting common functionality to a shared private method to improve maintainability.You could create a helper method like
__process_finding_enrichmentsthat both methods can call to handle the enrichments, attachments, and unfurl settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/robusta/integrations/slack/sender.py(10 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
src/robusta/integrations/slack/sender.py
9-9: json imported but unused
Remove unused import: json
(F401)
512-512: Local variable template_name is assigned to but never used
Remove assignment to unused variable template_name
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (3)
src/robusta/integrations/slack/sender.py (3)
97-102: LGTM: New sanitization method for template stringsThis helper method correctly handles newline escaping for JSON compatibility in templates, making the code more robust when dealing with multi-line text in Slack messages.
434-522: LGTM: Well-structured template context builderThe new header creation method provides a rich context object with appropriate emoji selection based on resource types. The implementation is thorough and handles various edge cases like missing values.
🧰 Tools
🪛 Ruff (0.11.9)
512-512: Local variable
template_nameis assigned to but never usedRemove assignment to unused variable
template_name(F841)
677-694: LGTM: Well-designed preview mode with fallbackThe method properly handles the preview mode flag and includes graceful fallback to the legacy method if the preview rendering fails.
Avi-Robusta
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_slack_preview.py (2)
64-71: Consider adding more test cases for template configurations.The tests cover basic functionality but could be expanded to test more template configurations. Consider parameterizing these tests to verify different template options like
prefer_redirect_to_platform,investigate_link, and various custom template formats.@pytest.mark.parametrize("investigate_link,prefer_redirect,expected_content", [ (True, False, "investigate link text"), (False, True, "platform redirect text"), (True, True, "platform preferred text"), ]) def test_slack_preview_template_options(slack_channel, investigate_link, prefer_redirect, expected_content): # Test with different configuration options # ...Also applies to: 123-131
114-122: Consider using a separate file for test templates.For better maintainability, consider moving test templates to separate files, especially if you plan to add more complex templates in the future. This also better mimics how users would typically use custom templates.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_slack_preview.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (3)
tests/test_slack_preview.py (3)
12-30: Well-implemented utility function.The
extract_text_from_blocksfunction effectively extracts text from both main blocks and attachments in Slack messages, which is useful for assertions in the tests.
32-83: Good test coverage for default template.The test creates a realistic Finding with enrichments and verifies that all expected content appears in the rendered Slack message. The setup is thorough and the assertions check all important elements.
84-144: Comprehensive test for custom templates.This test effectively validates that custom Slack templates are properly rendered with variables replaced. The custom template JSON is well-formed and the assertions verify both the template-specific content and the enrichment blocks.
added hide_buttons, added hide_enrichments does not show description on custom template
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/robusta/core/sinks/slack/preview/slack_sink_preview_params.py (1)
16-20: Validator enforces single template but design may be too restrictive.The validator requires exactly one key-value pair when
slack_custom_templatesis set, but the TODO comment on lines 9-10 suggests a future design where templates can be defined globally and referenced by name. This constraint may need to be relaxed in the future.Consider if this validation aligns with the intended roadmap or if it should be more flexible.
tests/manual_tests/test_slack_integration_manual.py (2)
41-41:⚠️ Potential issueFix placeholder token validation.
The placeholder token
"xoxb-your-actual-token-here"is still being used as a default, which bypasses the validation check on line 429. This was flagged in previous reviews but hasn't been addressed.Apply this fix:
-SLACK_TOKEN = os.environ.get("SLACK_TOKEN", "xoxb-your-actual-token-here") +SLACK_TOKEN = os.environ.get("SLACK_TOKEN")
429-431: 🛠️ Refactor suggestionEnhance token validation to detect placeholder values.
The current validation only checks for empty tokens but doesn't detect the placeholder value that could be set as the default.
Apply this enhancement:
- if not SLACK_TOKEN: + if not SLACK_TOKEN or SLACK_TOKEN == "xoxb-your-actual-token-here": logging.error("SLACK_TOKEN environment variable not set. Please set it to proceed.") sys.exit(1)src/robusta/integrations/slack/sender.py (1)
713-713: 🛠️ Refactor suggestionAddress unused return values as noted in comments.
The comments indicate that the return values from the AI_ANALYSIS branches are not used and should be removed. This was flagged in previous reviews.
- return "" # [arik] Looks like the return value here is not used, needs to be removed + return # Holmes analysis handled separatelyAlso consider updating the method signature to return
Noneinstead ofstrsince the return value isn't meaningful.Also applies to: 789-789
🧹 Nitpick comments (2)
tests/manual_tests/test_slack_integration_manual.py (1)
10-29: Remove unused imports to clean up code.Static analysis correctly identifies several unused imports that should be removed to improve code cleanliness.
-from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingStatus, FindingSubject, Link, \ - EnrichmentType +from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingSubject, Link, \ + EnrichmentType-from robusta.core.reporting.blocks import ( - MarkdownBlock, - HeaderBlock, - DividerBlock, - TableBlock, - ListBlock, - LinkProp, - LinksBlock, - CallbackBlock, - CallbackChoice, - FileBlock -) +from robusta.core.reporting.blocks import ( + MarkdownBlock, + HeaderBlock, + DividerBlock, + TableBlock, + ListBlock, + LinkProp, + LinksBlock, + FileBlock +) -from robusta.core.model.base_params import ResourceInfo -from robusta.core.playbooks.internal.ai_integration import ask_holmes🧰 Tools
🪛 Ruff (0.11.9)
10-10:
robusta.core.reporting.base.FindingStatusimported but unusedRemove unused import:
robusta.core.reporting.base.FindingStatus(F401)
22-22:
robusta.core.reporting.blocks.CallbackBlockimported but unusedRemove unused import
(F401)
23-23:
robusta.core.reporting.blocks.CallbackChoiceimported but unusedRemove unused import
(F401)
26-26:
robusta.core.model.base_params.ResourceInfoimported but unusedRemove unused import:
robusta.core.model.base_params.ResourceInfo(F401)
29-29:
robusta.core.playbooks.internal.ai_integration.ask_holmesimported but unusedRemove unused import:
robusta.core.playbooks.internal.ai_integration.ask_holmes(F401)
src/robusta/integrations/slack/sender.py (1)
9-9: Remove unused json import.Static analysis correctly identifies that the
jsonimport is not used anywhere in the file.-import json🧰 Tools
🪛 Ruff (0.11.9)
9-9:
jsonimported but unusedRemove unused import:
json(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/configuration/sinks/slack.rst(1 hunks)src/robusta/core/sinks/slack/preview/slack_sink_preview_params.py(1 hunks)src/robusta/integrations/slack/sender.py(10 hunks)tests/manual_tests/test_slack_integration_manual.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/configuration/sinks/slack.rst
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/manual_tests/test_slack_integration_manual.py
10-10: robusta.core.reporting.base.FindingStatus imported but unused
Remove unused import: robusta.core.reporting.base.FindingStatus
(F401)
22-22: robusta.core.reporting.blocks.CallbackBlock imported but unused
Remove unused import
(F401)
23-23: robusta.core.reporting.blocks.CallbackChoice imported but unused
Remove unused import
(F401)
26-26: robusta.core.model.base_params.ResourceInfo imported but unused
Remove unused import: robusta.core.model.base_params.ResourceInfo
(F401)
29-29: robusta.core.playbooks.internal.ai_integration.ask_holmes imported but unused
Remove unused import: robusta.core.playbooks.internal.ai_integration.ask_holmes
(F401)
src/robusta/integrations/slack/sender.py
9-9: json imported but unused
Remove unused import: json
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (8)
src/robusta/core/sinks/slack/preview/slack_sink_preview_params.py (2)
22-27: LGTM: Clean implementation of custom template retrieval.The
get_custom_template()method cleanly extracts the single custom template value when available, which aligns well with the validator constraint.
30-34: LGTM: Standard configuration wrapper pattern.The
SlackSinkPreviewConfigWrapperfollows the established pattern for sink configuration wrappers in the codebase.src/robusta/integrations/slack/sender.py (6)
66-87: LGTM: Clean constructor enhancement for preview mode.The addition of the
is_previewparameter with a sensible default maintains backward compatibility while enabling the new preview functionality.
97-102: Good addition: String sanitization for JSON templates.The
__slack_preview_sanitize_stringmethod properly handles newline escaping for JSON compatibility, which is essential for the Jinja2 template rendering.
434-518: Well-implemented preview header generation.The
__create_finding_header_previewmethod creates a comprehensive template context with all necessary variables for rich Slack message headers. The resource emoji mapping and conditional template rendering are well thought out.
668-690: Excellent routing logic with fallback handling.The main
send_finding_to_slackmethod elegantly routes to preview or legacy mode based on theis_previewflag, with proper exception handling that falls back to legacy mode if preview rendering fails.
768-859: Well-structured preview implementation with good configurability.The
__send_finding_to_slack_previewmethod properly implements the preview mode with configurable options for hiding buttons and enrichments. The logic flow mirrors the legacy method while adding template-based header generation.
899-922: Good consistency improvement: Source renamed to Cluster.The change from "Source:" to "Cluster:" in the summary message provides better consistency with the preview templates and clearer labeling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/manual_tests/test_slack_integration_manual.py (3)
41-41:⚠️ Potential issueFix placeholder token issue
The placeholder token will pass the truthiness check, preventing proper validation. This is a duplicate of a previously identified issue.
-SLACK_TOKEN = os.environ.get("SLACK_TOKEN", "xoxb-your-actual-token-here") +SLACK_TOKEN = os.environ.get("SLACK_TOKEN")
378-378:⚠️ Potential issueFix syntax error in markdown block concatenation
There's a syntax error where a markdown string is being concatenated directly with a TableBlock object without proper separation. This is a duplicate of a previously identified issue.
- "🛠 *Robusta's Recommendation:* Remove this pod's CPU limit entirely. <https://home.robusta.dev/blog/stop-using-cpu-limits/|Using CPU limits is *not* a best practice.>"),table_block + "🛠 *Robusta's Recommendation:* Remove this pod's CPU limit entirely. <https://home.robusta.dev/blog/stop-using-cpu-limits/|Using CPU limits is *not* a best practice.>"), + table_block
429-431:⚠️ Potential issueEnhance token validation
The current validation only checks for empty tokens but won't catch placeholder or invalid tokens. This is a duplicate of a previously identified issue.
def main(): - if not SLACK_TOKEN: + if not SLACK_TOKEN or SLACK_TOKEN == "xoxb-your-actual-token-here": logging.error("SLACK_TOKEN environment variable not set. Please set it to proceed.") sys.exit(1) + + # Optional: Add a simple format validation + if not SLACK_TOKEN.startswith("xoxb-") and not SLACK_TOKEN.startswith("xapp-"): + logging.warning("SLACK_TOKEN may not be valid - should start with 'xoxb-' or 'xapp-'")
🧹 Nitpick comments (1)
tests/manual_tests/test_slack_integration_manual.py (1)
10-11: Clean up unused importsSeveral imports are flagged as unused by static analysis tools and should be removed to keep the code clean.
-from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingStatus, FindingSubject, Link, \ - EnrichmentType +from robusta.core.reporting.base import Finding, FindingSource, FindingSeverity, FindingSubject, Link, \ + EnrichmentType from robusta.core.reporting.consts import FindingSubjectType, FindingType from robusta.core.reporting.base import LinkType from robusta.core.reporting.blocks import ( MarkdownBlock, HeaderBlock, DividerBlock, TableBlock, ListBlock, LinkProp, LinksBlock, - CallbackBlock, - CallbackChoice, FileBlock ) -from robusta.core.model.base_params import ResourceInfo from robusta.core.sinks.slack.slack_sink_params import SlackSinkParams from robusta.core.sinks.slack.preview.slack_sink_preview_params import SlackSinkPreviewParams -from robusta.core.playbooks.internal.ai_integration import ask_holmesAlso applies to: 22-23, 26-26, 29-29
🧰 Tools
🪛 Ruff (0.11.9)
10-10:
robusta.core.reporting.base.FindingStatusimported but unusedRemove unused import:
robusta.core.reporting.base.FindingStatus(F401)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/manual_tests/test_slack_integration_manual.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
tests/manual_tests/test_slack_integration_manual.py
10-10: robusta.core.reporting.base.FindingStatus imported but unused
Remove unused import: robusta.core.reporting.base.FindingStatus
(F401)
22-22: robusta.core.reporting.blocks.CallbackBlock imported but unused
Remove unused import
(F401)
23-23: robusta.core.reporting.blocks.CallbackChoice imported but unused
Remove unused import
(F401)
26-26: robusta.core.model.base_params.ResourceInfo imported but unused
Remove unused import: robusta.core.model.base_params.ResourceInfo
(F401)
29-29: robusta.core.playbooks.internal.ai_integration.ask_holmes imported but unused
Remove unused import: robusta.core.playbooks.internal.ai_integration.ask_holmes
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (4)
tests/manual_tests/test_slack_integration_manual.py (4)
173-233: Excellent test data creationThe crash loop finding creation function provides comprehensive and realistic test data that closely mimics real Kubernetes scenarios. The structured approach with proper metadata, labels, and enrichments will effectively exercise the Slack formatting capabilities.
438-456: Good test setup architectureThe initialization of both standard and preview SlackSender instances with consistent parameters provides excellent coverage for testing both sink modes. This design allows for easy comparison of formatting differences.
550-614: Comprehensive custom template testingThe custom Slack Block Kit JSON template demonstrates advanced templating capabilities with proper Jinja2 syntax, conditional rendering, and structured formatting. This provides excellent validation for the new templating feature.
617-657: Well-designed channel override testingThe conditional channel override test with label-based routing provides thorough coverage of the channel override functionality. The approach of adding labels to the finding and using them for routing is correctly implemented.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/robusta/integrations/slack/sender.py (2)
713-713: Remove unused return statementThis return statement is never used by any caller, as noted in previous reviews. The method signature should also be updated to return
Noneinstead ofstr.- return "" # [arik] Looks like the return value here is not used, needs to be removed + # AI analysis handled separately, no return needed
789-789: Remove unused return statementSame issue as in the legacy method - this return statement is never used by any caller.
- return "" # [arik] Looks like the return value here is not used, needs to be removed + # AI analysis handled separately, no return needed
🧹 Nitpick comments (3)
src/robusta/integrations/slack/sender.py (3)
9-9: Remove unused importThe
jsonmodule is imported but never used in this file. Static analysis tools and the template rendering logic suggest it's not needed.-import json🧰 Tools
🪛 Ruff (0.11.9)
9-9:
jsonimported but unusedRemove unused import:
json(F401)
434-518: Consider refactoring for improved maintainabilityWhile the logic is correct, this method is quite complex (80+ lines). Consider extracting the emoji mapping logic into a separate method to improve readability:
def __get_resource_emoji(self, subject_kind: str) -> str: """Get appropriate emoji for a given resource kind.""" emoji_map = { "pod": ":ship:", "deployment": ":package:", "node": ":computer:", "service": ":link:", "job": ":clock1:", "statefulset": ":chains:", } return emoji_map.get(subject_kind.lower(), ":package:")This would simplify the main method and make the emoji mappings easier to maintain.
768-859: Consider reducing code duplicationThere's significant overlap between
__send_finding_to_slack_previewand__send_finding_to_slack. Consider extracting common logic for enrichment processing and attachment handling into shared helper methods.This would improve maintainability and reduce the risk of inconsistencies between the two modes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/robusta/integrations/slack/sender.py(10 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
src/robusta/integrations/slack/sender.py
9-9: json imported but unused
Remove unused import: json
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (4)
src/robusta/integrations/slack/sender.py (4)
66-66: LGTM: Clean addition of preview mode supportThe
is_previewparameter with a sensible default maintains backward compatibility while enabling the new functionality.Also applies to: 87-87
97-102: LGTM: Robust newline sanitization logicThe two-step process (unescape then escape) ensures consistent JSON-safe output regardless of the input string's current escape state.
668-690: LGTM: Robust routing with fallback handlingThe conditional routing between preview and legacy modes with exception handling ensures the system remains functional even if preview rendering fails.
899-899: LGTM: Improved consistency in labelingChanging "Source:" to "Cluster:" provides clearer, more consistent labeling throughout the Slack messages.
Also applies to: 907-907, 922-922
arikalon1
left a comment
arikalon1
left a comment
There was a problem hiding this comment.
nice work
left a few comments
please verify that the old Slack sink works without issues as well
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/robusta/integrations/slack/sender.py (2)
798-801: Potential duplicate investigate buttons (previously identified)This issue was identified in previous reviews - when templates include investigate links and the code also adds links_block, users may see duplicate investigate buttons.
788-788: Unused return value (previously identified)As noted in previous reviews, the return value here is not used and should be removed along with updating the method signature to return None instead of str.
🧹 Nitpick comments (1)
src/robusta/integrations/slack/sender.py (1)
433-517: Consider refactoring this large method for better maintainabilityThis 85-line method handles multiple responsibilities. Consider breaking it down into smaller, focused methods:
+ def __build_template_context(self, finding: Finding, status: FindingStatus, platform_enabled: bool, include_investigate_link: bool) -> Dict[str, Any]: + """Build the template context for Slack preview messages.""" + # Move template context building logic here + + def __get_resource_info_with_emoji(self, finding: Finding) -> Tuple[str, str, str]: + """Extract resource information and determine appropriate emoji.""" + # Move resource emoji and ID logic here + + def __create_finding_header_preview(self, finding: Finding, status: FindingStatus, platform_enabled: bool, include_investigate_link: bool, sink_params: SlackSinkPreviewParams = None) -> List[SlackBlock]: + template_context = self.__build_template_context(finding, status, platform_enabled, include_investigate_link) + custom_template = sink_params.get_custom_template() if sink_params else None + if custom_template: + return template_loader.render_custom_template_to_blocks(custom_template, template_context) + else: + return template_loader.render_default_template_to_blocks(template_context)This improves readability and makes each method easier to test and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/robusta/integrations/slack/sender.py(11 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
src/robusta/integrations/slack/sender.py
313-313: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: run_tests
- GitHub Check: run_tests
🔇 Additional comments (5)
src/robusta/integrations/slack/sender.py (5)
8-8: LGTM: Required imports for the new preview functionalityThe new imports are necessary and correctly added:
Unionfor type hints in method signaturestemplate_loaderfor Jinja2 template renderingSlackSinkPreviewParamsfor preview-specific configurationAlso applies to: 15-15, 50-50
65-65: LGTM: Clean constructor enhancement for preview modeThe addition of the
is_previewparameter with a sensible default maintains backward compatibility while enabling the new preview functionality.Also applies to: 86-86
96-101: LGTM: Well-designed sanitization methodThe method correctly handles newline escaping for JSON compatibility by first unescaping any already escaped newlines to prevent double-escaping issues.
667-689: LGTM: Well-designed dispatch logic with graceful fallbackThe method elegantly handles the dispatch between preview and legacy modes with appropriate exception handling and fallback to ensure reliability.
887-887: LGTM: Improved text consistencyChanging "Source:" to "Cluster:" provides better consistency in the user interface terminology.
Also applies to: 895-895, 910-910
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/robusta/integrations/slack/sender.py (1)
313-317:⚠️ Potential issueMutable default argument risks cross-message leakage
output_blocksis initialised with[]in the function signature.
Because default arguments are evaluated once at import-time, blocks from one alert can accidentally appear in another.- output_blocks: List[SlackBlock] = [], + output_blocks: Optional[List[SlackBlock]] = None, ) -> str: + if output_blocks is None: + output_blocks = []Please also update the typing import (
Optional) if not already present.🧰 Tools
🪛 Ruff (0.11.9)
313-313: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
♻️ Duplicate comments (2)
src/robusta/integrations/slack/sender.py (2)
713-714: Return value is unused – consider changing toNoneBoth legacy and preview paths return an empty string after Holmes handling, and
SlackSinknever reads the value. ReturningNone(or better: changing the return type toNone) removes dead code and clarifies intent.Also applies to: 789-790
795-803: Potential duplicate “Investigate” buttonTemplates already include the investigate link when
include_investigate_linkis true, yet a manual button is still appended here. Users may see two identical buttons. Consider guarding the call to__create_linksor adding a parameter to suppress the link when the template handles it.
🧹 Nitpick comments (2)
docs/configuration/sinks/slack.rst (1)
186-257: YAML indentation & heading underline length mismatch can break the rendered docs
- In the YAML example (lines 195-207) the list items are indented two spaces instead of the standard four used throughout the rest of the page – Sphinx sometimes breaks the literal block when indentation is inconsistent.
- The underline below Message Templating (line 187) is 63 chars, while the heading text is 17 chars. Re-ST strips trailing spaces, so the visual mismatch is harmless but it violates the reStructuredText guideline that an underline “should be at least as long as the title text but not excessively longer”. Using exactly 17 (or 18) “-” characters keeps the style consistent with the other headings.
-Message Templating ------------------------------------------------------------------- +Message Templating +------------------and indent the example:
- sinksConfig: - - slack_sink_preview: - api_key: xoxb-198... - name: preview_slack_sink - slack_channel: demo-slack-preview - slack_custom_templates: - custom_template.j2: |- + sinksConfig: + - slack_sink_preview: + api_key: xoxb-198... + name: preview_slack_sink + slack_channel: demo-slack-preview + slack_custom_templates: + custom_template.j2: |-This guarantees correct rendering in Read-the-Docs and prevents copy-paste errors.
src/robusta/integrations/slack/sender.py (1)
468-483: Hard-coded emoji mapping is brittle – use a dictionaryThe long
elifchain forsubject_kind→ emoji is error-prone and difficult to extend (e.g. CronJob, DaemonSet). A mapping table is clearer and O(1):- if subject_kind.lower() == "pod": - resource_emoji = ":ship:" - elif subject_kind.lower() == "deployment": - resource_emoji = ":package:" - ... + emoji_map = { + "pod": ":ship:", + "deployment": ":package:", + "node": ":computer:", + "service": ":link:", + "job": ":clock1:", + "statefulset": ":chains:", + } + resource_emoji = emoji_map.get(subject_kind.lower(), resource_emoji)This simplifies maintenance and avoids missing cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/configuration/sinks/slack.rst(1 hunks)src/robusta/integrations/slack/sender.py(11 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
src/robusta/integrations/slack/sender.py
313-313: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: run_tests
- GitHub Check: run_tests
- GitHub Check: Deploy docs
🔇 Additional comments (1)
src/robusta/integrations/slack/sender.py (1)
96-102:__slack_preview_sanitize_stringre-escapes literal “\n” sequencesThe two chained
.replace()calls convert the first pass of “\n” to a newline and then immediately re-escape all newlines back to “\n”.
If the input already contained real newlines and literal “\n” sequences the end result is that every newline – real or literal – becomes the string “\n”, effectively losing the distinction.A safer, single-pass escape is:
- return text.replace("\\n", "\n").replace("\n", "\\n") + return text.replace("\n", "\\n")Or use
json.dumps(text)[1:-1]to delegate escaping to the std-lib.
[ suggest_optional_refactor ]
3 participants
Summary by CodeRabbit
New Features
SlackSinkPreviewsink type, allowing preview-specific message handling.Documentation
Bug Fixes
Tests
Changes in Format
Original Alert
New Default Alert
Using the Legacy configuration
Example custom formatting