diff --git a/backend/plugins/azuredevops_go/api/remote_helper.go b/backend/plugins/azuredevops_go/api/remote_helper.go index 6da68905e3c..4b9ea78e21c 100644 --- a/backend/plugins/azuredevops_go/api/remote_helper.go +++ b/backend/plugins/azuredevops_go/api/remote_helper.go @@ -82,12 +82,16 @@ func listAzuredevopsRemoteScopes( } if projectId != "" { - if remote, err := listRemoteRepos(vsc, orgId, projectId); err == nil { + if remote, listErr := listRemoteRepos(vsc, orgId, projectId); listErr == nil { children = append(children, remote...) + } else { + basicRes.GetLogger().Warn(listErr, "failed to list the external repositories of project %s", projectId) } - if remote, err := listAzuredevopsRepos(vsc, orgId, projectId); err == nil { + if remote, listErr := listAzuredevopsRepos(vsc, orgId, projectId); listErr == nil { children = append(children, remote...) + } else { + basicRes.GetLogger().Warn(listErr, "failed to list the Azure DevOps repositories of project %s", projectId) } } return children, nextPage, nil @@ -235,19 +239,25 @@ func listRemoteRepos( Provider: v.Type, ServiceEndpoint: v.Id, } + endpointName := v.Name g.Go(func() error { repos, err := vsc.GetRemoteRepositories(remoteRepoArgs) + if err != nil { + // A single service connection that cannot be queried (e.g. expired credentials) must not + // hide the repositories of all the other service connections, so skip it and keep going. + basicRes.GetLogger().Warn(err, "failed to list the repositories of service connection %q (%s), skipping it", endpointName, remoteRepoArgs.Provider) + return nil + } mu.Lock() remoteRepos = append(remoteRepos, repos...) mu.Unlock() - return err + return nil }) } - if err := g.Wait(); err != nil { - return nil, errors.Internal.Wrap(err, "failed to call 'GetRemoteRepositories', falling back to empty list") - } + // The goroutines never return an error, failures are logged and skipped above. + _ = g.Wait() for _, v := range remoteRepos { pID := orgId + idSeparator + projectId diff --git a/backend/plugins/azuredevops_go/api/remote_helper_test.go b/backend/plugins/azuredevops_go/api/remote_helper_test.go new file mode 100644 index 00000000000..3385b1b39c3 --- /dev/null +++ b/backend/plugins/azuredevops_go/api/remote_helper_test.go @@ -0,0 +1,94 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package api + +import ( + "io" + "net/http" + "net/url" + "strings" + "testing" + + "github.com/apache/devlake/core/errors" + "github.com/apache/devlake/core/plugin" + mockcontext "github.com/apache/devlake/mocks/core/context" + mocklog "github.com/apache/devlake/mocks/core/log" + "github.com/apache/devlake/plugins/azuredevops_go/api/azuredevops" + "github.com/apache/devlake/plugins/azuredevops_go/models" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" +) + +// fakeApiClient answers GET requests from a fixed map of "path?serviceEndpointId" to response bodies. +// A missing entry is returned as a request error. +type fakeApiClient struct { + plugin.ApiClient + responses map[string]string +} + +func (f *fakeApiClient) Get(path string, query url.Values, _ http.Header) (*http.Response, errors.Error) { + key := path + if id := query.Get("serviceEndpointId"); id != "" { + key += "?" + id + } + body, ok := f.responses[key] + if !ok { + return nil, errors.Unauthorized.New("request failed: " + key) + } + req, _ := http.NewRequest(http.MethodGet, "https://dev.azure.com/"+path, nil) + return &http.Response{ + StatusCode: http.StatusOK, + Header: http.Header{}, + Body: io.NopCloser(strings.NewReader(body)), + Request: req, + }, nil +} + +func TestListRemoteReposSkipsFailingServiceConnection(t *testing.T) { + // the failing service connection must be logged once and then skipped + logger := new(mocklog.Logger) + logger.On("Warn", mock.Anything, mock.Anything, mock.Anything).Once() + res := new(mockcontext.BasicRes) + res.On("GetLogger").Return(logger) + previous := basicRes + basicRes = res + defer func() { basicRes = previous }() + + client := &fakeApiClient{responses: map[string]string{ + "org/project/_apis/serviceendpoint/endpoints/": `{"value": [ + {"id": "broken-endpoint", "name": "broken", "type": "bitbucket"}, + {"id": "working-endpoint", "name": "working", "type": "github"} + ]}`, + "org/project/_apis/sourceProviders/github/repositories/?working-endpoint": `{"repositories": [ + {"id": "octo/repo", "sourceProviderName": "GitHub", "name": "repo", "fullName": "octo/repo", + "properties": {"connectedServiceId": "working-endpoint", "externalId": "123"}} + ]}`, + // no entry for the broken endpoint: its request fails + }} + vsc := azuredevops.NewClient(&models.AzuredevopsConnection{}, client, "") + + children, err := listRemoteRepos(vsc, "org", "project") + + assert.Nil(t, err) + if assert.Len(t, children, 1) { + assert.Equal(t, "octo/repo", children[0].FullName) + assert.Equal(t, "working-endpoint-123", children[0].Data.Id) + assert.Equal(t, "org/project", *children[0].ParentId) + } + logger.AssertExpectations(t) +}