[ROSAENG-62213] pkg/pagerduty: refactor and fix cluster ID extraction - #892
Conversation
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughPagerDuty cluster ID extraction now checks direct alert details, notes YAML, and ChangesPagerDuty cluster ID extraction
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 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: 2
🧹 Nitpick comments (2)
pkg/pagerduty/pagerduty_test.go (1)
344-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for the new firing-text path; consider adding an "all fields empty" case.
These two cases solidly cover single-line and multi-line
firingextraction. One gap: there's no test wheredetailsexists but none ofcluster_id,notes, orfiringare populated — the path that exercises thelastErrfallthrough across all three extractors inextractClusterIDFromAlertBody. Adding it would guard the loop's error-selection behavior (see the related comment onpkg/pagerduty/pagerduty.golines 422-441) against regressions.🤖 Prompt for 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. In `@pkg/pagerduty/pagerduty_test.go` around lines 344 - 370, Add a test alongside the existing RetrieveClusterID cases for an alert whose details object exists but has empty cluster_id, notes, and firing fields. Assert that RetrieveClusterID returns an error and no cluster ID, exercising the lastErr fallthrough across all extractors in extractClusterIDFromAlertBody.pkg/pagerduty/pagerduty.go (1)
422-441: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
lastErrcan mask the real root cause.Since
lastErris overwritten on every iteration regardless of which extractor produced it, the error ultimately returned is always from whichever extractor ran last (parseClusterIdFromFiring) if none succeed — even when an earlier extractor hit a more specific and actionable error (e.g., malformednotesYAML fromparseClusterIdFromNotes). A caller/log consumer debugging a failed extraction would see"could not find firing field"or"no cluster_id found in firing field"instead of the actual YAML decode error, hiding the true cause.♻️ Proposed fix: aggregate/preserve all errors
type extractor = func(map[string]interface{}) (string, error) extractors := []extractor{parseClusterIdFromField, parseClusterIdFromNotes, parseClusterIdFromFiring} - var lastErr error - for _, extractor := range extractors { - id, err := extractor(details) + var errs []error + for _, ext := range extractors { + id, err := ext(details) if err != nil { - lastErr = err + errs = append(errs, err) } if id != "" { return id, nil } } - return "", lastErr + return "", errors.Join(errs...)🤖 Prompt for 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. In `@pkg/pagerduty/pagerduty.go` around lines 422 - 441, Update extractClusterIDFromAlertBody so errors from all failed extractors are preserved instead of returning only the final extractor’s error. Aggregate the errors from parseClusterIdFromField, parseClusterIdFromNotes, and parseClusterIdFromFiring, then return the combined error when no extractor returns an ID while preserving the existing successful extraction behavior.
🤖 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 `@pkg/pagerduty/pagerduty.go`:
- Around line 424-426: Update the not-found error in extractAlertDetails to stop
embedding the full data alert body in the error message. Return an error
containing only a safe, relevant field identifier or a generic description,
while preserving the existing error path and context.
- Around line 414-420: Update clusterIDFromFiringRe to require a whitespace or
start-of-field boundary before cluster_id, preventing matches within names such
as hosted_cluster_id while still extracting standalone cluster_id values from
multi-line firing text.
---
Nitpick comments:
In `@pkg/pagerduty/pagerduty_test.go`:
- Around line 344-370: Add a test alongside the existing RetrieveClusterID cases
for an alert whose details object exists but has empty cluster_id, notes, and
firing fields. Assert that RetrieveClusterID returns an error and no cluster ID,
exercising the lastErr fallthrough across all extractors in
extractClusterIDFromAlertBody.
In `@pkg/pagerduty/pagerduty.go`:
- Around line 422-441: Update extractClusterIDFromAlertBody so errors from all
failed extractors are preserved instead of returning only the final extractor’s
error. Aggregate the errors from parseClusterIdFromField,
parseClusterIdFromNotes, and parseClusterIdFromFiring, then return the combined
error when no extractor returns an ID while preserving the existing successful
extraction behavior.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1fad5080-205a-4f94-973a-5e7e8b51d07d
📒 Files selected for processing (2)
pkg/pagerduty/pagerduty.gopkg/pagerduty/pagerduty_test.go
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #892 +/- ##
==========================================
+ Coverage 44.94% 45.08% +0.13%
==========================================
Files 73 73
Lines 8753 8775 +22
==========================================
+ Hits 3934 3956 +22
Misses 4594 4594
Partials 225 225
🚀 New features to boost your workflow:
|
3fd1500 to
bf12470
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 `@pkg/pagerduty/pagerduty_test.go`:
- Line 348: Handle the ignored fmt.Fprint errors in all three new mock handlers
in pkg/pagerduty/pagerduty_test.go at lines 348-348, 362-362, and 375-375: check
each fixture write result and fail the corresponding test or handler
consistently when writing fails, rather than discarding the error.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b1f776d-e78d-4ef6-ade6-aa5f89194c6d
📒 Files selected for processing (2)
pkg/pagerduty/pagerduty.gopkg/pagerduty/pagerduty_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/pagerduty/pagerduty.go
Add dedicated extractor functions, and add a third parse path that extracts cluster_id from the free-text 'firing' field to handle HCPNodepoolUpgradeDelay alerts that set neither 'cluster_id' nor 'notes'.
bf12470 to
b3d7863
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bergmannf, gvnnn The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@bergmannf: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Add dedicated extractor functions, and add a third parse path that extracts cluster_id from the free-text 'firing' field to handle HCPNodepoolUpgradeDelay alerts that set neither 'cluster_id' nor 'notes'.
What type of PR is this?
bug
What this PR does / Why we need it?
Special notes for your reviewer
Test Coverage
Guidelines for CAD investigations
Test coverage checks
Pre-checks (if applicable)
Summary by CodeRabbit
Bug Fixes
firingtext support and safer parsing to avoid false matches.Tests
hosted_cluster_idis not mistaken forcluster_id, and adjusted missing-field expectations.