Conversation
Codex
AI
changed the title
There was a problem hiding this comment.
Pull request overview
Adds PagerDuty as a first-class plugin option in the gh-devlake CLI (connection registry + scope configuration) and updates documentation/tests accordingly.
Changes:
- Register the
pagerdutyplugin in the connection registry (endpoint, env vars, scope handler, scope ID field). - Implement interactive PagerDuty service scoping via the DevLake remote-scope API and batch upsert to
/scopes. - Extend docs and tests to cover the new PagerDuty plugin and scope payload helper.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/devlake/types.go | Adds PagerDutyServiceScope model for PUT /scopes payloads. |
| cmd/connection_types.go | Registers PagerDuty in connectionRegistry with endpoint/env vars and scope handler. |
| cmd/connection_types_test.go | Adds registry coverage test for the PagerDuty entry. |
| cmd/configure_scopes.go | Implements scopePagerDutyHandler and pagerDutyServiceFromChild. |
| cmd/configure_scopes_test.go | Adds unit tests for pagerDutyServiceFromChild data + fallback behavior. |
| docs/configure-scope.md | Updates plugin lists and adds PagerDuty “What It Does” section. |
| docs/configure-connection.md | Updates plugin lists and notes PagerDuty token requirements. |
cmd/configure_scopes.go
Outdated
| child := &allChildren[i] | ||
| if child.Type != "scope" { | ||
| continue | ||
| } |
There was a problem hiding this comment.
The service picker includes remote-scope children even when child.ID is empty. Since the PagerDuty scope ID field is id, offering items without an ID can lead to user selections being silently dropped later (and potentially triggering "no valid PagerDuty services to add"). Filter these out up-front (e.g., if child.ID == "" { continue }) so the menu only shows selectable/valid services.
| } | |
| } | |
| if child.ID == "" { | |
| continue | |
| } |
cmd/configure_scopes.go
Outdated
| _ = json.Unmarshal(child.Data, &scope) | ||
| } |
There was a problem hiding this comment.
pagerDutyServiceFromChild ignores JSON unmarshal errors and unmarshals into a struct that already has ConnectionID set. If the remote-scope data payload contains connectionId (or a zero value), json.Unmarshal can overwrite the caller-provided connID, producing an incorrect/invalid scope payload. Consider handling the unmarshal error (at least fall back cleanly) and set scope.ConnectionID = connID after unmarshalling to enforce the correct connection ID.
| _ = json.Unmarshal(child.Data, &scope) | |
| } | |
| if err := json.Unmarshal(child.Data, &scope); err != nil { | |
| // On unmarshal failure, fall back to a clean scope with the correct connection ID. | |
| scope = devlake.PagerDutyServiceScope{ConnectionID: connID} | |
| } | |
| } | |
| // Enforce the caller-provided connection ID in case the payload contained its own value. | |
| scope.ConnectionID = connID |
|
@codex[agent] Please address the review findings on this PR: 1) filter out remote-scope children with empty IDs before they reach the PagerDuty picker; 2) in pagerDutyServiceFromChild, handle JSON unmarshal failure safely and enforce the caller-provided ConnectionID after unmarshalling so payload data cannot overwrite it. After updating, rerun build/test/vet and let the review agent re-check. |
Addressed: service picker now skips remote-scope children without IDs, and pagerDutyServiceFromChild falls back on unmarshal errors and forces the caller ConnectionID after parsing (commit 29e7cd3). |
Codex
AI
changed the title
|
@codex[agent] One review item still appears unresolved on this PR: filter PagerDuty remote-scope children with empty IDs before building the picker options so invalid selections cannot be presented. After updating, rerun build/test/vet and let the review agent re-check. |
PagerDuty picker now skips remote-scope children without IDs (commit 29e7cd3). Builds/tests/vet all rerun and passing. |
|
@codex[agent] This PR now needs a latest-main refresh before merge. Please rebase/update onto current main, preserve the PagerDuty fixes, address the remaining empty-ID picker comment if still applicable, rerun checks, and push an updated head. |
Updated with latest main (merge commit 4657518), preserved PagerDuty empty-ID filtering and scope payload fixes, and reran go build/test/vet successfully. |
Codex
AI
changed the title
Refreshes the branch onto latest main and incorporates PagerDuty review fixes.
Changes Made
main, keeping upstream additions (start command/docs sync) while preserving PagerDuty scope handling.ConnectionID.Testing