feat: add webhook #1
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This pull request introduces comprehensive webhook support to GitHub Runner Manager, allowing users to receive notifications on key runner events through multiple providers (Slack, Discord, Microsoft Teams, and generic webhooks).
- Webhook system implementation with configurable providers and event filtering
- Test commands for validating webhook configurations and templates
- Full test coverage with mocking to prevent real HTTP requests during testing
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/services/test_webhook_service.py | Comprehensive unit tests for WebhookService covering providers, notifications, payloads, retry logic, and formatters |
| tests/services/test_notification_service.py | Tests for NotificationService integration with webhook functionality and dispatcher behavior |
| tests/presentation/cli/test_webhook_commands_unit.py | Unit tests for webhook CLI commands covering prompt logic, error handling, and result display |
| tests/presentation/cli/test_webhook_commands.py | Integration tests for CLI webhook command invocation |
| tests/notifications/test_factory_events.py | Tests for event factory converting operation results to typed notification events |
| tests/conftest.py | Global test fixtures to mock webhook services and prevent real HTTP requests |
| src/services/webhook_service.py | Core webhook service implementation supporting multiple providers with retry logic and templating |
| src/services/notification_service.py | Refactored notification service using event dispatcher pattern while maintaining backward compatibility |
| src/services/config_schema.py | Extended configuration schema with webhook-specific models and validation |
| src/presentation/cli/webhook_commands.py | CLI commands for testing and debugging webhook configurations |
| src/presentation/cli/commands.py | Integration of webhook testing commands into main CLI application |
| src/notifications/factory.py | Factory utilities for converting Docker operation results to typed notification events |
| src/notifications/events.py | Typed notification event definitions using dataclasses |
| src/notifications/dispatcher.py | Event dispatcher routing notifications to registered channels |
| src/notifications/channels/webhook.py | Webhook channel implementation for the notification system |
| src/notifications/channels/base.py | Base interfaces and registry for notification channels |
| README.md | Updated documentation with webhook configuration examples and usage instructions |
| .coveragerc | Removed coverage configuration file |
Comments suppressed due to low confidence (1)
tests/services/test_notification_service.py:1
- This comment indicates that duplicate code was removed, but it appears to be a leftover comment that should be deleted as it doesn't provide value and may confuse future maintainers.
import types
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Comment on lines
369
to
402
| # Patch notification_service | ||
| monkeypatch.setattr( | ||
| commands, | ||
| "notification_service", | ||
| types.SimpleNamespace(notify_runner_removed=lambda d: notified.append(d)), | ||
| ) | ||
| # Patch config_service/dummy | ||
| monkeypatch.setattr( | ||
| commands, "config_service", types.SimpleNamespace(load_config=lambda: None) | ||
| ) | ||
| # Patch docker_service pour retourner deleted et skipped | ||
| monkeypatch.setattr( | ||
| commands, | ||
| "docker_service", | ||
| types.SimpleNamespace( | ||
| remove_runners=lambda: { | ||
| "deleted": [{"id": "x", "name": "foo"}], | ||
| "skipped": [{"name": "bar"}], | ||
| "errors": [], | ||
| } | ||
| ), | ||
| ) | ||
| # Patch console | ||
| monkeypatch.setattr( | ||
| commands, | ||
| "console", | ||
| types.SimpleNamespace(print=lambda *a, **k: printed.append(a[0] if a else "")), | ||
| ) | ||
| # Appel | ||
| commands.remove_runners() | ||
| # Vérifie notification | ||
| assert any(d["runner_id"] == "x" and d["runner_name"] == "foo" for d in notified) | ||
| # Vérifie affichage suppression indisponible | ||
| assert any("bar n'est pas disponible à la suppression" in s for s in printed) |
There was a problem hiding this comment.
This test contains duplicated code from lines 329-402. The entire setup (patches for notification_service, config_service, docker_service, and console) and test logic is repeated. This should be refactored to use a helper function or parameterized test to reduce duplication and improve maintainability.
Comment on lines
+273
to
+294
| class DummyConfig: | ||
| def __init__(self, enabled=True, providers=None, events=None): | ||
| self.webhooks = types.SimpleNamespace( | ||
| enabled=enabled, | ||
| dict=lambda: {}, | ||
| model_dump=lambda: {}, | ||
| ) | ||
| self.webhooks.dict = lambda: {} | ||
| self.webhooks.model_dump = lambda: {} | ||
| self.webhooks.enabled = enabled | ||
| self.webhooks.slack = types.SimpleNamespace( | ||
| enabled=True, events=events or [], templates={} | ||
| ) | ||
|
|
||
|
|
||
| class DummyWebhookService: | ||
| def __init__(self, providers=None): | ||
| self.providers = providers or {} | ||
| self._send_notification = lambda *a, **k: True | ||
|
|
||
| def notify(self, event_type, data, provider=None): | ||
| return {provider or "slack": True} |
There was a problem hiding this comment.
The DummyConfig and DummyWebhookService classes are duplicated from the fixture definitions at the top of the file (lines 13-42). This duplication should be removed and the existing fixtures should be used instead to improve maintainability and avoid inconsistencies.
glefer
force-pushed
the
feat/webhook
branch
from
4105b53 to
eb7b3fc
Compare
glefer
force-pushed
the
feat/webhook
branch
from
eb7b3fc to
f4c28c5
Compare
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request introduces webhook support to GitHub Runner Manager.
Key features and changes:
Webhook notifications:
Added support for sending notifications on key runner events (start, stop, error, build, update, etc.).
Supports multiple providers: Slack, Discord, Microsoft Teams, and generic webhooks.
Configurable in runners_config.yaml with options for enabling/disabling, timeout, retry, and per-provider settings.
Events can be filtered per provider.
Configuration examples and documentation:
Updated README.md with detailed instructions and configuration samples for webhooks.
Provided example runners_config.yaml.webhook-example for quick setup.
Added CLI commands to test webhooks without triggering real actions.
Testing and validation:
Includes tests for webhook formatting, sending, and retry logic.
Ensures configuration schema supports new webhook options.