artin/sourcegraph
1
0
Fork 0
mirror of https://github.com/sourcegraph/sourcegraph.git synced 2026-02-06 15:12:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

gitserver: Remove IsCloneable check from clone path (#63028)

Browse Source 
We made this preflight check always, and the doc string said that it's for "better error messages".

I checked on that, and the additional RPS claim and the additional latency of the iscloneable check plus load on the code host don't seem justified.

Error message before this change:

```
Error updating repo:
failed to clone dev.azure.com/sourcegraph-source/src-cli/src-cli: error cloning repo: repo dev.azure.com/sourcegraph-source/src-cli/src-cli not cloneable: failed to check remote access: fatal: Authentication failed for 'https://dev.azure.com/sourcegraph-source/src-cli/_git/src-cli/': exit status 128
```

Error message after this change:

```
Error updating repo:
failed to clone dev.azure.com/sourcegraph-source/src-cli/src-cli: clone failed. Output: Creating bare repo Created bare repo at /Users/erik/.sourcegraph/repos_1/.tmp/clone-2786287217/.git Fetching remote contents fatal: Authentication failed for 'https://dev.azure.com/sourcegraph-source/src-cli/_git/src-cli/': failed to fetch: exit status 128: command failed: exit status 128
```

Not much worse or less readable.

Closes https://github.com/sourcegraph/sourcegraph/issues/62786

Test plan:

Adjusted tests, made sure repos still clone alright and throw a somewhat useful error message on failure.
This commit is contained in:
Erik Seliger 2024-06-03 16:37:11 +02:00 committed by GitHub
parent d5c9c3a3c1
commit a18904b6fc
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 6 additions and 91 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

47
cmd/gitserver/internal/integration_tests/clone_test.go
View File

@ -188,59 +188,20 @@ func TestClone_Fail(t *testing.T) {
Hostname: "test-shard",
})
// Requesting a repo update should figure out that the repo is not yet
// cloned and call clone. We expect that clone to fail, because vcssyncer.IsCloneable
// fails here.
// Requesting a repo update should figure out that the repo is not yet cloned and call clone.
// We expect that clone to fail.
_, _, err := s.FetchRepository(ctx, repo)
require.Error(t, err)
// Note that this error is from IsCloneable(), not from Clone().
require.Contains(t, err.Error(), "error cloning repo: repo github.com/test/repo not cloneable:")
require.Contains(t, err.Error(), "exit status 128")
mockassert.CalledOnce(t, locker.TryAcquireFunc)
mockassert.CalledOnce(t, lock.ReleaseFunc)
// Check we reported an error.
// Check that it was called exactly once total.
mockrequire.CalledOnce(t, gsStore.SetLastErrorFunc)
// And that it was called for the right repo, setting the last error value.
mockassert.CalledWith(t, gsStore.SetLastErrorFunc, mockassert.Values(mockassert.Skip, repo, mockassert.Skip, "test-shard"))
require.Contains(t, gsStore.SetLastErrorFunc.History()[0].Arg2, `error cloning repo: repo github.com/test/repo not cloneable:`)
require.Contains(t, gsStore.SetLastErrorFunc.History()[0].Arg2, "exit status 128")
// And no other DB activity has happened.
mockassert.NotCalled(t, gsStore.SetCloneStatusFunc)
mockassert.NotCalled(t, gsStore.SetLastOutputFunc)
// ===================
// Now, fake that the IsCloneable check passes, then Clone will be called
// and is expected to fail.
vcssyncer.TestGitRepoExists = func(ctx context.Context, name api.RepoName) error {
return nil
}
t.Cleanup(func() {
vcssyncer.TestGitRepoExists = nil
})
// Reset mock counters.
gsStore = dbmocks.NewMockGitserverRepoStore()
db.GitserverReposFunc.SetDefaultReturn(gsStore)
// Requesting another repo update should figure out that the repo is not yet
// cloned and call clone. We expect that clone to fail, but in the vcssyncer.Clone
// stage this time, not vcssyncer.IsCloneable.
_, _, err = s.FetchRepository(ctx, repo)
require.Error(t, err)
require.Contains(t, err.Error(), "failed to clone github.com/test/repo: clone failed. Output: Creating bare repo\nCreated bare repo at")
// Should have acquired a lock.
mockassert.CalledN(t, locker.TryAcquireFunc, 2)
mockassert.CalledN(t, locker.TryAcquireFunc, 1)
// Should have reported status. 7 lines is the output git currently produces.
// This number might need to be adjusted over time, but before doing so please
// check that the calls actually use the args you would expect them to use.
mockassert.CalledN(t, lock.SetStatusFunc, 7)
// Should have released the lock.
mockassert.CalledN(t, lock.ReleaseFunc, 2)
mockassert.CalledN(t, lock.ReleaseFunc, 1)
// Check it was set to cloning first, then uncloned again (since clone failed).
mockassert.CalledN(t, gsStore.SetCloneStatusFunc, 2)

