Skip to content

Conversation

@MikaKerman
Copy link
Contributor

@MikaKerman MikaKerman commented Feb 4, 2025

null

@linear
Copy link

linear bot commented Feb 4, 2025

@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2025

👋 @MikaKerman
Thank you for raising your pull request.
Please make sure to add tests and document all user-facing changes.
You can do this by editing the docs files in this pull request.

@MikaKerman MikaKerman marked this pull request as draft February 4, 2025 16:06
@MikaKerman MikaKerman requested a review from ofek1weiss February 4, 2025 16:06
self,
destination: DestinationType,
message_context: MessageContextType,
message_body: MessageBody,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the names are a little confusing, since the parameters regard different messages, maybe message_context and reply_body

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about just body?

channel: Optional[str] = None


def send_adaptive_card(webhook_url: str, card: dict) -> requests.Response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of the comments in this method are redundant

@@ -0,0 +1,84 @@
from datetime import datetime
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this whole file is duplicated

- Moved alert grouping logic from integration to data monitoring alerts
- Added support for grouping all alerts when threshold is met
- Updated alert sending to handle individual alerts instead of bulk sending
- Simplified alert processing and added type hints
- Deprecated bulk alert sending methods in base integration
- Created base abstract class for messaging integrations with generic support
- Implemented `BaseMessagingIntegration` with abstract methods for sending messages and checking reply support
- Added custom exceptions for messaging integration errors
- Introduced `MessageSendResult` generic model to track message sending context
@MikaKerman MikaKerman force-pushed the ele-4028-messaging-integ branch from 044b0c0 to 53b5c68 Compare February 6, 2025 15:53
- Implemented `TeamsWebhookMessagingIntegration` for sending messages to Microsoft Teams
- Created support for sending Adaptive Cards via webhook
- Added error handling and logging for webhook message sending
- Implemented method to check reply support (not supported for Teams)
- Created detailed documentation explaining the new messaging integration architecture
- Described key components like `BaseMessagingIntegration` and migration strategy
- Outlined implementation guidelines for new messaging platform integrations
- Highlighted current and future support for messaging platforms
- Updated `DataMonitoringAlerts` to support both legacy and new messaging integrations
- Implemented dual-path alert sending logic for BaseIntegration and BaseMessagingIntegration
- Added `_send_message()` method to handle new messaging integration message sending
- Created `get_health_check_message()` for test message support in new integrations
- Updated `Integrations` class to support gradual migration to new messaging system
- Added destination resolution method for messaging integrations
@MikaKerman MikaKerman force-pushed the ele-4028-messaging-integ branch from 53b5c68 to e197ac6 Compare February 6, 2025 16:01
- Updated parameter name from `message_body` to `body` in base messaging integration methods
- Synchronized parameter naming across `BaseMessagingIntegration`, `TeamsWebhookMessagingIntegration`, and `DataMonitoringAlerts`
- Simplified method signatures and improved consistency in messaging integration code
- Updated README example to reflect new parameter naming convention
@MikaKerman MikaKerman marked this pull request as ready for review February 6, 2025 17:26
@MikaKerman MikaKerman merged commit af3c1eb into master Feb 9, 2025
4 checks passed
@MikaKerman MikaKerman deleted the ele-4028-messaging-integ branch February 9, 2025 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants