fix(ROSAENG-435): support PagerDuty incident lookup for HCP clusters - #962
fix(ROSAENG-435): support PagerDuty incident lookup for HCP clusters#962fullsend-ai-coder[bot] wants to merge 4 commits into
Conversation
HCP clusters use region-based PD services instead of per-cluster services keyed by DNS base domain. For classic clusters, PD services are named after the cluster's DNS domain, so querying by baseDomain works. For HCP, the PD service corresponds to the AWS region (e.g. us-east-1), and incidents for multiple clusters share the same service. Changes: - Add WithClusterID() to the PD client builder so callers can set a cluster ID for incident filtering - Modify GetFiringAlertsForCluster() to include first_trigger_log_entries when a cluster ID is set, and filter incidents by matching the cluster ID in EventDetails - Add incidentMatchesCluster() helper that checks cluster_id, clusterID, and cluster-id keys in EventDetails - In cmd/cluster/context.go: override baseDomain with the region ID for HCP clusters, and pass externalClusterID (the external UUID) to WithClusterID — PD alerts reference the external UUID, not the internal OCM ID - In cmd/org/context.go: update NewPDClient signature to accept clusterID, detect HCP clusters and pass cluster.ExternalID() The critical fix is using ExternalID() rather than ID() when filtering incidents. PagerDuty alerts contain the cluster's external UUID in their EventDetails, so passing the internal OCM ID would cause incidentMatchesCluster to always return false, filtering out all incidents. Related to ROSAENG-435
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
🤖 Review · Commit: |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fullsend-ai-coder[bot] The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @fullsend-ai-coder[bot]. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
🤖 Finished Review · ✅ Success · Started 3:30 PM UTC · Completed 3:48 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $7.76 |
|
Risk Assessment: low (1/5) DetailsLow-risk change: moderate line count but well-scoped to 5 files with good test coverage (40% test files), no protected paths or security-sensitive files touched, no dependency or CI changes, bot author with prior contributions, and changed files show low recent churn. Previous runRisk Assessment: moderate (2/5) DetailsSmall-to-medium change (5 files, 404 lines) adding HCP PagerDuty incident lookup support by a bot author. No protected paths, security-sensitive files, CI, or dependency changes. Good test coverage ratio (0.40). cmd/cluster/context.go has notable regression history but no recent churn. Overall risk remains moderate, consistent with prior assessment. Previous run (2)Risk Assessment: moderate (2/5) DetailsSmall focused change (5 files, 296 lines) adding HCP PagerDuty incident lookup support by a bot author. No protected paths, no security-sensitive files, no CI or dependency changes. Good test coverage ratio (0.40). Most files stable for over a year but change scope is well-contained. |
ReviewFindingsMedium
Low
Next steps:
Previous runReviewFindingsMedium
Low
Next steps:
Previous run (2)ReviewFindingsMedium
Low
Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 3:49 PM UTC · Completed 4:05 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.01 |
- Add cluster-ID filtering to GetHistoricalAlertsForCluster for HCP clusters, matching the existing filtering in GetFiringAlertsForCluster. Requests first_trigger_log_entries and filters by incidentMatchesCluster when clusterID is set. - Fix pre-existing pagination bug: reset incidentListOffset to 0 at the start of each outer loop iteration over pdServiceIDs in GetFiringAlertsForCluster. - Make HCP detection in generateContextData() consistent with setup() by adding Region() != nil && Region().ID() != "" guards. Addresses #962
🔧 Fix agent — iteration 1 (bot-triggered)Addressed 3 of 5 review findings with code fixes: added cluster-ID filtering to GetHistoricalAlertsForCluster for HCP clusters, fixed the pre-existing pagination offset bug in GetFiringAlertsForCluster, and made the HCP detection guard consistent between setup() and generateContextData(). Disagreed with 2 findings (scope-coherence and api-design-coherence) as out-of-scope refactoring suggestions. All 29 PagerDuty tests pass (2 new). All changed packages compile and pass go vet. Fixed (3):
Disagreed (2):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 4:06 PM UTC · Completed 4:25 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.97 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Fix · ✅ Success · Started 4:26 PM UTC · Completed 4:36 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $1.97 |
- Fix pre-existing bug in GetHistoricalAlertsForCluster where the incidents slice was not reset between service ID iterations, causing inflated IncidentOccurrenceTracker counts for subsequent services - Add debug logging when incidents are filtered out by incidentMatchesCluster to aid HCP incident visibility debugging - Move incidentMatchesCluster tests inside existing Describe block for consistency with test file organization Addresses #962
🔧 Fix agent — iteration 2 (bot-triggered)Fixed incidents slice accumulation bug in GetHistoricalAlertsForCluster, added debug logging for filtered incidents, and reorganized incidentMatchesCluster tests into the existing test structure. All 27 pagerduty tests and the org context test pass. Fixed (3):
Disagreed (1):
Tests: passed Decision points
Next steps:
|
|
🤖 Finished Review · ✅ Success · Started 4:38 PM UTC · Completed 5:20 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
fullsend-ai-review
Bot
added
risk/low
| continue | ||
| } | ||
| incidents[pdServiceID] = append(incidents[pdServiceID], incident) | ||
| } |
There was a problem hiding this comment.
[medium] error handling / output corruption
The fmt.Printf("Skipping incident ...") calls in GetFiringAlertsForCluster (line 143) and GetHistoricalAlertsForCluster (line 210) write to stdout. Both osdctl cluster context -o json and osdctl org context -o json also write structured JSON to stdout. When HCP filtering occurs, these debug messages corrupt the JSON output for downstream consumers (e.g., jq, scripts). This is also inconsistent with the codebase logging pattern — the pagerduty package uses fmt.Printf only for genuine error conditions.
Suggested fix: Replace fmt.Printf with fmt.Fprintf(os.Stderr, ...) or remove the debug logging entirely, since the filtering behavior is expected and validated by tests.
| @@ -111,6 +111,29 @@ func TestFetchContext_NoSubscriptions(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[low] test-inadequate
TestNewPDClient_PassesClusterID only verifies the function signature accepts two parameters. While pagerduty_test.go has thorough unit tests for incidentMatchesCluster and HCP filtering, there is no integration-level test verifying that FetchContext correctly detects HCP clusters and passes the region as baseDomain and externalID as clusterID.
Suggested fix: Add an integration test for the FetchContext HCP detection path.
| // given cluster by inspecting the first trigger log entry's EventDetails for | ||
| // a matching cluster_id value. This is used for HCP clusters where PD services | ||
| // are region-based and contain incidents for multiple clusters. | ||
| func incidentMatchesCluster(incident pd.Incident, clusterID string) bool { |
There was a problem hiding this comment.
[low] edge-case
When EventDetails is nil in incidentMatchesCluster, the incident is silently filtered out. If PagerDuty doesn't populate EventDetails for certain incident types (e.g., incidents created via the UI rather than an integration), legitimate HCP incidents could be silently dropped.
| o.clusterID = o.cluster.ID() | ||
| o.externalClusterID = o.cluster.ExternalID() | ||
| o.baseDomain = o.cluster.DNS().BaseDomain() | ||
| // HCP clusters use region-based PD services rather than per-cluster |
There was a problem hiding this comment.
[low] pattern-violation
The HCP detection guard appears identically in two places within the same file (setup() and generateContextData()) and once in cmd/org/context.go. Computing the HCP check once in setup() and storing the result on contextOptions would reduce the maintenance surface.
Suggested fix: Compute the HCP check once in setup() and store on contextOptions.
| ctrl.Finish() | ||
| }) | ||
|
|
||
| Context("WithClusterID", func() { |
There was a problem hiding this comment.
[low] code-organization
The WithClusterID test is placed in the Provider Functionality block alongside mock-backed integration tests. The existing file structure separates builder/setter tests under Client Creation.
Suggested fix: Move the WithClusterID test under the Client Creation Describe block.
| @@ -41,7 +41,7 @@ type DefaultContextFetcher struct { | |||
| GetLimitedSupport func(*sdk.Connection, string) ([]*cmv1.LimitedSupportReason, error) | |||
There was a problem hiding this comment.
[low] design-direction
The NewPDClient signature change from one to two parameters is consistent with project patterns. If more parameters are needed in the future, consider refactoring to an options struct.
|
🤖 Finished Fix · ✅ Success · Started 5:21 PM UTC · Completed 5:43 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.73 |
- Remove fmt.Printf stdout debug logging that corrupts JSON output when HCP filtering skips incidents; add stderr warning only for nil EventDetails edge case (visibility for silent drops) - Deduplicate HCP region guard: compute once in setup() and store as isHCPRegionBased on contextOptions - Move WithClusterID test to "Client Creation" describe block Addresses #962
🔧 Fix agent — iteration 3 (bot-triggered)Addressed 4 of 8 review findings with code changes: removed stdout-corrupting debug logging, added stderr warning for nil EventDetails edge case, deduplicated HCP guard in cmd/cluster/context.go, and moved WithClusterID test to correct describe block. Recorded reasoned disagreements for the remaining 4 findings (integration test scope, PR title convention, scope coherence observation, future architectural suggestion). Fixed (4):
Disagreed (4):
Tests: passed Decision points
Next steps:
|
|
🤖 Review · Started 5:44 PM UTC Commit: |
Summary
osdctl cluster contextandosdctl org contextcommandsEventDetailsin the first trigger log entry, ensuring only incidents for the target cluster are shownWhat changed
pkg/provider/pagerduty/pagerduty.goclusterIDfield andWithClusterID()builder method to the PD clientGetFiringAlertsForCluster()to requestfirst_trigger_log_entriesfrom PD and filter incidents by cluster ID when setincidentMatchesCluster()helper that checkscluster_id,clusterID, andcluster-idkeys in EventDetailscmd/cluster/context.gobaseDomainwith the region ID for PD service lookupexternalClusterIDtoWithClusterID()(the external UUID that PD alerts reference)cmd/org/context.goNewPDClientfunction signature to acceptclusterIDparametercluster.ExternalID()for incident filteringTesting
incidentMatchesCluster, 1 forWithClusterID, 4 for HCP filtering inGetFiringAlertsForCluster)cmd/org/context_test.goverifyingNewPDClientpasses both parametersgo vetandgo buildpass on all changed packagesRelated to https://redhat.atlassian.net/browse/ROSAENG-435
Post-script verification
agent/ROSAENG-435-hcp-pagerduty-incidents)c02ab22b7faf938a24ff7f1a084ae3c650f07759..HEAD)