feat: add SolarWinds Orion provider with pull (SWIS API) and webhook support#6235
feat: add SolarWinds Orion provider with pull (SWIS API) and webhook support#6235alexanderxfgl-bit wants to merge 9 commits intokeephq:mainfrom
Conversation
dosubot
bot
added
the
size:XL
|
|
|
Key advantages of this implementation over existing PRs:
Implementation details:
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 5 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 005a6a8. Configure here.
tests/test_solarwinds_provider.py
Outdated
| "LastNote": "", | ||
| "AlertDefName": "High CPU Load", | ||
| "NodeCaption": "server1", | ||
| "IP_Address": "10.0.0.1", |
There was a problem hiding this comment.
Test mock uses wrong key for IP address
Medium Severity
The test mock data uses key IP_Address but the SWQL query aliases it as NodeIP (n.IP_Address AS NodeIP), and the production code reads row.get("NodeIP"). Since the mock response contains IP_Address instead of NodeIP, the ip_address field will be None, causing the assertion alerts[0].ip_address == "10.0.0.1" to fail.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 005a6a8. Configure here.
tests/test_solarwinds_provider.py
Outdated
| cm = ContextManager(tenant_id="test", workflow_id="test") | ||
| provider = SolarwindsProvider(cm, "test-solarwinds", config) | ||
| with pytest.raises(ValueError, match="requires either"): | ||
| provider.validate_config() |
There was a problem hiding this comment.
Test raises ValueError before pytest.raises context block
Medium Severity
In test_validate_config_no_auth_fails, the SolarwindsProvider constructor calls validate_config() via BaseProvider.__init__, so the ValueError is raised at line 238 during construction — before the with pytest.raises block at line 239. This makes the test fail with an uncaught exception rather than testing what it intends.
Reviewed by Cursor Bugbot for commit 005a6a8. Configure here.
| name = alert_name or node_name or "SolarWinds Alert" | ||
|
|
||
| # Build fingerprint-compatible id | ||
| alert_id = event.get("id") or event.get("AlertObjectID") or event.get("alert_id") |
There was a problem hiding this comment.
Alert ID extraction skips case-insensitive matching
Medium Severity
The alert_id extraction uses event.get() directly (case-sensitive), while all other field extractions use the _get() helper for case-insensitive matching. Webhook payloads with differently-cased ID keys (e.g., alertobjectid or AlertObjectId) will not be recognized, causing unnecessary fallback to generated IDs and potential deduplication issues.
Reviewed by Cursor Bugbot for commit 005a6a8. Configure here.
| if key in event: | ||
| return event[key] | ||
| # Try case-insensitive match | ||
| lower_event = {k.lower(): v for k, v in event.items()} |
There was a problem hiding this comment.
Lowercase dict rebuilt per key per _get call
Low Severity
The _get() helper reconstructs lower_event via dict comprehension on every loop iteration for every key checked. Since _get is called 9+ times with up to 5 keys each, this creates up to ~45 redundant dictionary copies. Building the lowercase mapping once before the loop (or once per _format_alert call) would be cleaner and more efficient.
Reviewed by Cursor Bugbot for commit 005a6a8. Configure here.
| } | ||
| alert = SolarwindsProvider._format_alert(event) | ||
| assert alert.lastReceived is not None | ||
| assert "2025" in alert.lastReceived |
There was a problem hiding this comment.
Test asserts wrong year for Unix timestamp value
Low Severity
The Unix timestamp 1705312200 corresponds to approximately January 15, 2024, not 2025. The assertion "2025" in alert.lastReceived will fail because datetime.fromtimestamp(1705312200) produces a 2024 date. Either the timestamp value or the expected year in the assertion is wrong.
Reviewed by Cursor Bugbot for commit 005a6a8. Configure here.
- Fix test assertion: Unix timestamp 1705312200 is 2024 not 2025 - Fix test_validate_config_no_auth_fails: move constructor inside pytest.raises - Add NodeIP to mock data to match SWQL query alias - Use _get() for case-insensitive alert_id extraction - Cache lowercase dict outside _get() loop for efficiency
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
I have read the CLA Document and I hereby sign the CLA |
|
CLA recheck requested. |
- Use case-insensitive lookup for alert_id in pull mode (line 318) - Fix Unix timestamp in test: 1705312200 (2024) -> 1736899200 (2025) - Cache lowercase event dict to avoid rebuilding per _get call
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
- Remove duplicate IP_Address key from test mock data (production code reads NodeIP via SWQL alias, not IP_Address) - Fix incorrect timestamp comment: 1736899200 = Jan 15 2024, not 2025 The other 3 cursor bot findings were false positives: - pytest.raises placement is correct (ValueError fires inside block) - alert_id already uses _get() for case-insensitive matching - lowercase dict is already built once before _get() function
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
…havior
Pydantic v1 stores Enum fields as their string values (e.g., 'firing'
instead of AlertStatus.FIRING). Updated all test assertions to compare
against string values, matching how other Keep provider tests work
(e.g., uptimekuma_provider).
Changes:
- AlertStatus enum comparisons -> string values ('firing', 'resolved', 'acknowledged')
- AlertSeverity enum comparisons -> string values ('critical', 'high', 'warning', 'info')
- Timestamp assertion relaxed to check date/time components (AlertDto normalizes format)
- Fixed comment: 1736899200 = Jan 15 2025 (not 2024)
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
|
I have read the CLA Document and I hereby sign the CLA |
Improve the case-insensitive field lookup helper to use dict.get() instead of 'in' + separate key access, reducing redundant lookups. Add clarifying docstring about _lower_event closure behavior. Addresses BugBot review feedback (cursor/bugbot).
|
You have used all of your free Bugbot PR reviews. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
2 participants
Summary
Adds comprehensive SolarWinds Orion provider to Keep with both pull-based (SWIS REST API) and push-based (webhook) alert ingestion.
Features
Pull-based alerts: Fetches active alerts from SolarWinds Orion via SWIS API using SWQL queries
Orion.AlertActiveview for active alertsPush-based alerts (webhook): Receives alerts via webhooks
Comprehensive configuration:
Full test coverage:
Related Issue
Closes #3526
/claim #3526
Note
Medium Risk
Adds a new external-integration provider with credential handling and outbound HTTP calls to the SolarWinds SWIS API, which could affect alert ingestion correctness and operational reliability if mappings or error handling are off.
Overview
Introduces a new SolarWinds Orion provider (
keep/providers/solarwinds_provider) that supports both pull-based alert fetching via SWIS/SWQL and push-based ingestion via webhooks, including severity/status mapping, timestamp parsing, and optional basic-auth vs bearer-token authentication.Adds provider documentation (
README.md) and a comprehensive test suite covering webhook payload normalization and SWIS polling behavior, including auth validation and error cases.Reviewed by Cursor Bugbot for commit 005a6a8. Bugbot is set up for automated code reviews on this repo. Configure here.