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

fix: propagate failed scans with --all-projects #1301

Merged
merged 1 commit into from
Aug 4, 2020
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
5 changes: 4 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,7 @@ cert.pem
key.pem

# Diagnostic reports (https://nodejs.org/api/report.html)
report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json
report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json
# test fixture build artifacts
/test/acceptance/workspaces/**/project/
/test/acceptance/workspaces/**/target/
23 changes: 17 additions & 6 deletions src/cli/commands/monitor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ import {
GitRepoCommitStats,
execShell,
} from '../../../lib/monitor/dev-count-analysis';
import { FailedToRunTestError } from '../../../lib/errors';
import { FailedToRunTestError, MonitorError } from '../../../lib/errors';

const SEPARATOR = '\n-------------------------------------------------------\n';
const debug = Debug('snyk');
Expand Down Expand Up @@ -166,13 +166,8 @@ async function monitor(...args0: MethodArgs): Promise<any> {
}),
spinner.clear(analyzingDepsSpinnerLabel),
);

analytics.add('pluginName', inspectResult.plugin.name);

const postingMonitorSpinnerLabel =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved a little further down unchanged

'Posting monitor snapshot for ' + displayPath + ' ...';
await spinner(postingMonitorSpinnerLabel);

// We send results from "all-sub-projects" scanning as different Monitor objects
// multi result will become default, so start migrating code to always work with it
let perProjectResult: MultiProjectResultCustom;
Expand All @@ -183,6 +178,22 @@ async function monitor(...args0: MethodArgs): Promise<any> {
perProjectResult = convertMultiResultToMultiCustom(inspectResult);
}

const failedResults = (inspectResult as MultiProjectResultCustom)
.failedResults;
if (failedResults?.length) {
failedResults.forEach((result) => {
results.push({
ok: false,
data: new MonitorError(500, result.errMessage),
path: result.targetFile || '',
});
});
}

const postingMonitorSpinnerLabel =
'Posting monitor snapshot for ' + displayPath + ' ...';
await spinner(postingMonitorSpinnerLabel);

// Post the project dependencies to the Registry
for (const projectDeps of perProjectResult.scannedProjects) {
try {
Expand Down
2 changes: 1 addition & 1 deletion src/cli/commands/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ async function test(...args: MethodArgs): Promise<TestCommandResult> {
testOpts.path = path;
testOpts.projectName = testOpts['project-name'];

let res;
let res: (TestResult | TestResult[]) | Error;

try {
res = await snyk.test(path, testOpts);
Expand Down
7 changes: 5 additions & 2 deletions src/lib/plugins/get-deps-from-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ import { legacyPlugin as pluginApi } from '@snyk/cli-interface';
import { find } from '../find-files';
import { Options, TestOptions, MonitorOptions } from '../types';
import { NoSupportedManifestsFoundError } from '../errors';
import { getMultiPluginResult } from './get-multi-plugin-result';
import {
getMultiPluginResult,
MultiProjectResultCustom,
} from './get-multi-plugin-result';
import { getSinglePluginResult } from './get-single-plugin-result';
import {
detectPackageFile,
Expand Down Expand Up @@ -32,7 +35,7 @@ const multiProjectProcessors = {
export async function getDepsFromPlugin(
root: string,
options: Options & (TestOptions | MonitorOptions),
): Promise<pluginApi.MultiProjectResult> {
): Promise<pluginApi.MultiProjectResult | MultiProjectResultCustom> {
let inspectRes: pluginApi.InspectResult;

if (Object.keys(multiProjectProcessors).some((key) => options[key])) {
Expand Down
29 changes: 28 additions & 1 deletion src/lib/plugins/get-multi-plugin-result.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import { convertSingleResultToMultiCustom } from './convert-single-splugin-res-t
import { convertMultiResultToMultiCustom } from './convert-multi-plugin-res-to-multi-custom';
import { PluginMetadata } from '@snyk/cli-interface/legacy/plugin';
import { CallGraph } from '@snyk/cli-interface/legacy/common';
import { FailedToRunTestError } from '../errors';

const debug = debugModule('snyk-test');
export interface ScannedProjectCustom
Expand All @@ -20,9 +21,16 @@ export interface ScannedProjectCustom
plugin: PluginMetadata;
callGraph?: CallGraph;
}

interface FailedProjectScanError {
targetFile?: string;
error?: Error;
errMessage: string;
}
export interface MultiProjectResultCustom
extends cliInterface.legacyPlugin.MultiProjectResult {
scannedProjects: ScannedProjectCustom[];
failedResults?: FailedProjectScanError[];
}

export async function getMultiPluginResult(
Expand All @@ -31,6 +39,8 @@ export async function getMultiPluginResult(
targetFiles: string[],
): Promise<MultiProjectResultCustom> {
const allResults: ScannedProjectCustom[] = [];
const failedResults: FailedProjectScanError[] = [];

for (const targetFile of targetFiles) {
const optionsClone = _.cloneDeep(options);
optionsClone.file = path.relative(root, targetFile);
Expand Down Expand Up @@ -68,14 +78,31 @@ export async function getMultiPluginResult(

allResults.push(...pluginResultWithCustomScannedProjects.scannedProjects);
} catch (err) {
debug(chalk.bold.red(err.message));
// TODO: propagate this all the way back and include in --json output
failedResults.push({
targetFile,
error: err,
errMessage: err.message || 'Something went wrong getting dependencies',
});
debug(
chalk.bold.red(
`\n✗ Failed to get dependencies for ${targetFile}\nERROR: ${err.message}\n`,
),
);
}
}

if (!allResults.length) {
throw new FailedToRunTestError(
`Failed to get dependencies for all ${targetFiles.length} potential projects. Run with \`-d\` for debug output and contact support@snyk.io`,
);
}

return {
plugin: {
name: 'custom-auto-detect',
},
scannedProjects: allResults,
failedResults,
};
}
1 change: 1 addition & 0 deletions src/lib/plugins/nodejs-plugin/npm-modules-parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export async function parse(
'Analyzing npm dependencies for ' +
path.dirname(path.resolve(root, targetFile));
try {
await spinner.clear<void>(resolveModuleSpinnerLabel)();
await spinner(resolveModuleSpinnerLabel);
if (targetFile.endsWith('yarn.lock')) {
options.file =
Expand Down
20 changes: 18 additions & 2 deletions src/lib/snyk-test/run-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import * as fs from 'fs';
import * as _ from '@snyk/lodash';
import * as path from 'path';
import * as debugModule from 'debug';
import chalk from 'chalk';
import * as pathUtil from 'path';
import { parsePackageString as moduleToObject } from 'snyk-module';
import * as depGraphLib from '@snyk/dep-graph';
Expand Down Expand Up @@ -42,7 +43,10 @@ import {
} from '../types';
import { pruneGraph } from '../prune';
import { getDepsFromPlugin } from '../plugins/get-deps-from-plugin';
import { ScannedProjectCustom } from '../plugins/get-multi-plugin-result';
import {
ScannedProjectCustom,
MultiProjectResultCustom,
} from '../plugins/get-multi-plugin-result';

import request = require('../request');
import spinner = require('../spinner');
Expand Down Expand Up @@ -72,6 +76,7 @@ async function sendAndParseResults(
): Promise<TestResult[]> {
const results: TestResult[] = [];
for (const payload of payloads) {
await spinner.clear<void>(spinnerLbl)();
await spinner(spinnerLbl);
if (options.iac) {
const iacScan: IacScan = payload.body as IacScan;
Expand Down Expand Up @@ -348,12 +353,23 @@ async function assembleLocalPayloads(

try {
const payloads: Payload[] = [];

await spinner.clear<void>(spinnerLbl)();
await spinner(spinnerLbl);
if (options.iac) {
return assembleIacLocalPayloads(root, options);
}
const deps = await getDepsFromPlugin(root, options);
const failedResults = (deps as MultiProjectResultCustom).failedResults;
if (failedResults?.length) {
await spinner.clear<void>(spinnerLbl)();
console.warn(
chalk.bold.red(
`✗ ${failedResults.length}/${failedResults.length +
deps.scannedProjects
.length} potential projects failed to get dependencies. Run with \`-d\` for debug output.`,
),
);
}
analytics.add('pluginName', deps.plugin.name);
const javaVersion = _.get(
deps.plugin,
Expand Down
19 changes: 11 additions & 8 deletions test/acceptance/cli-monitor/cli-monitor.all-projects.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,14 @@ export const AllProjectsTests: AcceptanceTests = {
utils.chdirWorkspaces();
const spyPlugin = sinon.spy(params.plugins, 'loadPlugin');
t.teardown(spyPlugin.restore);

const result = await params.cli.monitor('monorepo-bad-project', {
allProjects: true,
});
let result;
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these were not previously propagated up if dependencies failed to be detected, but we do fail if one of the paths fails to be scanned so this aligns that behaviour.

This may however start failing some pipelines that previously were skipping the failures. How can this be communicated & is this the desired behaviour right? You want to know something you monitored failed and it did not pass.

await params.cli.monitor('monorepo-bad-project', {
allProjects: true,
});
} catch (error) {
result = error.message;
}
t.ok(spyPlugin.withArgs('rubygems').calledOnce, 'calls rubygems plugin');
t.ok(spyPlugin.withArgs('yarn').calledOnce, 'calls npm plugin');
t.ok(spyPlugin.withArgs('maven').notCalled, 'did not call maven plugin');
Expand All @@ -184,11 +188,10 @@ export const AllProjectsTests: AcceptanceTests = {
'rubygems/graph/some/project-id',
'rubygems project was monitored',
);

t.notMatch(
t.match(
result,
'yarn/graph/some/project-id',
'yarn project was not monitored',
'Dependency snyk was not found in yarn.lock',
'yarn project had an error and we displayed it',
);

const request = params.server.popRequest();
Expand Down
17 changes: 17 additions & 0 deletions test/acceptance/cli-test/cli-test.all-projects.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,23 @@ import { CommandResult } from '../../../src/cli/commands/types';
export const AllProjectsTests: AcceptanceTests = {
language: 'Mixed',
tests: {
'`test yarn-out-of-sync` out of sync fails': (params, utils) => async (
t,
) => {
utils.chdirWorkspaces();
try {
await params.cli.test('yarn-workspace-out-of-sync', {
allProjects: true,
});
t.fail('Should fail');
} catch (e) {
t.equal(
e.message,
'\nTesting yarn-workspace-out-of-sync...\n\nFailed to get dependencies for all 3 potential projects. Run with `-d` for debug output and contact support@snyk.io',
'Contains enough info about err',
);
}
},
'`test mono-repo-with-ignores --all-projects` respects .snyk policy': (
params,
utils,
Expand Down