feat: add client for deepcode [IDE-175] #10
Conversation
teodora-sandu
force-pushed
the
feat/snyk-code-client
branch
2 times, most recently
from
794039f
to
0b0d1af
Compare
| # TODO(forbidigo): revisit | ||
| #- forbidigo | ||
| # TODO(forcetypeassert): revisit this one! beware fragile asserts in this codebase | ||
| #- forcetypeassert |
There was a problem hiding this comment.
I wish I didn't have to comment these out but it's linting problems that come from snyk-ls and although I tried to fix them, there were way too many
There was a problem hiding this comment.
Yes... I think we'll have to take these one at a time in separate PRs to knock down the tech debt. A nice calming activity for a Friday afternoon or a cooldown perhaps.
teodora-sandu
force-pushed
the
feat/snyk-code-client
branch
from
0b0d1af
to
f87f895
Compare
.circleci/config.yml
Outdated
There was a problem hiding this comment.
Suggestion for the future: we could run golangci-lint in fixing mode when format is called.
| pact.Setup(true) | ||
|
|
||
| t.Setenv("DEEPROXY_API_URL", fmt.Sprintf("http://localhost:%d", pact.Server.Port)) | ||
| config.CurrentConfig().SetOrganization(orgUUID) |
There was a problem hiding this comment.
I'm wondering, and I'm aware that this is not really part of the PR, whether we should switch code-client to ONLY use the go-application-framework configuration package.
internal/http/helpers.go
Outdated
There was a problem hiding this comment.
As discussed, we'd be relying on go-application-framework configuration for this.
| type localInstrumentor struct { | ||
| } | ||
|
|
||
| // NewTestInstrumentor is used in pact testing. |
There was a problem hiding this comment.
Why not just use the normal one? Originally, we had the TestInstrumentor because we didn't want to tests to end up in Sentry, but as we don't use Sentry for distributed tracing & instrumentation anymore, I don't see why we wouldn't be able to use the "normal" instrumentor. It basically just creates a request_id and makes it available in its context - and that's functionality that would be good to have even in tests.
There was a problem hiding this comment.
I think we could use the snyk-ls one but I wonder now if that's a good idea since we already spoke about how we're worried about the cyclic dependency of snyk-ls->code-client-go->snyk-ls
Like discussed I will merge this and then add the following improvements but let me know if you have any reason to believe we should use the snyk-ls instrumentor in the pact tests too:
- use the configuration from
GAFso we don't depend on the one insnyk-ls - refactor the
error_reporterso thatCaptureErrortakes in some configuration that decides whether to send diagnostics for LSP - use the logger from GAF
Implements the client for the DeepCode API calls, by more or less extracting it from
snyk-lsas it is today in https://github.com/snyk/snyk-ls/blob/5be0cbefa40f92837e7470e3550a60492ae1ed24/infrastructure/code/snyk_code_http_client.go. The API calls included are:GET /filtersPOST /bundlePUT /bundle/:bundleHashIt rewrites the unit tests to use Golang generated mocks and it duplicates the Pact testing that's already in
snyk-ls.We might eventually be able to just use this client and replace the code in
snyk-lsbut that is not necessarily what I plan on doing next.