Conversation
Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds ArgoCD as a supported DevLake plugin in this GitHub CLI extension, enabling users to configure ArgoCD connections and select application scopes for GitOps/DORA-related collection.
Changes:
- Introduces
ArgoCDAppScopepayload type for batch scope upserts. - Registers the
argocdplugin inconnectionRegistryand adds an interactive scope selection handler. - Adds registry tests and documents ArgoCD in the supported plugins table.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/devlake/types.go | Adds ArgoCDAppScope for PUT /scopes payloads. |
| cmd/connection_types.go | Registers argocd plugin metadata and wires scopeArgoCDHandler. |
| cmd/configure_scopes.go | Implements ArgoCD remote-scope pagination, selection, and scope upsert. |
| cmd/connection_types_test.go | Adds TestConnectionRegistry_ArgoCD for registry field coverage. |
| README.md | Documents ArgoCD as a supported plugin. |
Comments suppressed due to low confidence (1)
cmd/configure_scopes.go:1343
ScopeNameis set tochild.Name, but earlier you already fall back tochild.IDwhenchild.Nameis empty for display. Ifchild.Nameis missing, this will create blueprint scopes with an emptyscopeName, which makes later output/UI harder to interpret. Consider falling back tochild.ID(or the selected label) whenchild.Nameis empty.
blueprintScopes = append(blueprintScopes, devlake.BlueprintScope{
ScopeID: child.ID,
ScopeName: child.Name,
})
cmd/configure_scopes.go
Outdated
| label := child.Name | ||
| if label == "" { | ||
| label = child.ID | ||
| } | ||
| appOptions = append(appOptions, label) | ||
| appMap[label] = child | ||
| } |
There was a problem hiding this comment.
appMap is keyed by the user-facing label (child.Name fallback to child.ID). If multiple ArgoCD applications share the same Name (or if Name is not guaranteed unique), later entries overwrite earlier ones and the selected label can resolve to the wrong scope. Consider making the option label unambiguous (e.g., include the ID in the label like the SonarQube handler does) or keep a label→ID map and key the child map by ID.
This issue also appears on line 1340 of the same file.
cmd/connection_types_test.go
Outdated
| }{ | ||
| {"Plugin", def.Plugin, "argocd"}, | ||
| {"DisplayName", def.DisplayName, "ArgoCD"}, | ||
| {"Available", def.Available, true}, | ||
| {"Endpoint", def.Endpoint, ""}, | ||
| {"SupportsTest", def.SupportsTest, true}, | ||
| {"AuthMethod", def.AuthMethod, "AccessToken"}, | ||
| {"ScopeIDField", def.ScopeIDField, "name"}, | ||
| {"HasRepoScopes", def.HasRepoScopes, false}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| if tt.got != tt.want { | ||
| t.Errorf("%s: got %v, want %v", tt.name, tt.got, tt.want) | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| if def.ScopeFunc == nil { | ||
| t.Error("ScopeFunc should not be nil") | ||
| } | ||
|
|
||
| // ArgoCD uses auth tokens, not OAuth/PAT scopes | ||
| if len(def.RequiredScopes) != 0 { | ||
| t.Errorf("RequiredScopes should be empty for ArgoCD auth tokens, got %v", def.RequiredScopes) | ||
| } | ||
| if def.ScopeHint != "" { | ||
| t.Errorf("ScopeHint should be empty for ArgoCD auth tokens, got %q", def.ScopeHint) | ||
| } | ||
|
|
||
| expectedEnvVars := []string{"ARGOCD_TOKEN", "ARGOCD_AUTH_TOKEN"} | ||
| if len(def.EnvVarNames) != len(expectedEnvVars) { | ||
| t.Errorf("EnvVarNames length: got %d, want %d", len(def.EnvVarNames), len(expectedEnvVars)) | ||
| } else { | ||
| for i, v := range expectedEnvVars { | ||
| if def.EnvVarNames[i] != v { | ||
| t.Errorf("EnvVarNames[%d]: got %q, want %q", i, def.EnvVarNames[i], v) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| expectedEnvFileKeys := []string{"ARGOCD_TOKEN", "ARGOCD_AUTH_TOKEN"} | ||
| if len(def.EnvFileKeys) != len(expectedEnvFileKeys) { | ||
| t.Errorf("EnvFileKeys length: got %d, want %d", len(def.EnvFileKeys), len(expectedEnvFileKeys)) | ||
| } else { | ||
| for i, v := range expectedEnvFileKeys { | ||
| if def.EnvFileKeys[i] != v { | ||
| t.Errorf("EnvFileKeys[%d]: got %q, want %q", i, def.EnvFileKeys[i], v) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This new test block is not gofmt-formatted (indentation/tabs), unlike the rest of the file. Please run gofmt on this file (or at least this new function) to keep formatting consistent and avoid noisy diffs later.
| }{ | |
| {"Plugin", def.Plugin, "argocd"}, | |
| {"DisplayName", def.DisplayName, "ArgoCD"}, | |
| {"Available", def.Available, true}, | |
| {"Endpoint", def.Endpoint, ""}, | |
| {"SupportsTest", def.SupportsTest, true}, | |
| {"AuthMethod", def.AuthMethod, "AccessToken"}, | |
| {"ScopeIDField", def.ScopeIDField, "name"}, | |
| {"HasRepoScopes", def.HasRepoScopes, false}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| if tt.got != tt.want { | |
| t.Errorf("%s: got %v, want %v", tt.name, tt.got, tt.want) | |
| } | |
| }) | |
| } | |
| if def.ScopeFunc == nil { | |
| t.Error("ScopeFunc should not be nil") | |
| } | |
| // ArgoCD uses auth tokens, not OAuth/PAT scopes | |
| if len(def.RequiredScopes) != 0 { | |
| t.Errorf("RequiredScopes should be empty for ArgoCD auth tokens, got %v", def.RequiredScopes) | |
| } | |
| if def.ScopeHint != "" { | |
| t.Errorf("ScopeHint should be empty for ArgoCD auth tokens, got %q", def.ScopeHint) | |
| } | |
| expectedEnvVars := []string{"ARGOCD_TOKEN", "ARGOCD_AUTH_TOKEN"} | |
| if len(def.EnvVarNames) != len(expectedEnvVars) { | |
| t.Errorf("EnvVarNames length: got %d, want %d", len(def.EnvVarNames), len(expectedEnvVars)) | |
| } else { | |
| for i, v := range expectedEnvVars { | |
| if def.EnvVarNames[i] != v { | |
| t.Errorf("EnvVarNames[%d]: got %q, want %q", i, def.EnvVarNames[i], v) | |
| } | |
| } | |
| } | |
| expectedEnvFileKeys := []string{"ARGOCD_TOKEN", "ARGOCD_AUTH_TOKEN"} | |
| if len(def.EnvFileKeys) != len(expectedEnvFileKeys) { | |
| t.Errorf("EnvFileKeys length: got %d, want %d", len(def.EnvFileKeys), len(expectedEnvFileKeys)) | |
| } else { | |
| for i, v := range expectedEnvFileKeys { | |
| if def.EnvFileKeys[i] != v { | |
| t.Errorf("EnvFileKeys[%d]: got %q, want %q", i, def.EnvFileKeys[i], v) | |
| } | |
| } | |
| } | |
| } | |
| }{ | |
| {"Plugin", def.Plugin, "argocd"}, | |
| {"DisplayName", def.DisplayName, "ArgoCD"}, | |
| {"Available", def.Available, true}, | |
| {"Endpoint", def.Endpoint, ""}, | |
| {"SupportsTest", def.SupportsTest, true}, | |
| {"AuthMethod", def.AuthMethod, "AccessToken"}, | |
| {"ScopeIDField", def.ScopeIDField, "name"}, | |
| {"HasRepoScopes", def.HasRepoScopes, false}, | |
| } | |
| for _, tt := range tests { | |
| t.Run(tt.name, func(t *testing.T) { | |
| if tt.got != tt.want { | |
| t.Errorf("%s: got %v, want %v", tt.name, tt.got, tt.want) | |
| } | |
| }) | |
| } | |
| if def.ScopeFunc == nil { | |
| t.Error("ScopeFunc should not be nil") | |
| } | |
| // ArgoCD uses auth tokens, not OAuth/PAT scopes | |
| if len(def.RequiredScopes) != 0 { | |
| t.Errorf("RequiredScopes should be empty for ArgoCD auth tokens, got %v", def.RequiredScopes) | |
| } | |
| if def.ScopeHint != "" { | |
| t.Errorf("ScopeHint should be empty for ArgoCD auth tokens, got %q", def.ScopeHint) | |
| } | |
| expectedEnvVars := []string{"ARGOCD_TOKEN", "ARGOCD_AUTH_TOKEN"} | |
| if len(def.EnvVarNames) != len(expectedEnvVars) { | |
| t.Errorf("EnvVarNames length: got %d, want %d", len(def.EnvVarNames), len(expectedEnvVars)) | |
| } else { | |
| for i, v := range expectedEnvVars { | |
| if def.EnvVarNames[i] != v { | |
| t.Errorf("EnvVarNames[%d]: got %q, want %q", i, def.EnvVarNames[i], v) | |
| } | |
| } | |
| } | |
| expectedEnvFileKeys := []string{"ARGOCD_TOKEN", "ARGOCD_AUTH_TOKEN"} | |
| if len(def.EnvFileKeys) != len(expectedEnvFileKeys) { | |
| t.Errorf("EnvFileKeys length: got %d, want %d", len(def.EnvFileKeys), len(expectedEnvFileKeys)) | |
| } else { | |
| for i, v := range expectedEnvFileKeys { | |
| if def.EnvFileKeys[i] != v { | |
| t.Errorf("EnvFileKeys[%d]: got %q, want %q", i, def.EnvFileKeys[i], v) | |
| } | |
| } | |
| } | |
| } |
|
@copilot Please address the review findings on this PR: 1) make ArgoCD app selection labels/mapping unambiguous so duplicate names cannot overwrite each other or resolve to the wrong scope; 2) run gofmt on the new ArgoCD registry test block in cmd/connection_types_test.go. After updating, rerun build/test/vet and let the review agent re-check. |
Co-authored-by: ewega <26189114+ewega@users.noreply.github.com>
Both issues addressed in bce5d6f:
|
ewega
left a comment
There was a problem hiding this comment.
Reviewed after follow-up fixes; no remaining merge blockers found.
Adds ArgoCD as a supported DevLake plugin, enabling GitOps deployment frequency and change tracking data for DORA metrics.
Changes
internal/devlake/types.go— NewArgoCDAppScopestruct (connectionId+name) matching the ArgoCD plugin's scope ID conventioncmd/connection_types.go—argocdentry inconnectionRegistry:Available: true,AccessTokenauth,ScopeIDField: "name", no default endpoint (user-supplied), env keysARGOCD_TOKEN/ARGOCD_AUTH_TOKENcmd/configure_scopes.go—scopeArgoCDHandler: paginatesListRemoteScopes, presents interactive multi-select over returned applications, PUTs selections asArgoCDAppScopescopescmd/connection_types_test.go—TestConnectionRegistry_ArgoCDcovering all registry fieldsREADME.md— ArgoCD row added to supported plugins tableUsage
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.