Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: repo url subfolder [IDE-246] #29

Merged
merged 4 commits into from
Apr 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 46 additions & 7 deletions http/http.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,59 @@ type HTTPClient interface {
}

type httpClient struct {
retryCount int
clientFactory func() *http.Client
instrumentor observability.Instrumentor
errorReporter observability.ErrorReporter
logger *zerolog.Logger
}

type OptionFunc func(*httpClient)

func WithRetryCount(retryCount int) OptionFunc {
return func(h *httpClient) {
h.retryCount = retryCount
}
}

func WithInstrumentor(instrumentor observability.Instrumentor) OptionFunc {
return func(h *httpClient) {
h.instrumentor = instrumentor
}
}

func WithErrorReporter(errorReporter observability.ErrorReporter) OptionFunc {
return func(h *httpClient) {
h.errorReporter = errorReporter
}
}

func WithLogger(logger *zerolog.Logger) OptionFunc {
return func(h *httpClient) {
h.logger = logger
}
}

func NewHTTPClient(
logger *zerolog.Logger,
clientFactory func() *http.Client,
instrumentor observability.Instrumentor,
errorReporter observability.ErrorReporter,
options ...OptionFunc,
) HTTPClient {
return &httpClient{clientFactory, instrumentor, errorReporter, logger}
nopLogger := zerolog.Nop()
instrumentor := observability.NewInstrumentor()
errorReporter := observability.NewErrorReporter(&nopLogger)
client := &httpClient{
retryCount: 3,
clientFactory: clientFactory,
instrumentor: instrumentor,
errorReporter: errorReporter,
logger: &nopLogger,
}

for _, option := range options {
option(client)
}

return client
}

