fix: repo url subfolder [IDE-246] #29
Conversation
teodora-sandu
force-pushed
the
fix/repo-url-subfolder
branch
from
07b44a2 to
df32ca2
Compare
| @@ -81,7 +82,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 { | |||
There was a problem hiding this comment.
Should we have incremental backoff here?
There was a problem hiding this comment.
I think we should but I will argue against doing this now because:
- We haven't seen a need for it yet and this is code that was taken from
snyk-lsso it has been used to call Deeproxy for a while now - 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?
http/http.go
Outdated
| clientFactory func() *http.Client | ||
| instrumentor observability.Instrumentor | ||
| errorReporter observability.ErrorReporter | ||
| logger *zerolog.Logger | ||
| } | ||
|
|
||
| func NewHTTPClient( | ||
| retryCount int, |
There was a problem hiding this comment.
I'd very much love a constructor with options (function options pattern), with a default one that assumes 3 as default retries.
There was a problem hiding this comment.
I've done that in 543855b. I prefer it with this pattern and I inteded to change it in the CodeScanner too so the exposed functions are consistent but that takes a bit of refactoring. How do we feel about pushing this and then i refactor the code in a follow-up (today or later, in its own ticket)?
| @@ -25,7 +25,9 @@ import ( | |||
| ) | |||
|
|
|||
| func GetRepositoryUrl(path string) (string, error) { | |||
| repo, err := git.PlainOpen(path) | |||
| repo, err := git.PlainOpenWithOptions(path, &git.PlainOpenOptions{ | |||
There was a problem hiding this comment.
Let's make it more complicated. How do we treat git subrepos?
There was a problem hiding this comment.
There was a problem hiding this comment.
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?
The CLI team have requested to be able to make the retry count configurable in the HTTP client.
The computation logic for the repo URL should work even on sub-folders.