Conversation
|
@DevSolex Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an Opsgenie alert channel with authenticated creation and resolution requests, event-specific payloads and deduplication aliases, built-in registration using a routing key, and comprehensive tests. ChangesOpsgenie delivery channel
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AlertDispatcher
participant OpsgenieChannel
participant OpsgenieAPI
AlertDispatcher->>OpsgenieChannel: send(AlertEvent)
OpsgenieChannel->>OpsgenieAPI: POST create or close request
OpsgenieAPI-->>OpsgenieChannel: HTTP response
OpsgenieChannel-->>AlertDispatcher: delivery result
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alerts/opsgenie.ts`:
- Around line 107-129: Update postJson to read the Opsgenie response body when
response.ok is false and include its diagnostic message or errors in the thrown
error alongside the HTTP status. Preserve the existing successful-response
behavior and timeout cleanup.
- Around line 151-159: Update the close URL in the alert_resolved branch to
include Opsgenie’s identifierType=alias query parameter while retaining the
encoded alias route segment, and adjust the close endpoint test to expect the
query-param URL.
In `@tests/alerts/builtins.test.ts`:
- Around line 100-104: Extend the existing “each missingTargetError message
matches the historical CLI wording” test in builtins.test.ts to include the
Opsgenie channel’s exact missingTargetError string. Locate the Opsgenie
definition via getAlertChannel("opsgenie") or the related test setup, and assert
the wording requiring --routing-key and referencing the Opsgenie API key.
In `@tests/alerts/opsgenie.test.ts`:
- Around line 426-458: Add a test in the “error handling” suite for
sendOpsgenieAlert that uses fake timers and a pending mockFetch observing
options.signal, advances time by 10 seconds, and verifies the request aborts and
the promise rejects. Restore real timers reliably after the assertion so the
test does not affect other cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1f8a40ea-3ce2-4b06-9c62-585f8aa6270e
📒 Files selected for processing (4)
src/alerts/builtins.tssrc/alerts/opsgenie.tstests/alerts/builtins.test.tstests/alerts/opsgenie.test.ts
📜 Review details
🔇 Additional comments (7)
src/alerts/opsgenie.ts (4)
13-17: LGTM on priority mapping.Straightforward severity → priority mapping with a safe
P3default; matches the tested behavior forcritical,warning, andinfo.
27-37: LGTM!
41-103: LGTM!
133-180: LGTM!src/alerts/builtins.ts (1)
5-5: LGTM!Also applies to: 68-74
tests/alerts/builtins.test.ts (1)
10-10: LGTM!Also applies to: 32-34, 47-55, 70-70
tests/alerts/opsgenie.test.ts (1)
1-425: LGTM!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/alerts/opsgenie.test.ts`:
- Around line 480-498: Update the “aborts the request after the 10-second
timeout” test to wrap its fake-timer setup, request, timer advancement, and
assertion in try/finally, and call vi.useRealTimers() in the finally block so
real timers are restored even when the test fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dd9d2280-57d6-4909-a311-8abd86d280d6
📒 Files selected for processing (3)
src/alerts/opsgenie.tstests/alerts/builtins.test.tstests/alerts/opsgenie.test.ts
📜 Review details
🔇 Additional comments (3)
src/alerts/opsgenie.ts (1)
127-134: LGTM!Also applies to: 159-162
tests/alerts/builtins.test.ts (1)
47-55: LGTM!Also applies to: 100-103, 121-123
tests/alerts/opsgenie.test.ts (1)
275-284: LGTM!Also applies to: 446-456
252091d to
68a6543
Compare
Closes TegoLabs#314 - Add src/alerts/opsgenie.ts with OpsgenieChannel class and sendOpsgenieAlert function following the AlertChannel interface - POST to /v2/alerts with GenieKey authorization header for threshold_crossed, resource_alert, and state_changed events - Map alert_resolved to POST /v2/alerts/{alias}/close endpoint - buildAlias() mirrors pagerduty.ts buildDedupKey() exactly so repeated threshold crossings for the same entry deduplicate - Validate API key before any network call with a clear error message - Use AbortController/timeout pattern consistent with other channels - Register in builtins.ts with targetOption: routingKey - Add 32 tests in tests/alerts/opsgenie.test.ts (TDD, written first) - Update tests/alerts/builtins.test.ts for the new 6-channel count
- Add ?identifierType=alias to the close URL so Opsgenie looks up the alert by alias rather than ID (critical: without this, close silently finds nothing since the default identifierType is 'id') - Surface Opsgenie response body in error messages for faster debugging - Test that identifierType=alias is present in the close URL - Test that error body message is included in the thrown error - Test AbortController timeout fires after 10 seconds - Assert opsgenie missingTargetError wording in builtins test
Ensures vi.useRealTimers() is always called even if the test assertion fails, preventing timer pollution affecting subsequent tests.
690866f to
b20431a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alerts/opsgenie.ts`:
- Around line 107-136: Update postJson to distinguish AbortError caused by the
request timeout from other fetch failures, rethrowing a clear message that
includes the configured timeout duration (10 seconds). Preserve propagation of
non-timeout errors, and update the timeout assertion in the Opsgenie tests to
match the new message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7eaaec91-59ff-48c2-a09b-f3d1400c10ac
📒 Files selected for processing (4)
src/alerts/builtins.tssrc/alerts/opsgenie.tstests/alerts/builtins.test.tstests/alerts/opsgenie.test.ts
📜 Review details
🔇 Additional comments (6)
src/alerts/opsgenie.ts (3)
1-103: LGTM!
140-184: LGTM!
186-189: LGTM!src/alerts/builtins.ts (1)
5-5: LGTM!Also applies to: 68-74
tests/alerts/builtins.test.ts (1)
10-10: LGTM!Also applies to: 32-34, 47-55, 70-70, 100-104, 121-123
tests/alerts/opsgenie.test.ts (1)
1-503: LGTM!
Distinguish AbortError (our own 10-second timeout) from other fetch failures by catching it explicitly and rethrowing with a message that names the configured timeout duration: 'Opsgenie API request timed out after 10 seconds' Other network errors (ECONNREFUSED etc.) are rethrown unchanged. Update the timeout test assertion to match the new message.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/alerts/opsgenie.ts`:
- Around line 150-158: Update the Opsgenie client constructor to store the
trimmed API key after validation, using the same normalized value for the
Authorization header and preserving rejection of empty keys.
In `@tests/alerts/opsgenie.test.ts`:
- Around line 248-261: Remove the initial alert_resolved sendOpsgenieAlert call
and its associated mock reset from the “sets priority P3 for info severity”
test; keep only the resource-event invocation, response setup, and priority
assertion using sendOpsgenieAlert.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9f2c2131-dd7f-404e-abe2-9a8174945629
📒 Files selected for processing (4)
src/alerts/builtins.tssrc/alerts/opsgenie.tstests/alerts/builtins.test.tstests/alerts/opsgenie.test.ts
📜 Review details
🔇 Additional comments (7)
src/alerts/opsgenie.ts (4)
13-17: LGTM!
160-190: LGTM!
107-131: 🩺 Stability & AvailabilityRun this review in a checkout where
src/alerts/opsgenie.tsis available.The repository could not be cloned in this environment, so the code path cannot be inspected or the proposed
clearTimeout/AbortErrorchanges validated here.
27-37: 🩺 Stability & AvailabilityNo change needed.
alert_resolvedevents include thethreshold.configuredLedgersfield, sobuildAliascan reuse the same close-time alias as the create-time threshold alias.src/alerts/builtins.ts (1)
5-5: LGTM!Also applies to: 68-74
tests/alerts/builtins.test.ts (1)
10-10: LGTM!Also applies to: 32-34, 47-55, 70-70, 100-103, 121-123
tests/alerts/opsgenie.test.ts (1)
480-501: LGTM!
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
- Store trimmed API key in constructor so leading/trailing whitespace is not forwarded in the GenieKey Authorization header - Simplify 'sets priority P3 for info severity' test: remove the redundant alert_resolved call and mock reset; test directly with a resource event at info severity
) Small maintainer fixup on top of #550's Opsgenie channel: the timeout error was thrown without a cause chain, tripping preserve-caught-error.
|
Merged manually as d3f16f2 on |
) Small maintainer fixup on top of #550's Opsgenie channel: the timeout error was thrown without a cause chain, tripping preserve-caught-error.
2 participants
Closes #314
Summary
Adds Opsgenie as a sixth built-in alert channel, following the plugin recipe in
docs/adding-an-alert-channel.mdexactly.Changes
Created:
src/alerts/opsgenie.ts—OpsgenieChannelclass +sendOpsgenieAlertfunction implementing theAlertChannelinterfacetests/alerts/opsgenie.test.ts— 32 tests, written before the implementation (TDD)Edited:
src/alerts/builtins.ts— oneregisterAlertChannelcall added foropsgenie, nothing elsetests/alerts/builtins.test.ts— updated hardcoded channel count (5→6), addedopsgenieto thetargetOptiontable, added delegate test, addedvi.mockforopsgenie.jsImplementation Notes
threshold_crossed,resource_alert, andstate_changedevents POST tohttps://api.opsgenie.com/v2/alertswith aGenieKey <apiKey>authorization headeralert_resolvedevents POST to/v2/alerts/{alias}/closeinsteadbuildAlias()mirrorspagerduty.ts'sbuildDedupKey()exactly — same three-branch logic — so repeated threshold crossings for the same entry map to one Opsgenie alert rather than duplicates"Opsgenie API key is required..."error if empty or whitespaceAbortController/10 s timeout pattern as the other built-in channelstargetOption: "routingKey"— users pass their Opsgenie API key via--routing-keyAcceptance Criteria
threshold_crossedevent creates an Opsgenie alert via a mocked fetch, asserting theGenieKeyheader and payload shapealert_resolvedevent calls the close-alert endpoint instead of createTesting
All 32 new tests pass. Full suite clean — no regressions.
Files Not Touched
dispatcher.ts,registry.ts,commands/alerts.ts,db/schema.sql,webhook.ts,slack.ts,discord.ts,telegram.ts,pagerduty.ts— as required by the issue scope.