refactor: http client and deepcode client [IDE-195] #22
Conversation
teodora-sandu
force-pushed
the
refactor/http-client
branch
3 times, most recently
from
6a6ca0c to
75fe4ab
Compare
teodora-sandu
changed the title
teodora-sandu
force-pushed
the
refactor/http-client
branch
from
bc53a53 to
95364d6
Compare
| } | ||
|
|
||
| func NewSnykCodeClient( | ||
| logger *zerolog.Logger, | ||
| httpClient codeClientHTTP.HTTPClient, | ||
| instrumentor observability.Instrumentor, | ||
| errorReporter observability.ErrorReporter, |
There was a problem hiding this comment.
We should consider using the Function Options Pattern, as the constructor parameters are growing quite a bit :).
I also wonder, if we do that, if we should assume sensible defaults for errorReporter, e.g implementing an error reporter that just logs, an instrumentor that does nothing etc
There was a problem hiding this comment.
Great idea, that's very similar to what I did in #23 for the code scanner. This deepcode client is hopefully not going to be here very long so I think I will leave it as is for now and look at doing what you suggested after PR#23 gets merged.
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
| var bundleResponse BundleResponse | ||
| err = json.Unmarshal(responseBody, &bundleResponse) | ||
| return bundleResponse.BundleHash, bundleResponse.MissingFiles, err | ||
| } | ||
|
|
||
| // This is only exported for tests. |
There was a problem hiding this comment.
Would it be an option, to just use the same package?
There was a problem hiding this comment.
We're doing blackbox testing here. Technically I should not test this function but the code I extracted from snyk-ls was testing it and I wanted to keep it to make sure it continue working. I'll maybe think about writing the other tests in a way that tests this function too
|
|
||
| // This is only exported for tests. | ||
| func (s *snykCodeClient) Host() (string, error) { | ||
| var codeApiRegex = regexp.MustCompile(`^(deeproxy\.)?`) |
There was a problem hiding this comment.
I think this is not correct - for local code engine and on Fedramp, it is not always deeproxy.
There was a problem hiding this comment.
This is how this code works in https://github.com/snyk/snyk-ls/blob/75df46322994145b2d1ca8ca9e464dc1394357c4/infrastructure/code/snyk_code_http_client.go#L589-L607 too. Are we thinking that's incorrect too?
teodora-sandu
force-pushed
the
refactor/http-client
branch
from
95364d6 to
a3a873b
Compare
teodora-sandu
force-pushed
the
refactor/http-client
branch
from
a3a873b to
dc43572
Compare
Refactors the HTTP client, so that it's a general client that can be used when making calls to the Workspace and Orchestration API as part of https://snyksec.atlassian.net/browse/IDE-195.
This means:
deepcodepackage from thehttppackagehttppackage is just a client with retries and can then be used to pass to the clients generated byoapi-codegenTested it with
snyk-lsandsnyk-intellij-pluginby following https://snyksec.atlassian.net/wiki/spaces/IDE/pages/1966866488/How+to+setup+development+for+Consistent+Ignores