Generic scope ID extraction and remote-scope API client#115
Conversation
- Add ScopeIDField and HasRepoScopes to ConnectionDef; set on github/gh-copilot entries - Replace ScopeListEntry typed struct with json.RawMessage + extractScopeID helper - Add RemoteScopeChild/RemoteScopeResponse types and ListRemoteScopes/SearchRemoteScopes client methods - Remove hardcoded if-plugin-github checks from listConnectionScopes, scope list, scope delete - Add table-driven tests for ExtractScopeID, ScopeListWrapper helpers, and new client methods Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR generalizes DevLake scope handling so scope ID resolution and scope-name display work across plugins without hardcoded "github" checks, and it adds typed client support for DevLake’s remote-scope APIs.
Changes:
- Add
ScopeIDFieldandHasRepoScopestoConnectionDefand populate them for existing plugins. - Replace typed scope parsing with
json.RawMessage(RawScope) plus helpers (ScopeName,ScopeFullName,ExtractScopeID) to preserve plugin-specific payloads. - Add
ListRemoteScopes/SearchRemoteScopesclient methods and new tests for the helpers and client calls.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
cmd/connection_types.go |
Extends ConnectionDef with ScopeIDField and HasRepoScopes, sets values for GitHub and Copilot. |
cmd/configure_projects.go |
Removes hardcoded GitHub scope-ID/repo logic and uses generic extraction + helpers. |
cmd/configure_scope_list.go |
Uses generic scope ID extraction and wrapper helpers for display/JSON output. |
cmd/configure_scope_delete.go |
Uses generic scope ID extraction and wrapper helpers when building interactive deletion choices. |
internal/devlake/types.go |
Switches scope wrapper to RawScope, adds name/full-name helpers, adds ExtractScopeID, and defines remote-scope response types. |
internal/devlake/client.go |
Adds remote-scope API client methods using the existing doGet[T] pattern. |
internal/devlake/client_test.go |
Updates scope list test to use the new wrapper helpers. |
internal/devlake/types_test.go |
Adds table-driven tests for extraction/helpers and remote-scope client methods. |
internal/devlake/types.go
Outdated
| for _, key := range []string{"fullName", "name"} { | ||
| if v, ok := m[key]; ok { | ||
| var s string | ||
| if err := json.Unmarshal(v, &s); err == nil { |
There was a problem hiding this comment.
ScopeName() returns the first decoded string value even if it’s empty. If the JSON contains "fullName": "" alongside a non-empty "name", this will return "" and never fall back to name. Consider treating empty strings as missing (like ExtractScopeID does) so ScopeName() will continue to the next key when the decoded value is empty.
| if err := json.Unmarshal(v, &s); err == nil { | |
| if err := json.Unmarshal(v, &s); err == nil && s != "" { |
cmd/configure_projects.go
Outdated
| scopeName := w.ScopeFullName() | ||
| if scopeName == "" { | ||
| scopeName = s.Name | ||
| scopeName = w.ScopeName() | ||
| } | ||
| if scopeID == "" { |
There was a problem hiding this comment.
In this loop, ScopeFullName() is called multiple times (scopeName := w.ScopeFullName() and later fullName := w.ScopeFullName()). Since these helpers unmarshal the raw JSON each call, this does redundant work. Consider calling ScopeFullName() once per scope (or just use ScopeName() for the display name since it already prefers fullName) and reuse the value for repo tracking.
| expectedPath := "/plugins/gitlab/connections/2/search-remote-scopes" | ||
| if r.URL.Path != expectedPath { | ||
| t.Errorf("path = %s, want %s", r.URL.Path, expectedPath) | ||
| } | ||
| if tt.search != "" && r.URL.Query().Get("search") != tt.search { | ||
| t.Errorf("search = %q, want %q", r.URL.Query().Get("search"), tt.search) | ||
| } | ||
| w.WriteHeader(tt.statusCode) |
There was a problem hiding this comment.
TestSearchRemoteScopes sets page and pageSize in the test cases, but the handler assertions never verify that the client actually sends page/pageSize query parameters. This can let regressions slip through where pagination params are omitted or misnamed; consider adding assertions for page and pageSize when non-zero (and optionally asserting they’re absent when zero).
|
@copilot Please fix these two review comments:
if err := json.Unmarshal(v, &s); err == nil && s != "" {
|
… call - ScopeName() now skips empty strings (like ExtractScopeID does), so a JSON field of "" correctly falls through to the next candidate key - configure_projects.go: call ScopeFullName() once per scope and reuse the value for both the display name and repo-tracking check - Add "empty fullName falls through to name" test case Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Fixed in 5ec9da9:
|
cmd/configure_scope_list.go
Outdated
| for i, s := range resp.Scopes { | ||
| scopeID := s.Scope.ID | ||
| if scopeID == "" { | ||
| scopeID = strconv.Itoa(s.Scope.GithubID) | ||
| var scopeID string | ||
| if def != nil && def.ScopeIDField != "" { | ||
| scopeID = devlake.ExtractScopeID(s.RawScope, def.ScopeIDField) | ||
| } |
There was a problem hiding this comment.
If ExtractScopeID returns "" (e.g., unexpected payload / missing scope ID field), scopeID stays empty and the list output (and JSON) will emit blank IDs. Consider falling back to s.ScopeFullName() or s.ScopeName() when scopeID == "" so the command remains usable even when ID extraction fails.
| for _, s := range resp.Scopes { | ||
| id := s.Scope.ID | ||
| if id == "" { | ||
| id = strconv.Itoa(s.Scope.GithubID) | ||
| var id string | ||
| if def != nil && def.ScopeIDField != "" { | ||
| id = devlake.ExtractScopeID(s.RawScope, def.ScopeIDField) | ||
| } |
There was a problem hiding this comment.
In interactive delete, if ExtractScopeID yields an empty string, the label becomes [] name and selectedScopeID will remain empty, causing invalid scope selection. Add a fallback (e.g., use s.ScopeFullName()/s.ScopeName() when id == "") so scope deletion still works when the plugin payload doesn’t include the expected ID field.
See below for a potential fix:
name := s.ScopeFullName()
if name == "" {
name = s.ScopeName()
}
var id string
if def != nil && def.ScopeIDField != "" {
id = devlake.ExtractScopeID(s.RawScope, def.ScopeIDField)
}
if id == "" {
id = name
| // ScopeName returns the display name from the raw scope JSON (checks "fullName" then "name"). | ||
| func (w *ScopeListWrapper) ScopeName() string { | ||
| var m map[string]json.RawMessage | ||
| if err := json.Unmarshal(w.RawScope, &m); err != nil { | ||
| return "" | ||
| } | ||
| for _, key := range []string{"fullName", "name"} { | ||
| if v, ok := m[key]; ok { |
There was a problem hiding this comment.
ScopeName() and ScopeFullName() each json.Unmarshal the same RawScope into a map. Callers often invoke both per item (e.g., scope list), doubling parse work. Consider a shared private helper (parse-once) or caching the decoded map/fields on ScopeListWrapper to avoid repeated unmarshalling in loops.
| expectedPath := "/plugins/gitlab/connections/2/search-remote-scopes" | ||
| if r.URL.Path != expectedPath { | ||
| t.Errorf("path = %s, want %s", r.URL.Path, expectedPath) | ||
| } | ||
| if tt.search != "" && r.URL.Query().Get("search") != tt.search { | ||
| t.Errorf("search = %q, want %q", r.URL.Query().Get("search"), tt.search) | ||
| } | ||
| w.WriteHeader(tt.statusCode) |
There was a problem hiding this comment.
TestSearchRemoteScopes passes non-zero page/pageSize but never asserts that the request includes page and pageSize query params (or that search is URL-encoded). Add assertions on r.URL.Query() for these fields so the test actually covers the pagination/query-building logic in SearchRemoteScopes.
|
@copilot Please address these code review comments:
|
…ation test assertions - ScopeListWrapper gains a shared parseScope() helper with lazy map caching; ScopeName() and ScopeFullName() now unmarshal RawScope at most once per item - ScopeFullName() now treats "" as absent (consistent with ScopeName/ExtractScopeID) - configure_scope_list: deduplicate FindConnectionDef; add scopeIDFor() closure that falls back to ScopeName() when ExtractScopeID returns "" - configure_scope_delete: add id=="" fallback to name so interactive delete always has a usable identifier even when ScopeIDField payload is missing - TestSearchRemoteScopes: assert page, pageSize, and search query params are sent when non-zero and absent when zero Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Fixed in 61e4270:
|
| var s string | ||
| if err := json.Unmarshal(v, &s); err == nil && s != "" { | ||
| return s | ||
| } | ||
| var n int64 | ||
| if err := json.Unmarshal(v, &n); err == nil && n != 0 { | ||
| return strconv.FormatInt(n, 10) | ||
| } |
There was a problem hiding this comment.
ExtractScopeID() only attempts to decode numeric IDs as int64. This will fail for plugins that return unsigned IDs (e.g., uint64) or numbers larger than MaxInt64, causing scope ID extraction to silently return "". Consider unmarshaling into json.Number (via a Decoder with UseNumber) or trying uint64 in addition to int64 so all numeric ID fields are supported.
…nDef and bring in scope extraction changes Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
ScopeIDFieldandHasRepoScopesfields toConnectionDef; set values ongithub/gh-copilotentriesScopeListEntrytyped struct withjson.RawMessage+ helpersRemoteScopeChild/RemoteScopeResponsetypesListRemoteScopes/SearchRemoteScopesclient methodslistConnectionScopes()to use generic extraction; deduplicateScopeFullName()callGithubIDfallback from scope list / scope deleteScopeName()to skip empty stringsScopeFullName()to skip empty strings (consistent withScopeName/ExtractScopeID)ScopeListWrapper(sharedparseScope()lazy helper, parse-once per item)ScopeName()whenExtractScopeIDreturns""in scope list and scope deletepage/pageSize/searchquery-param assertions inTestSearchRemoteScopesgo build ./...,go test ./...,go vet ./...passOriginal prompt
This section details on the original issue you should resolve
<issue_title>Generic scope ID extraction and remote-scope API client</issue_title>
<issue_description>## Problem
Hardcoded scope ID resolution
listConnectionScopes()incmd/configure_projects.go:240contains:go if c.plugin == "github" && s.GithubID > 0 { scopeID = fmt.Sprintf("%d", s.GithubID) }And line 251:
go if c.plugin == "github" && s.FullName != "" { repos = append(repos, s.FullName) }These are hardcoded plugin names outside the registry. Every upstream plugin uses a different scope ID field:
githubgithubIdgh-copilotidgitlabgitlabIdazuredevops_goazuredevopsIdjiraboardIdjenkinsfullNamebitbucketbitbucketIdLimited ScopeListEntry struct
ScopeListEntryintypes.go:56-61only capturesGithubID(int) andID(string). When GitLab scopes returngitlabId, it's silently dropped.No remote-scope API client
DevLake provides a standardized remote-scope API across all plugins:
GET /plugins/{plugin}/connections/{id}/remote-scopes?groupId=...&pageToken=...GET /plugins/{plugin}/connections/{id}/search-remote-scopes?search=...&page=...&pageSize=...Currently scope handlers call external APIs directly (e.g.,
gh.ListRepos). New plugins should use the DevLake backend's remote-scope API instead — it handles authentication, pagination, and normalizes the response format across all plugins.Proposed Solution
1. Add
ScopeIDFieldandHasRepoScopestoConnectionDefgo type ConnectionDef struct { // ... existing fields ... ScopeIDField string // JSON field name for scope ID (e.g., "githubId", "gitlabId", "id") HasRepoScopes bool // true = scopes have FullName that should be tracked as repos }2. Generic scope ID extraction
Replace
ScopeListWrapper.Scope(typedScopeListEntry) withjson.RawMessageand add a generic extractor:go type ScopeListWrapper struct { Scope json.RawMessagejson:"scope"// parsed fields for display scopeID string scopeName string fullName string }Add helper:
go func extractScopeID(raw json.RawMessage, fieldName string) string { var m map[string]json.RawMessage json.Unmarshal(raw, &m) if v, ok := m[fieldName]; ok { // try string, then number var s string if json.Unmarshal(v, &s) == nil { return s } var n int if json.Unmarshal(v, &n) == nil { return strconv.Itoa(n) } } return "" }3. Remove hardcoded checks in
listConnectionScopes()Replace:
go if c.plugin == "github" && s.GithubID > 0 { ... }With:
go def := FindConnectionDef(c.plugin) scopeID := extractScopeID(w.RawScope, def.ScopeIDField)Replace:
go if c.plugin == "github" && s.FullName != "" { repos = ... }With:
go if def != nil && def.HasRepoScopes && fullName != "" { repos = ... }4. Fix scope list and delete commands
configure_scope_list.go:94-95andconfigure_scope_delete.go:103-104also uses.Scope.GithubIDfallback. Update these to use the same generic extraction.5. Add remote-scope API client methods
go // RemoteScopeChild represents a group or scope from the remote-scope API. type RemoteScopeChild struct { Type stringjson:"type"// "group" or "scope" ID stringjson:"id"ParentID stringjson:"parentId"Name stringjson:"name"FullName stringjson:"fullName"Data json.RawMessagejson:"data"`}
type RemoteScopeResponse struct {
Children []RemoteScopeChild
json:"children"NextPageToken string
json:"nextPageToken"}
func (c *Client) ListRemoteScopes(plugin string, connID int, groupID, pageToken string) (*RemoteScopeResponse, error)
func (c *Client) SearchRemoteScopes(plugin string, connID int, search string, page, pageSize int) (*RemoteScopeResponse, error)
`
Scope of Changes
cmd/connection_types.go— addScopeIDField,HasRepoScopestoConnectionDef; set values on existing github/gh-copilot entriesinternal/devlake/types.go— replaceScopeListEntrywith raw JSON approach; addRemoteScopeResponse,RemoteScopeChildinternal/devlake/client.go— addListRemoteScopes,SearchRemoteScopesmethodscmd/configure_projects.go— refactorlistConnectionScopes()to use generic extractioncmd/configure_scope_list.go— removeGithubIDfallbackcmd/configure_scope_delete.go— removeGithubIDfallbackAcceptance Criteria
listConnectionScopes(), scope list, or scope delete commandsScopeIDFieldonConnectionDefdrives scope ID extraction for all plugins✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.