feat: monitor all-sub-projecs #432
Conversation
kyegupov
force-pushed
the
feat/monitor-all-sub-projects
branch
4 times, most recently
from
6b1367e
to
4d86d18
Compare
kyegupov
changed the title
kyegupov
force-pushed
the
feat/monitor-all-sub-projects
branch
3 times, most recently
from
53d9f0d
to
9ed5f5e
Compare
kyegupov
force-pushed
the
feat/monitor-all-sub-projects
branch
3 times, most recently
from
9bff0ff
to
f644bf7
Compare
src/cli/commands/monitor.ts
Outdated
| const pluginOptions = plugins.getPluginOptions(packageManager, options); | ||
|
|
||
| // TODO: the type should depend on multiDepRoots flag | ||
| const inspectResult: SingleDepRootResult|MultiDepRootsResult = |
There was a problem hiding this comment.
Are we safe with removing try/catch here?
Could we move back to try/catch block rather than inline promise catch? My previouse sentence reveals it's not readable enough (for me :-)).
There was a problem hiding this comment.
There's a Typescript reason for that. Added a documented promiseOrCleanup function do explain the intention.
src/cli/commands/monitor.ts
Outdated
| // SingleDepRootResult is a legacy format understood by Registry, so we have to convert | ||
| // a MultiDepRootsResult to an array of these. | ||
|
|
||
| const perDepRootResults: SingleDepRootResult[] = (inspectResult as MultiDepRootsResult).depRoots |
There was a problem hiding this comment.
Understand the intention, however I'd prefer less type casting here and map outside of ternary operator for readability (YMMV). I don't have a better approach, will think about it.
There was a problem hiding this comment.
Converted, made the code a bit more readable with a type guard.
src/cli/commands/monitor.ts
Outdated
| for (const depRootDeps of perDepRootResults) { | ||
| // TODO(kyegupov): make snyk.monitor typed by converting src/lib/index.js to TS | ||
| const snykMonitor = snyk.monitor as any as (path, meta, depRootDeps) => Promise<any>; | ||
| const res = await snykMonitor(path, meta, depRootDeps) |
There was a problem hiding this comment.
Same try/catch vs inline .catch suggestion
kyegupov
changed the title
kyegupov
force-pushed
the
feat/monitor-all-sub-projects
branch
from
f644bf7
to
d51b4a0
Compare
kyegupov
force-pushed
the
feat/monitor-all-sub-projects
branch
from
d51b4a0
to
d12167d
Compare
|
(not merging because weekend 🍺 ) |
|
(restarting the CI build) |
|
🎉 This PR is included in version 1.148.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What does this PR do?
Enables --all-sub-project flag for
snyk monitor. Data for each sub-project aka depRoot is sent to the Registry separately.--all-sub-project is incompatible with --project-name flag.
Where should the reviewer start?
monitor.ts
How should this be manually tested?
snyk monitor --all-sub-projecton a gradle project with subprojsWhat are the relevant tickets?
https://snyksec.atlassian.net/browse/BST-517
https://snyksec.atlassian.net/browse/BST-506