feat: depGraph snyk-test for npm and yarn #459
Conversation
orsagie
force-pushed
the
refactor/snyk-test-npm
branch
10 times, most recently
from
85e7787
to
a1129d6
Compare
orsagie
changed the title
orsagie
force-pushed
the
refactor/snyk-test-npm
branch
from
a1129d6
to
fbad1a6
Compare
orsagie
force-pushed
the
refactor/snyk-test-npm
branch
from
fbad1a6
to
a798a2e
Compare
|
|
||
| if (!manifestFileFullPath && lockFileFullPath) { | ||
| throw new Error('Detected a lockfile at location: ' | ||
| + lockFileFullPath + '\n However the package.json is missing!'); |
There was a problem hiding this comment.
misleading message: it appears to be not missing, but rather unspecified
There was a problem hiding this comment.
Now that I look at it, there is no way for it to not be specified. And we already check that it exists earlier, so will altogether remove this check.
| import * as fs from 'fs'; | ||
| import * as lockFileParser from 'snyk-nodejs-lockfile-parser'; | ||
|
|
||
| export async function parse(root, targetFile, options) { |
There was a problem hiding this comment.
can you specify the parameter and return types please? it's very useful for exported functions
|
|
||
| function countUniqueVulns(vulns: AnnotatedIssue[]): number { | ||
| const seen = {}; | ||
| const count = vulns.reduce((acc, curr) => { |
There was a problem hiding this comment.
I'd much rather use for..of and Object.keys(seen).length instead of reduce and acc
There was a problem hiding this comment.
This code is mostly copied from what it was, and is going away in next PR.
|
|
||
| return { | ||
| method: 'POST', | ||
| // options.vulnEndpoint is only used for file system tests |
There was a problem hiding this comment.
Maybe this comment belongs above, where options.vulnEndpoint is actually used?
Also, I think, vulnEndpoint itself could use more explanation. When is it set, what does it mean, can the code between those two calls be shared?
| return assembleRemotePayload(root, options); | ||
| } | ||
|
|
||
| function assembleRemotePayload(root: string, options): Payload { |
There was a problem hiding this comment.
assembleRemotePayload is not immediately understandable. Maybe rename it to payloadForTestingRepositoryPackages? and the other one payloadForTestingLocalPackages?
There was a problem hiding this comment.
will do it for the next PR
orsagie
force-pushed
the
refactor/snyk-test-npm
branch
from
fcf95e9
to
6ccd088
Compare
| analytics.add('vulns-pre-policy', res.vulnerabilities.length); | ||
|
|
||
| res.filesystemPolicy = filesystemPolicy; | ||
| res.filesystemPolicy = !!payloadPolicy; |
There was a problem hiding this comment.
Is filesystemPolicy even used somewhere? :-)
|
Superseded by #494 |
What does this PR do?
snyk-test/npmpath.test-dep-graphendpoint and converts the result back to a depTree