[WIP] refactor(resolution): replace go-scm with internal scmclient#10208
Draft
ary82 wants to merge 2 commits into
Draft
[WIP] refactor(resolution): replace go-scm with internal scmclient#10208ary82 wants to merge 2 commits into
ary82 wants to merge 2 commits into
Conversation
tekton-robot
added
release-note-none
tekton-robot
added
the
size/XXL
ary82
force-pushed
the
cleanup/resolution-replace-go-scm-lib-9989
branch
from
351a9f9 to
f6f9cff
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the git resolver’s API-mode SCM integration to stop depending on github.com/jenkins-x/go-scm by introducing a new internal pkg/internal/scmclient package that implements a lightweight SCMClient interface (GetFileContent / GetCommitSHA / GetCloneURL) and wiring the resolver(s) to use it.
Changes:
- Added
pkg/internal/scmclientwith HTTP-based clients (GitHub/GitLab/Gitea) behind a small interface plus a fake for unit tests. - Updated both in-tree git resolvers to construct SCM clients via
scmclient.Newinstead ofgo-scm/factory.NewClient. - Expanded/adjusted e2e tests to validate API mode for additional providers (GitHub/GitLab) and refactored Gitea API-mode tests.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
test/resolvers_git_test.go |
E2E updates: adds GitHub/GitLab API-mode tests and refactors Gitea API-mode tests/config handling. |
pkg/resolution/resolver/git/resolver.go |
Switches API-mode resolution to use pkg/internal/scmclient instead of go-scm. |
pkg/resolution/resolver/git/resolver_test.go |
Updates unit tests to mock via scmclient.FakeSCMClient and adjusts expected errors. |
pkg/remoteresolution/resolver/git/resolver.go |
Switches remoteresolution git resolver to use pkg/internal/scmclient. |
pkg/remoteresolution/resolver/git/resolver_test.go |
Updates remoteresolution unit tests to mock via scmclient.FakeSCMClient. |
pkg/internal/scmclient/client.go |
Adds SCMClient interface and provider factory. |
pkg/internal/scmclient/http.go |
Adds a small HTTP helper for request/response handling. |
pkg/internal/scmclient/github.go / github_test.go |
Adds GitHub REST client + unit tests. |
pkg/internal/scmclient/gitlab.go / gitlab_test.go |
Adds GitLab REST client + unit tests. |
pkg/internal/scmclient/gitea.go / gitea_test.go |
Adds Gitea REST client + unit tests. |
pkg/internal/scmclient/fake.go |
Adds a fake implementation for resolver unit tests. |
pkg/internal/scmclient/bitbucketcloud.go |
Placeholder Bitbucket Cloud client (currently panics). |
pkg/internal/scmclient/bitbucketserver.go |
Placeholder Bitbucket Server client (currently panics). |
pkg/internal/scmclient/azure.go |
Placeholder Azure DevOps client (currently panics). |
pkg/internal/scmclient/client_test.go |
Tests factory behavior for supported provider keys. |
Comments suppressed due to low confidence (1)
test/resolvers_git_test.go:558
- This test verification request places the auth token in the URL query (
...&token=...) and useshttp.Getwithout a context. Tokens in query strings are easily leaked via logs/proxies, and omitting context can cause the call to hang beyond the test deadline. Prefer sending the token viaAuthorizationheader and usinghttp.NewRequestWithContext.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+40
to
+44
| case "bitbucketcloud": | ||
| return newBitbucketCloudClient(serverURL, token), nil | ||
| case "bitbucketserver": | ||
| return newBitbucketServerClient(serverURL, token), nil | ||
| case "azure": |
| } | ||
|
|
||
| func (c *githubClient) GetFileContent(ctx context.Context, org, repo, path, ref string) ([]byte, error) { | ||
| url := fmt.Sprintf("%s/repos/%s/%s/contents/%s?ref=%s", c.baseURL, org, repo, url.PathEscape(path), ref) |
Author
There was a problem hiding this comment.
Github does handle path param that is PathEscaped. I can confirm this works.
Fixed for ref QueryEscape.
cURL request for PathEscaped content path
╰─➤ curl -L \ 3s
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer $GITHUB_TOKEN" \
-H "X-GitHub-Api-Version: 2026-03-10" \
https://api.github.com/repos/tektoncd/pipeline/contents/test%2Fgit-resolver%2Fremote-task.yaml
{
"name": "remote-task.yaml",
"path": "test/git-resolver/remote-task.yaml",
"sha": "95f0e8d4b27050ef7826701f22274145c63b9f5e",
"size": 203,
"url": "https://api.github.com/repos/tektoncd/pipeline/contents/test/git-resolver/remote-task.yaml?ref=main",
"html_url": "https://github.com/tektoncd/pipeline/blob/main/test/git-resolver/remote-task.yaml",
"git_url": "https://api.github.com/repos/tektoncd/pipeline/git/blobs/95f0e8d4b27050ef7826701f22274145c63b9f5e",
"download_url": "https://raw.githubusercontent.com/tektoncd/pipeline/main/test/git-resolver/remote-task.yaml",
"type": "file",
"content": "YXBpVmVyc2lvbjogdGVrdG9uLmRldi92MWJldGExCmtpbmQ6IFRhc2sKbWV0\nYWRhdGE6CiAgbmFtZTogcmVtb3RlLXNjbS1yZXNvbHZlci10YXNrCnNwZWM6\nCiAgc3RlcHM6CiAgLSBuYW1lOiBzbGVlcAogICAgaW1hZ2U6IG1pcnJvci5n\nY3IuaW8vYnVzeWJveAogICAgY29tbWFuZDogWycvYmluL3NoJ10KICAgIGFy\nZ3M6IFsnLWMnLCAnc2xlZXAgMTAnXQo=\n",
"encoding": "base64",
"_links": {
"self": "https://api.github.com/repos/tektoncd/pipeline/contents/test/git-resolver/remote-task.yaml?ref=main",
"git": "https://api.github.com/repos/tektoncd/pipeline/git/blobs/95f0e8d4b27050ef7826701f22274145c63b9f5e",
"html": "https://github.com/tektoncd/pipeline/blob/main/test/git-resolver/remote-task.yaml"
}
}
Comment on lines
+45
to
+50
| func (f *FakeSCMClient) GetCommitSHA(_ context.Context, org, repo, ref string) (string, error) { | ||
| if f.Err != nil { | ||
| return "", f.Err | ||
| } | ||
| return f.CommitSHAs[org+"/"+repo+"/"+ref], nil | ||
| } |
Comment on lines
+52
to
+57
| func (f *FakeSCMClient) GetCloneURL(_ context.Context, org, repo string) (string, error) { | ||
| if f.Err != nil { | ||
| return "", f.Err | ||
| } | ||
| return f.CloneURLs[org+"/"+repo], nil | ||
| } |
ary82
force-pushed
the
cleanup/resolution-replace-go-scm-lib-9989
branch
from
f6f9cff to
8f43882
Compare
Collaborator
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
The git resolver's API depended on `github.com/jenkins-x/go-scm` for fetching file content from SCM providers, causing a heavy dependency and ongoing maintenance pain. This change introduces the `scmclient` internal package, which defines lightweight implementations of the SCM providers according to the `SCMClient` interface. It has three methods (GetFileContent, GetCommitSHA, GetCloneURL) Signed-off-by: Aryan Goyal <mail@ary82.dev>
ary82
force-pushed
the
cleanup/resolution-replace-go-scm-lib-9989
branch
from
8f43882 to
3e7d091
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 19 out of 19 changed files in this pull request and generated 15 comments.
Comments suppressed due to low confidence (1)
test/resolvers_git_test.go:619
- The verification request puts the Gitea token into the query string and uses http.Get without context. Tokens in URLs can leak via logs/proxies, and the request won’t be canceled if the test context is canceled. Prefer an Authorization header with a context-aware request, and ensure the response body is closed via defer.
Comment on lines
344
to
355
| // find the actual git commit sha by the ref | ||
| commit, _, err := scmClient.Git.FindCommit(ctx, orgRepo, ref) | ||
| if err != nil || commit == nil { | ||
| sha, err := client.GetCommitSHA(ctx, org, repo, ref) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("couldn't fetch the commit sha for the ref %s in the repo: %w", ref, err) | ||
| } | ||
|
|
||
| // fetch the repository URL | ||
| repo, _, err := scmClient.Repositories.Find(ctx, orgRepo) | ||
| // fetch the repository clone URL | ||
| cloneURL, err := client.GetCloneURL(ctx, org, repo) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("couldn't fetch repository: %w", err) | ||
| } | ||
|
|
Comment on lines
+19
to
+23
| import ( | ||
| "context" | ||
| "net/http" | ||
| "strings" | ||
| ) |
Comment on lines
+45
to
+47
| func (c *azureClient) GetFileContent(ctx context.Context, org, repo, path, ref string) ([]byte, error) { | ||
| panic("unimplemented") | ||
| } |
Comment on lines
+49
to
+51
| func (c *azureClient) GetCommitSHA(ctx context.Context, org, repo, ref string) (string, error) { | ||
| panic("unimplemented") | ||
| } |
Comment on lines
+53
to
+55
| func (c *azureClient) GetCloneURL(ctx context.Context, org, repo string) (string, error) { | ||
| panic("unimplemented") | ||
| } |
Comment on lines
+33
to
+48
| switch scmType { | ||
| case "github": | ||
| return newGitHubClient(serverURL, token), nil | ||
| case "gitlab": | ||
| return newGitLabClient(serverURL, token), nil | ||
| case "gitea": | ||
| return newGiteaClient(serverURL, token), nil | ||
| case "bitbucketcloud": | ||
| return newBitbucketCloudClient(serverURL, token), nil | ||
| case "bitbucketserver": | ||
| return newBitbucketServerClient(serverURL, token), nil | ||
| case "azure": | ||
| return newAzureClient(serverURL, token), nil | ||
| default: | ||
| return nil, fmt.Errorf("unsupported scm type %q", scmType) | ||
| } |
ary82
force-pushed
the
cleanup/resolution-replace-go-scm-lib-9989
branch
from
0548234 to
e9cc80c
Compare
Signed-off-by: Aryan Goyal <mail@ary82.dev>
ary82
force-pushed
the
cleanup/resolution-replace-go-scm-lib-9989
branch
from
e9cc80c to
77a75d5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #9989
Changes
The git resolver's API depended on
github.com/jenkins-x/go-scmfor fetching file content from SCM providers, causing a heavy dependency and ongoing maintenance pain.This change introduces the
scmclientinternal package, which defines lightweight implementations of the SCM providers according to theSCMClientinterface. It has three methods (GetFileContent, GetCommitSHA, GetCloneURL)/kind cleanup
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes