Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: convert nodejs plugin response to use multi format #773

Merged
merged 2 commits into from
Oct 7, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions src/cli/commands/monitor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ interface GoodResult {
ok: true;
data: string;
path: string;
subProjectName?: string;
projectName?: string;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change of renaming this from subProjectName to projectName. We don't really have a concept of sub project, as we monitor each sub project as separate project.

}

interface BadResult {
Expand Down Expand Up @@ -166,10 +166,10 @@ async function monitor(...args0: MethodArgs): Promise<any> {
// SinglePackageResult is a legacy format understood by Registry, so we have to convert
// a MultiProjectResult to an array of these.

let perSubProjectResults: pluginApi.SinglePackageResult[] = [];
let perProjectResult: pluginApi.SinglePackageResult[] = [];
let advertiseSubprojectsCount: number | null = null;
if (pluginApi.isMultiResult(inspectResult)) {
perSubProjectResults = inspectResult.scannedProjects.map(
perProjectResult = inspectResult.scannedProjects.map(
(scannedProject) => ({
plugin: inspectResult.plugin,
package: scannedProject.depTree,
Expand All @@ -185,15 +185,15 @@ async function monitor(...args0: MethodArgs): Promise<any> {
advertiseSubprojectsCount =
inspectResult.plugin.meta.allSubProjectNames.length;
}
perSubProjectResults = [inspectResult];
perProjectResult = [inspectResult];
}

// Post the project dependencies to the Registry
for (const subProjDeps of perSubProjectResults) {
maybePrintDeps(options, subProjDeps.package);
for (const projectDeps of perProjectResult) {
maybePrintDeps(options, projectDeps.package);

const res = await promiseOrCleanup(
snykMonitor(path, meta, subProjDeps, targetFile),
snykMonitor(path, meta, projectDeps, targetFile),
spinner.clear(postingMonitorSpinnerLabel),
);

Expand All @@ -209,18 +209,18 @@ async function monitor(...args0: MethodArgs): Promise<any> {
const manageUrl = url.format(endpoint);

endpoint.pathname = leader + '/monitor/' + res.id;
const subProjectName = pluginApi.isMultiResult(inspectResult)
? subProjDeps.package.name
const projectName = pluginApi.isMultiResult(inspectResult)
? projectDeps.package.name
: undefined;
const monOutput = formatMonitorOutput(
packageManager,
res,
manageUrl,
options,
subProjectName,
projectName,
advertiseSubprojectsCount,
);
results.push({ ok: true, data: monOutput, path, subProjectName });
results.push({ ok: true, data: monOutput, path, projectName });
}
// push a good result
} catch (err) {
Expand All @@ -233,8 +233,8 @@ async function monitor(...args0: MethodArgs): Promise<any> {
let dataToSend = results.map((result) => {
if (result.ok) {
const jsonData = JSON.parse(result.data);
if (result.subProjectName) {
jsonData.subProjectName = result.subProjectName;
if (result.projectName) {
jsonData.projectName = result.projectName;
}
return jsonData;
}
Expand Down Expand Up @@ -282,12 +282,12 @@ function formatMonitorOutput(
res: MonitorResult,
manageUrl,
options,
subProjectName?: string,
projectName?: string,
advertiseSubprojectsCount?: number | null,
) {
const issues = res.licensesPolicy ? 'issues' : 'vulnerabilities';
const humanReadableName = subProjectName
? `${res.path} (${subProjectName})`
const humanReadableName = projectName
? `${res.path} (${projectName})`
: res.path;
const strOutput =
chalk.bold.white('\nMonitoring ' + humanReadableName + '...\n\n') +
Expand Down
10 changes: 5 additions & 5 deletions src/cli/commands/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -112,12 +112,12 @@ async function test(...args: MethodArgs): Promise<string> {
results.push(_.assign(resArray[i], { path }));
// currently testOpts are identical for each test result returned even if it's for multiple projects.
// we want to return the project names, so will need to be crafty in a way that makes sense.
if (!testOpts.subProjectNames) {
if (!testOpts.projectNames) {
resultOptions.push(testOpts);
} else {
resultOptions.push(
_.assign(_.cloneDeep(testOpts), {
subProjectName: testOpts.subProjectNames[i],
projectName: testOpts.projectNames[i],
}),
);
}
Expand Down Expand Up @@ -478,10 +478,10 @@ function metaForDisplay(res, options) {
options.file,
);
}
if (options.subProjectName) {
if (options.projectName) {
meta.push(
chalk.bold(rightPadWithSpaces('Sub project: ', padToLength)) +
options.subProjectName,
chalk.bold(rightPadWithSpaces('Project name: ', padToLength)) +
options.projectName,
);
}
if (options.docker) {
Expand Down
11 changes: 7 additions & 4 deletions src/lib/plugins/nodejs-plugin/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@ import * as modulesParser from './npm-modules-parser';
import * as lockParser from './npm-lock-parser';
import * as types from '../types';
import { MissingTargetFileError } from '../../errors/missing-targetfile-error';
import { MultiProjectResult } from '@snyk/cli-interface/legacy/plugin';

export async function inspect(
root: string,
targetFile: string,
options: types.Options = {},
): Promise<types.InspectResult> {
): Promise<MultiProjectResult> {
if (!targetFile) {
throw MissingTargetFileError(root);
}
Expand All @@ -16,13 +17,15 @@ export async function inspect(
targetFile.endsWith('yarn.lock');

const getLockFileDeps = isLockFileBased && !options.traverseNodeModules;
const depTree: any = getLockFileDeps
? await lockParser.parse(root, targetFile, options)
: await modulesParser.parse(root, targetFile, options);

return {
plugin: {
name: 'snyk-nodejs-lockfile-parser',
runtime: process.version,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't see an issue there

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the issue will be that different projects will be scanned soon and they will not share always the exact same version of gradle or maven or sbt for example. Or node in this example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we can fix that easily I think but adding meta inside each result not on a top level response

},
package: getLockFileDeps
? await lockParser.parse(root, targetFile, options)
: await modulesParser.parse(root, targetFile, options),
scannedProjects: [{ depTree }],
};
}
1 change: 1 addition & 0 deletions src/lib/plugins/rubygems/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ export async function inspect(
name: 'bundled:rubygems',
runtime: 'unknown',
},
// TODO: must be a depTree!
package: {
name: specs.packageName,
targetFile: specs.targetFile,
Expand Down
2 changes: 1 addition & 1 deletion src/lib/snyk-test/run-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ async function getDepsFromPlugin(
// We are using "options" to store some information returned from plugin that we need to use later,
// but don't want to send to Registry in the Payload.
// TODO(kyegupov): decouple inspect and payload so that we don't need this hack
(options as any).subProjectNames = inspectRes.scannedProjects.map(
(options as any).projectNames = inspectRes.scannedProjects.map(
(scannedProject) => scannedProject.depTree.name,
);
return inspectRes;
Expand Down
2 changes: 1 addition & 1 deletion src/lib/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export interface Options {
'show-vulnerable-paths'?: string;
packageManager: SupportedPackageManagers;
advertiseSubprojectsCount?: number;
subProjectNames?: string[];
projectNames?: string[];
severityThreshold?: SEVERITY;
dev?: boolean;
'print-deps'?: boolean;
Expand Down
15 changes: 10 additions & 5 deletions test/acceptance/cli.acceptance.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -756,8 +756,8 @@ test('`test gradle-app --all-sub-projects` returns correct multi tree meta', asy
t.match(meta[0], /Organization:\s+test-org/, 'organization displayed');
t.match(meta[1], /Package manager:\s+gradle/, 'package manager displayed');
t.match(meta[2], /Target file:\s+build.gradle/, 'target file displayed');
t.match(meta[3], /Sub project:\s+tree/, 'subproject displayed');
t.includes(meta[3], `tree${i}`, 'subproject displayed');
t.match(meta[3], /Project name:\s+tree/, 'sub-project displayed');
t.includes(meta[3], `tree${i}`, 'sub-project displayed');
t.match(meta[4], /Open source:\s+no/, 'open source displayed');
t.match(meta[5], /Project path:\s+gradle-app/, 'path displayed');
t.notMatch(
Expand All @@ -782,9 +782,14 @@ test('`test npm-package-policy` returns correct meta', async (t) => {
t.match(meta[0], /Organization:\s+test-org/, 'organization displayed');
t.match(meta[1], /Package manager:\s+npm/, 'package manager displayed');
t.match(meta[2], /Target file:\s+package.json/, 'target file displayed');
t.match(meta[3], /Open source:\s+no/, 'open source displayed');
t.match(meta[4], /Project path:\s+npm-package-policy/, 'path displayed');
t.match(meta[5], /Local Snyk policy:\s+found/, 'local policy displayed');
t.match(
meta[3],
/Project name:\s+custom-policy-location-package/,
'project name displayed',
);
t.match(meta[4], /Open source:\s+no/, 'open source displayed');
t.match(meta[5], /Project path:\s+npm-package-policy/, 'path displayed');
t.match(meta[6], /Local Snyk policy:\s+found/, 'local policy displayed');
});

test('`test ruby-gem-no-lockfile --file=ruby-gem.gemspec`', async (t) => {
Expand Down