22
cmd/gitserver/internal/server.go
View File

@ -392,27 +392,9 @@ func (s *Server) cloneRepo(ctx context.Context, repo api.RepoName, lock Reposito
logger := s.logger.Scoped("cloneRepo").With(log.String("repo", string(repo)))
syncer, err := func() (_ vcssyncer.VCSSyncer, err error) {
syncer, err := s.getVCSSyncer(ctx, repo)
if err != nil {
return nil, errors.Wrap(err, "get VCS syncer")
}
if err = s.rpsLimiter.Wait(ctx); err != nil {
return nil, err
}
if err := syncer.IsCloneable(ctx, repo); err != nil {
return nil, errors.Wrapf(err, "error cloning repo: repo %s not cloneable", repo)
}
return syncer, nil
}()
syncer, err := s.getVCSSyncer(ctx, repo)
if err != nil {
if ctx.Err() != nil {
return ctx.Err()
}
return err
return errors.Wrap(err, "get VCS syncer")
}
dir := s.fs.RepoDir(repo)

20
cmd/gitserver/internal/server_test.go
View File

@ -185,15 +185,6 @@ func TestExecRequest(t *testing.T) {
return false, nil
})
vcssyncer.TestGitRepoExists = func(ctx context.Context, repoName api.RepoName) error {
if strings.Contains(string(repoName), "nicksnyder/go-i18n") {
return nil
}
return errors.New("not cloneable")
}
t.Cleanup(func() { vcssyncer.TestGitRepoExists = nil })
for _, test := range tests {
t.Run(test.Name, func(t *testing.T) {
ss := gitserver.NewMockGitserverService_ExecServer()
@ -447,17 +438,6 @@ func TestCloneRepoRecordsFailures(t *testing.T) {
getVCSSyncer func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error)
wantErr string
}{
{
name: "Not cloneable",
getVCSSyncer: func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) {
m := vcssyncer.NewMockVCSSyncer()
m.IsCloneableFunc.SetDefaultHook(func(context.Context, api.RepoName) error {
return errors.New("not_cloneable")
})
return m, nil
},
wantErr: "failed to clone example.com/foo/bar: error cloning repo: repo example.com/foo/bar not cloneable: not_cloneable",
},
{
name: "Failing clone",
getVCSSyncer: func(ctx context.Context, name api.RepoName) (vcssyncer.VCSSyncer, error) {

8
cmd/gitserver/internal/vcssyncer/git.go
View File

@ -42,20 +42,12 @@ func (s *gitRepoSyncer) Type() string {
return "git"
}
// TestGitRepoExists is a test fixture that overrides the return value for
// GitRepoSyncer.IsCloneable when it is set.
var TestGitRepoExists func(ctx context.Context, repoName api.RepoName) error
// IsCloneable checks to see if the Git remote URL is cloneable.
func (s *gitRepoSyncer) IsCloneable(ctx context.Context, repoName api.RepoName) (err error) {
if isAlwaysCloningTest(repoName) {
return nil
}
if TestGitRepoExists != nil {
return TestGitRepoExists(ctx, repoName)
}
source, err := s.getRemoteURLSource(ctx, repoName)
if err != nil {
return errors.Wrapf(err, "failed to get remote URL source for %s", repoName)