Conversation
Codex
AI
changed the title
There was a problem hiding this comment.
Pull request overview
Adds first-class Azure DevOps support to the gh-devlake GitHub CLI extension by enabling the upstream azuredevops_go plugin in the connection registry and providing an interactive remote-scope (project → repo) selection flow.
Changes:
- Enable
azuredevops_goinconnectionRegistry, including PAT env var fallback (AZURE_DEVOPS_PAT→AZDO_PAT) and alias support forazure-devops. - Add an Azure DevOps scope handler that pages through remote scopes, lets users select projects/repositories, and PUTs selected scopes to DevLake.
- Update tests and documentation/README to reflect Azure DevOps availability and token key names.
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 |
|---|---|
cmd/connection_types.go |
Enables Azure DevOps connection definition and adds azure-devops → azuredevops_go aliasing in lookup. |
cmd/configure_scopes.go |
Implements Azure DevOps remote-scope browsing and scope creation (projects + repos). |
cmd/connection_types_test.go |
Adds registry entry validation for Azure DevOps and updates token-expiry test plugin slug. |
cmd/configure_connection_add_test.go |
Updates plugin selection test to validate alias resolution. |
internal/token/resolve_test.go |
Adds AZDO_PAT fallback to Azure DevOps token resolution tests. |
docs/token-handling.md |
Documents Azure DevOps/GitLab as supported and lists env key precedence and required access notes. |
README.md |
Marks Azure DevOps as available in supported plugins table and updates required PAT access text. |
Comments suppressed due to low confidence (1)
cmd/connection_types.go:336
- The
azure-devopsalias is only normalized insideFindConnectionDef, but other flows (e.g. scope add) pass the raw plugin slug through to state lookups and DevLake API calls. If a user runs--plugin azure-devops,resolveConnectionIDwill tryListConnections("azure-devops")and fail (DevLake expectsazuredevops_go), and it also won’t match the saved state (which storesdef.Plugin). Consider canonicalizing the plugin slug once (e.g. return the canonical slug alongside the def, or add aNormalizePluginSlughelper used everywhere) and ensure all API/state calls use the canonical slug.
// FindConnectionDef returns the def for the given plugin slug, or nil.
func FindConnectionDef(plugin string) *ConnectionDef {
switch plugin {
case "azure-devops":
plugin = "azuredevops_go"
}
for _, d := range connectionRegistry {
if d.Plugin == plugin {
return d
}
| func TestResolve_AzureDevOps_AZDO_EnvVar(t *testing.T) { | ||
| t.Setenv("AZURE_DEVOPS_PAT", "") | ||
| t.Setenv("AZDO_PAT", "azdo_fallback") | ||
| result, err := Resolve(adoOpts("", "")) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if result.Token != "azdo_fallback" || result.Source != "environment" { | ||
| t.Errorf("got token=%q source=%q, want token=%q source=%q", result.Token, result.Source, "azdo_fallback", "environment") | ||
| } |
There was a problem hiding this comment.
This env-var test calls Resolve(adoOpts("", "")), which makes Resolve check the default .devlake.env before env vars. If a developer has a .devlake.env in the repo (or CWD) with Azure keys, the test can become flaky by resolving from envfile instead of environment. To make the test deterministic, pass a non-existent env file path (or a temp file without those keys) when constructing ResolveOpts for env-var-only tests.
cmd/configure_connection_add_test.go
Outdated
| func TestSelectPlugin_UnavailablePlugin(t *testing.T) { | ||
| _, err := selectPlugin("azure-devops") | ||
| if err == nil { | ||
| t.Fatal("expected error for unavailable plugin, got nil") | ||
| def, err := selectPlugin("azure-devops") | ||
| if err != nil { | ||
| t.Fatalf("expected alias resolution for azure-devops, got error: %v", err) | ||
| } | ||
| if !strings.Contains(err.Error(), "coming soon") { | ||
| t.Errorf("unexpected error message: %v", err) | ||
| if def == nil { | ||
| t.Fatal("expected ConnectionDef, got nil") | ||
| } | ||
| if def.Plugin != "azuredevops_go" { | ||
| t.Errorf("expected plugin %q, got %q", "azuredevops_go", def.Plugin) | ||
| } | ||
| } |
There was a problem hiding this comment.
The test name TestSelectPlugin_UnavailablePlugin no longer matches what the test is asserting (it now verifies alias resolution for azure-devops, not an unavailable plugin error). Renaming it to reflect alias behavior would keep the suite self-describing.
|
@codex[agent] Please fix: Rename |
Renamed the test to cover the azure-devops alias explicitly (commit a789e78). |
Codex
AI
changed the title
| switch plugin { | ||
| case "azure-devops": | ||
| plugin = "azuredevops_go" | ||
| } |
There was a problem hiding this comment.
FindConnectionDef maps the legacy slug "azure-devops" to "azuredevops_go", but other flows still use the user-supplied slug for state lookup and DevLake API calls (e.g., resolveConnectionID compares c.Plugin == plugin and calls client.ListConnections(plugin)). This means --plugin azure-devops can validate successfully but later fail to find/list connections because the backend/state uses the canonical "azuredevops_go" slug. Consider introducing a single slug-canonicalization helper (or alias map on the registry) and ensure callers use def.Plugin (canonical) for subsequent operations, not the raw flag value.
cmd/configure_scopes.go
Outdated
| if _, ok := payload["name"]; !ok || payload["name"] == "" { | ||
| payload["name"] = child.Name | ||
| } | ||
| if _, ok := payload["fullName"]; !ok && child.FullName != "" { |
There was a problem hiding this comment.
azureDevOpsScopePayload only populates fullName when the key is missing, but not when it exists with an empty value. This is inconsistent with the id/name handling (missing OR empty) and can leave fullName blank even though child.FullName is available. It should probably set fullName when the existing value is empty as well.
| if _, ok := payload["fullName"]; !ok && child.FullName != "" { | |
| if _, ok := payload["fullName"]; (!ok || payload["fullName"] == "") && child.FullName != "" { |
| func azureDevOpsScopePayload(child devlake.RemoteScopeChild, connID int) map[string]any { | ||
| var payload map[string]any | ||
| if len(child.Data) > 0 { | ||
| _ = json.Unmarshal(child.Data, &payload) | ||
| } | ||
| if payload == nil { | ||
| payload = make(map[string]any) | ||
| } | ||
| if _, ok := payload["id"]; !ok || payload["id"] == "" { | ||
| payload["id"] = child.ID | ||
| } | ||
| if _, ok := payload["name"]; !ok || payload["name"] == "" { | ||
| payload["name"] = child.Name | ||
| } | ||
| if _, ok := payload["fullName"]; !ok && child.FullName != "" { | ||
| payload["fullName"] = child.FullName | ||
| } | ||
| payload["connectionId"] = connID | ||
| return payload | ||
| } | ||
|
|
||
| func azureScopeLabel(child devlake.RemoteScopeChild) string { | ||
| if child.FullName != "" { | ||
| return child.FullName | ||
| } | ||
| if child.Name != "" { | ||
| return child.Name | ||
| } | ||
| return child.ID | ||
| } |
There was a problem hiding this comment.
The new Azure DevOps scope helper logic (azureScopeLabel, azureDevOpsScopePayload, and pagination in listAzureDevOpsRemoteChildren) isn’t covered by unit tests. Since cmd/configure_scopes.go already has a substantial test suite, adding focused tests for these helpers (e.g., fullName/id/name fallback behavior) would help prevent regressions.
|
@codex[agent] Please fix these two issues:
|
Applied canonical slug usage after plugin selection and updated Azure DevOps fullName fallback (commit 15225ac). |
Codex
AI
changed the title
| @@ -64,7 +65,7 @@ func runDeleteConnection(cmd *cobra.Command, args []string) error { | |||
| } | |||
|
|
|||
| // ── Resolve plugin + ID ── | |||
| plugin := connDeletePlugin | |||
| plugin := canonicalPlugin | |||
| connID := connDeleteID | |||
There was a problem hiding this comment.
In flag mode, plugin is now canonicalized via canonicalPluginSlug, but the state update logic later removes entries only when c.Plugin == plugin. If an existing state file contains the legacy alias (e.g. azure-devops), the DevLake API delete will succeed but the state entry will not be removed, leaving stale local state. Consider canonicalizing both sides when filtering state connections (e.g., compare canonicalPluginSlug(c.Plugin) to plugin).
| @@ -78,7 +79,7 @@ func runUpdateConnection(cmd *cobra.Command, args []string) error { | |||
| var connID int | |||
|
|
|||
| if flagMode { | |||
| plugin = updateConnPlugin | |||
| plugin = canonicalPlugin | |||
| connID = updateConnID | |||
There was a problem hiding this comment.
In flag mode, plugin is now canonicalized (canonicalPluginSlug(updateConnPlugin)), but the state update loop later matches with if c.Plugin == plugin && c.ConnectionID == updated.ID. If a state file contains the legacy alias (e.g. azure-devops), the connection will update successfully in DevLake but the local state file won’t be updated. Consider canonicalizing plugin slugs when matching state entries (e.g., compare canonicalPluginSlug(c.Plugin) to plugin) so alias handling stays consistent for existing state files.
cmd/configure_scopes.go
Outdated
| func azureDevOpsScopePayload(child devlake.RemoteScopeChild, connID int) map[string]any { | ||
| var payload map[string]any | ||
| if len(child.Data) > 0 { | ||
| _ = json.Unmarshal(child.Data, &payload) |
There was a problem hiding this comment.
azureDevOpsScopePayload ignores json.Unmarshal errors. If the remote-scope API ever returns malformed/partial JSON in child.Data, this will silently drop fields and proceed with a potentially invalid/incomplete payload. Consider checking the unmarshal error and returning it (or at least falling back to an empty payload with a warning) so failures are diagnosable and you don’t PUT scopes with unexpected shape.
| _ = json.Unmarshal(child.Data, &payload) | |
| if err := json.Unmarshal(child.Data, &payload); err != nil { | |
| fmt.Printf("\n⚠️ failed to decode Azure DevOps scope data for child %s: %v\n", child.ID, err) | |
| payload = make(map[string]any) | |
| } |
| if opts.Plugin != "" { | ||
| def, err := requirePlugin(opts.Plugin) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| selectedPlugin = opts.Plugin | ||
| selectedDef = def | ||
| selectedPlugin = def.Plugin | ||
| // Warn about flags that don't apply to the selected plugin. |
There was a problem hiding this comment.
runScopeAdd now canonicalizes selectedPlugin to def.Plugin. However, connection auto-detection still relies on resolveConnectionID, which checks state.Connections[i].Plugin == plugin (exact string match). This will break resolving connection IDs from existing state files that stored the legacy alias (e.g. azure-devops) even though the CLI accepts that alias. Consider canonicalizing plugin slugs when comparing against state (e.g., compare canonicalPluginSlug(c.Plugin) to the selected plugin) so alias support works end-to-end.
|
@codex[agent] Please fix these two issues:
|
Canonicalized state comparisons using canonicalPluginSlug and added Azure scope payload unmarshal warning handling (commit 9bb0346). |
|
@codex[agent] Please fix: Your changes to |
Normalized README.md and docs/token-handling.md back to CRLF line endings (commit 2eb15c3). |
Codex
AI
changed the title
| canonical := plugin | ||
| if state != nil { | ||
| for _, c := range state.Connections { | ||
| if c.Plugin == plugin { | ||
| if canonicalPluginSlug(c.Plugin) == canonical { | ||
| return c.ConnectionID, nil | ||
| } | ||
| } | ||
| } | ||
| conns, err := client.ListConnections(plugin) | ||
| conns, err := client.ListConnections(canonical) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("could not list %s connections: %w", plugin, err) |
There was a problem hiding this comment.
resolveConnectionID sets canonical := plugin but then compares canonicalPluginSlug(c.Plugin) against canonical and calls ListConnections(canonical). If plugin is an alias (e.g. azure-devops), this will not match state entries stored under the canonical slug and may call the API with the wrong plugin slug. Canonicalize the input up front (e.g. canonical := canonicalPluginSlug(plugin)) and use that for both the state comparison and the ListConnections call (while keeping user-facing messages based on the original plugin/display name).
ewega
force-pushed
the
codex/add-azure-devops-support
branch
from
2eb15c3 to
f181a2e
Compare
No description provided.