var retryErrorCodes = map[int]bool{
Expand All @@ -60,8 +100,7 @@ func (s *httpClient) Do(req *http.Request) (response *http.Response, err error)
span := s.instrumentor.StartSpan(req.Context(), "http.Do")
defer s.instrumentor.Finish(span)

const retryCount = 3
for i := 0; i < retryCount; i++ {
for i := 0; i < s.retryCount; i++ {
requestId := span.GetTraceId()
req.Header.Set("snyk-request-id", requestId)

Expand All @@ -81,7 +120,7 @@ func (s *httpClient) Do(req *http.Request) (response *http.Response, err error)

if retryErrorCodes[response.StatusCode] {
s.logger.Debug().Err(err).Str("method", req.Method).Int("attempts done", i+1).Msg("retrying")
if i < retryCount-1 {
if i < s.retryCount-1 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have incremental backoff here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should but I will argue against doing this now because:

  1. We haven't seen a need for it yet and this is code that was taken from snyk-ls so it has been used to call Deeproxy for a while now
  2. The CLI have said that long term they would like this code to be part of the HTTP client in GAF so I'd rather leave the proper implementation for then
    WDYT?

time.Sleep(5 * time.Second)
continue
}
Expand Down
24 changes: 21 additions & 3 deletions http/http_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,13 @@ func TestSnykCodeBackendService_DoCall_shouldRetry(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://httpstat.us/500", nil)
require.NoError(t, err)

s := codeClientHTTP.NewHTTPClient(newLogger(t), dummyClientFactory, mockInstrumentor, mockErrorReporter)
s := codeClientHTTP.NewHTTPClient(
dummyClientFactory,
codeClientHTTP.WithRetryCount(3),
codeClientHTTP.WithInstrumentor(mockInstrumentor),
codeClientHTTP.WithErrorReporter(mockErrorReporter),
codeClientHTTP.WithLogger(newLogger(t)),
)
res, err := s.Do(req)
assert.NoError(t, err)
assert.NotNil(t, res)
Expand All @@ -101,7 +107,13 @@ func TestSnykCodeBackendService_DoCall_shouldRetryWithARequestBody(t *testing.T)
req, err := http.NewRequest(http.MethodGet, "https://httpstat.us/500", io.NopCloser(strings.NewReader("body")))
require.NoError(t, err)

s := codeClientHTTP.NewHTTPClient(newLogger(t), dummyClientFactory, mockInstrumentor, mockErrorReporter)
s := codeClientHTTP.NewHTTPClient(
dummyClientFactory,
codeClientHTTP.WithRetryCount(3),
codeClientHTTP.WithInstrumentor(mockInstrumentor),
codeClientHTTP.WithErrorReporter(mockErrorReporter),
codeClientHTTP.WithLogger(newLogger(t)),
)
res, err := s.Do(req)
assert.NoError(t, err)
assert.NotNil(t, res)
Expand All @@ -125,7 +137,13 @@ func TestSnykCodeBackendService_doCall_rejected(t *testing.T) {
req, err := http.NewRequest(http.MethodGet, "https://127.0.0.1", nil)
require.NoError(t, err)

s := codeClientHTTP.NewHTTPClient(newLogger(t), dummyClientFactory, mockInstrumentor, mockErrorReporter)
s := codeClientHTTP.NewHTTPClient(
dummyClientFactory,
codeClientHTTP.WithRetryCount(3),
codeClientHTTP.WithInstrumentor(mockInstrumentor),
codeClientHTTP.WithErrorReporter(mockErrorReporter),
codeClientHTTP.WithLogger(newLogger(t)),
)
_, err = s.Do(req)
assert.Error(t, err)
}
Expand Down
3 changes: 2 additions & 1 deletion internal/analysis/analysis_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,13 @@ func TestAnalysis_CreateWorkspace_NotARepository(t *testing.T) {

logger := zerolog.Nop()

repoDir := t.TempDir()
analysisOrchestrator := analysis.NewAnalysisOrchestrator(&logger, mockHTTPClient, mockInstrumentor, mockErrorReporter, mockConfig)
_, err := analysisOrchestrator.CreateWorkspace(
context.Background(),
"4a72d1db-b465-4764-99e1-ecedad03b06a",
"b372d1db-b465-4764-99e1-ecedad03b06a",
"../",
repoDir,
"testBundleHash")
assert.ErrorContains(t, err, "open local repository")
}
Expand Down
17 changes: 11 additions & 6 deletions internal/deepcode/client_pact_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,11 @@ package deepcode_test
import (
"context"
"fmt"
"net/http"
"testing"

"github.com/golang/mock/gomock"
"github.com/pact-foundation/pact-go/dsl"
"github.com/stretchr/testify/assert"
"net/http"
"testing"

confMocks "github.com/snyk/code-client-go/config/mocks"
codeClientHTTP "github.com/snyk/code-client-go/http"
Expand Down Expand Up @@ -222,9 +221,15 @@ func setupPact(t *testing.T) {

instrumentor := testutil.NewTestInstrumentor()
errorReporter := testutil.NewTestErrorReporter()
httpClient := codeClientHTTP.NewHTTPClient(newLogger(t), func() *http.Client {
return http.DefaultClient
}, instrumentor, errorReporter)
httpClient := codeClientHTTP.NewHTTPClient(
func() *http.Client {
return http.DefaultClient
},
codeClientHTTP.WithRetryCount(3),
codeClientHTTP.WithInstrumentor(instrumentor),
codeClientHTTP.WithErrorReporter(errorReporter),
codeClientHTTP.WithLogger(newLogger(t)),
)
client = deepcode.NewSnykCodeClient(newLogger(t), httpClient, instrumentor, errorReporter, config)
}

Expand Down
4 changes: 3 additions & 1 deletion internal/util/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,9 @@ import (
)

func GetRepositoryUrl(path string) (string, error) {
repo, err := git.PlainOpen(path)
repo, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make it more complicated. How do we treat git subrepos?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test with what I expected to happen and it passes: 1a10f0e. Could you have a look and see if this matches what you think should happen?

DetectDotGit: true,
})
if err != nil {
return "", fmt.Errorf("open local repository: %w", err)
}
Expand Down
45 changes: 41 additions & 4 deletions internal/util/repository_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package util_test

import (
"net/url"
"path/filepath"
"testing"

"github.com/go-git/go-git/v5"
Expand All @@ -25,10 +26,12 @@ import (
"github.com/snyk/code-client-go/internal/util"
)

func clone(t *testing.T, repoUrl string) (string, *git.Repository) {
func clone(t *testing.T, repoUrl string, repoDir string) (string, *git.Repository) {
t.Helper()

repoDir := t.TempDir()
if repoDir == "" {
repoDir = t.TempDir()
}
repo, err := git.PlainClone(repoDir, false, &git.CloneOptions{URL: repoUrl})
assert.NoError(t, err)
assert.NotNil(t, repo)
Expand All @@ -40,7 +43,7 @@ func Test_GetRepositoryUrl_repo_with_credentials(t *testing.T) {
// check out a repo and prepare its config to contain credentials in the URL
expectedRepoUrl := "https://github.com/snyk-fixtures/shallow-goof-locked.git"

repoDir, repo := clone(t, expectedRepoUrl)
repoDir, repo := clone(t, expectedRepoUrl, "")

config, err := repo.Config()
assert.NoError(t, err)
Expand All @@ -64,7 +67,18 @@ func Test_GetRepositoryUrl_repo_with_credentials(t *testing.T) {
func Test_GetRepositoryUrl_repo_without_credentials(t *testing.T) {
// check out a repo and prepare its config to contain credentials in the URL
expectedRepoUrl := "https://github.com/snyk-fixtures/shallow-goof-locked.git"
repoDir, _ := clone(t, "git@github.com:snyk-fixtures/shallow-goof-locked.git")
repoDir, _ := clone(t, expectedRepoUrl, "")

// run method under test
actualUrl, err := util.GetRepositoryUrl(repoDir)
assert.NoError(t, err)
assert.Equal(t, expectedRepoUrl, actualUrl)
}

func Test_GetRepositoryUrl_repo_with_ssh(t *testing.T) {
// check out a repo and prepare its config to contain credentials in the URL
expectedRepoUrl := "https://github.com/snyk-fixtures/shallow-goof-locked.git"
repoDir, _ := clone(t, "git@github.com:snyk-fixtures/shallow-goof-locked.git", "")

// run method under test
actualUrl, err := util.GetRepositoryUrl(repoDir)
Expand All @@ -78,3 +92,26 @@ func Test_GetRepositoryUrl_no_repo(t *testing.T) {
assert.Error(t, err)
assert.Empty(t, actualUrl)
}

func Test_GetRepositoryUrl_repo_subfolder(t *testing.T) {
expectedRepoUrl := "https://github.com/snyk-fixtures/mono-repo.git"
repoDir, _ := clone(t, "git@github.com:snyk-fixtures/mono-repo.git", "")

// run method under test
actualUrl, err := util.GetRepositoryUrl(filepath.Join(repoDir, "multi-module"))
assert.NoError(t, err)
assert.Equal(t, expectedRepoUrl, actualUrl)
}

func Test_GetRepositoryUrl_repo_submodule(t *testing.T) {
parentRepoDir, _ := clone(t, "https://github.com/snyk-fixtures/shallow-goof-locked.git", "")
nestedRepoDir, _ := clone(t, "git@github.com:snyk-fixtures/mono-repo.git", filepath.Join(parentRepoDir, "shallow-goof-locked"))
// run method under test
actualUrl, err := util.GetRepositoryUrl(parentRepoDir)
assert.NoError(t, err)
assert.Equal(t, "https://github.com/snyk-fixtures/shallow-goof-locked.git", actualUrl)

actualUrl, err = util.GetRepositoryUrl(nestedRepoDir)
assert.NoError(t, err)
assert.Equal(t, "https://github.com/snyk-fixtures/mono-repo.git", actualUrl)
}
12 changes: 9 additions & 3 deletions internal/workspace/2024-03-12/client_pact_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,15 @@ func setupPact(t *testing.T) {
logger := zerolog.New(zerolog.NewTestWriter(t))
instrumentor := testutil.NewTestInstrumentor()
errorReporter := testutil.NewTestErrorReporter()
httpClient := codeClientHTTP.NewHTTPClient(&logger, func() *http.Client {
return http.DefaultClient
}, instrumentor, errorReporter)
httpClient := codeClientHTTP.NewHTTPClient(
func() *http.Client {
return http.DefaultClient
},
codeClientHTTP.WithRetryCount(3),
codeClientHTTP.WithInstrumentor(instrumentor),
codeClientHTTP.WithErrorReporter(errorReporter),
codeClientHTTP.WithLogger(&logger),
)
var err error
client, err = v20240312.NewClientWithResponses(restApi, v20240312.WithHTTPClient(httpClient))
require.NoError(t, err)
Expand Down
37 changes: 37 additions & 0 deletions observability/error_reporter_impl.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* © 2024 Snyk Limited All rights reserved.
*
* Licensed 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 observability

import (
"github.com/rs/zerolog"
)

type errorReporter struct {
logger *zerolog.Logger
}

func NewErrorReporter(logger *zerolog.Logger) ErrorReporter {
return &errorReporter{logger}
}

func (s *errorReporter) FlushErrorReporting() {
}

func (s *errorReporter) CaptureError(err error, options ErrorReporterOptions) bool {
s.logger.Log().Err(err).Msg("An error has been captured by the error reporter")
return true
}
Loading