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

refactor: set default test options #1704

Merged
merged 3 commits into from
Mar 12, 2021
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 src/cli/commands/protect/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,10 @@ import * as errors from '../../../lib/errors';
const debug = debugModule('snyk');

async function protectFunc(
options: types.PolicyOptions & types.Options & types.TestOptions,
options: types.PolicyOptions &
types.Options &
types.TestOptions &
types.ProtectOptions,
) {
const protectOptions = { ...options };
protectOptions.loose = true; // replace missing policies with empty ones
Expand Down
4 changes: 2 additions & 2 deletions src/cli/commands/protect/wizard.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ import {
Options,
MonitorMeta,
MonitorResult,
WizardOptions,
PackageJson,
ProtectOptions,
} from '../../../lib/types';
import { LegacyVulnApiResult } from '../../../lib/snyk-test/legacy';
import { MultiProjectResult } from '@snyk/cli-interface/legacy/plugin';
Expand Down Expand Up @@ -82,7 +82,7 @@ async function processPackageManager(options: Options) {
return Promise.resolve(options);
}

async function loadOrCreatePolicyFile(options: Options & WizardOptions) {
async function loadOrCreatePolicyFile(options: Options & ProtectOptions) {
let policyFile;
try {
policyFile = await snyk.policy.load(options['policy-path'], options);
Expand Down
21 changes: 3 additions & 18 deletions src/cli/commands/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,11 @@ const cloneDeep = require('lodash.clonedeep');
const assign = require('lodash.assign');
import chalk from 'chalk';
import * as snyk from '../../../lib';
import * as config from '../../../lib/config';
import { isCI } from '../../../lib/is-ci';
import * as Debug from 'debug';
import * as pathLib from 'path';
import {
Options,
ShowVulnPaths,
SupportedProjectTypes,
TestOptions,
} from '../../../lib/types';
Expand Down Expand Up @@ -54,30 +52,17 @@ import * as iacLocalExecution from './iac-local-execution';
import { validateCredentials } from './validate-credentials';
import { generateSnykTestError } from './generate-snyk-test-error';
import { validateTestOptions } from './validate-test-options';
import { setDefaultTestOptions } from './set-default-test-options';
import { processCommandArgs } from '../process-command-args';

const debug = Debug('snyk-test');
const SEPARATOR = '\n-------------------------------------------------------\n';

const showVulnPathsMapping: Record<string, ShowVulnPaths> = {
false: 'none',
none: 'none',
true: 'some',
some: 'some',
all: 'all',
};

// TODO: avoid using `as any` whenever it's possible

async function test(...args: MethodArgs): Promise<TestCommandResult> {
const { options, paths } = processCommandArgs(...args);
// org fallback to config unless specified
options.org = options.org || config.org;

// making `show-vulnerable-paths` 'some' by default.
const svpSupplied = (options['show-vulnerable-paths'] || '').toLowerCase();
options.showVulnPaths = showVulnPathsMapping[svpSupplied] || 'some';

const { options: originalOptions, paths } = processCommandArgs(...args);
const options = setDefaultTestOptions(originalOptions);
validateTestOptions(options);
validateCredentials(options);

Expand Down
25 changes: 25 additions & 0 deletions src/cli/commands/test/set-default-test-options.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import * as config from '../../../lib/config';
import { Options, ShowVulnPaths, TestOptions } from '../../../lib/types';

export function setDefaultTestOptions(options: Options): Options & TestOptions {
const svpSupplied = (options['show-vulnerable-paths'] || '')
.toString()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this essentially adds support for --show-vulnerable-paths to be treated as --show-vulnerable-paths=true WDYT?

Otherwise if you run test with snyk test --show-vulnerable-paths you get a bad error that says
toLowerCase is not a function because it is not a string. Or alternative is to add validation for this options and throw outside of this function somewhere probably

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 this sounds in line with how arg parsing libraries should treat this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gitphill while @aviadhahami is off, could you help 👀 ?

.toLowerCase();

delete options['show-vulnerable-paths'];
return {
...options,
// org fallback to config unless specified
org: options.org || config.org,
// making `show-vulnerable-paths` 'some' by default.
showVulnPaths: showVulnPathsMapping[svpSupplied] || 'some',
};
}

const showVulnPathsMapping: Record<string, ShowVulnPaths> = {
false: 'none',
none: 'none',
true: 'some',
some: 'some',
all: 'all',
};
9 changes: 5 additions & 4 deletions src/lib/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ export type DepTree = legacyApi.DepTree;
export type ShowVulnPaths = 'none' | 'some' | 'all';

export interface TestOptions {
traverseNodeModules: boolean;
interactive: boolean;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a protect option for wizard only

traverseNodeModules?: boolean;
pruneRepeatedSubdependencies?: boolean;
showVulnPaths: ShowVulnPaths;
failOn?: FailOn;
Expand All @@ -27,7 +26,8 @@ export interface TestOptions {
iacDirFiles?: IacFileInDirectory[];
}

export interface WizardOptions {
export interface ProtectOptions {
interactive?: boolean;
newPolicy: boolean;
}

Expand Down Expand Up @@ -184,7 +184,8 @@ export type SupportedUserReachableFacingCliArgs =
| 'reachable-vulns-timeout'
| 'init-script'
| 'integration-name'
| 'integration-version';
| 'integration-version'
| 'show-vulnerable-paths';

export enum SupportedCliCommands {
version = 'version',
Expand Down
3 changes: 0 additions & 3 deletions test/run-test.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ describe('CLI runTest - propagate correct user error', () => {
const options: Options & TestOptions = {
path: '',
traverseNodeModules: false,
interactive: false,
showVulnPaths: 'none',
};

Expand All @@ -37,7 +36,6 @@ describe('CLI runTest - propagate correct user error', () => {
const options: Options & TestOptions = {
path: '',
traverseNodeModules: false,
interactive: false,
showVulnPaths: 'none',
};

Expand All @@ -59,7 +57,6 @@ describe('CLI runTest - propagate correct user error', () => {
const options: Options & TestOptions = {
path: '',
traverseNodeModules: false,
interactive: false,
showVulnPaths: 'none',
};

Expand Down
99 changes: 99 additions & 0 deletions test/set-default-test-options.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { setDefaultTestOptions } from '../src/cli/commands/test/set-default-test-options';

describe('setDefaultTestOptions', () => {
it('defaults to show-vulnerable-paths:some & org from config when no options passed in', () => {
const updated = setDefaultTestOptions({ path: '/' });
expect(updated).toEqual({
org: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious, in the jest's equals logic, is org: undefined same as not having org key at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does indeed!

path: '/',
showVulnPaths: 'some',
});
});

it('with show-vulnerable-paths set to `false` => `none`', () => {
const options = {
path: '/',
'show-vulnerable-paths': 'false',
};
const updated = setDefaultTestOptions(options);
expect(updated).toEqual({
org: undefined,
path: '/',
showVulnPaths: 'none',
});
});

it('with show-vulnerable-paths as boolean => `some`', () => {
const options = {
path: '/',
'show-vulnerable-paths': true,
};
const updated = setDefaultTestOptions(options as any);
expect(updated).toEqual({
org: undefined,
path: '/',
showVulnPaths: 'some',
});
});

it('with show-vulnerable-paths set to `none` => `none`', () => {
const options = {
path: '/',
'show-vulnerable-paths': 'none',
};
const updated = setDefaultTestOptions(options);
expect(updated).toEqual({
org: undefined,
path: '/',
showVulnPaths: 'none',
});
});

it('with show-vulnerable-paths set to `true` => `some`', () => {
const options = {
path: '/',
'show-vulnerable-paths': 'true',
};
const updated = setDefaultTestOptions(options);
expect(updated).toEqual({
org: undefined,
path: '/',
showVulnPaths: 'some',
});
});

it('with show-vulnerable-paths set to `some` => `some`', () => {
const options = {
path: '/',
'show-vulnerable-paths': 'some',
};
const updated = setDefaultTestOptions(options);
expect(updated).toEqual({
org: undefined,
path: '/',
showVulnPaths: 'some',
});
});

it('with show-vulnerable-paths set to `all` => `all`', () => {
const options = {
path: '/',
'show-vulnerable-paths': 'all',
};
const updated = setDefaultTestOptions(options);
expect(updated).toEqual({
org: undefined,
path: '/',
showVulnPaths: 'all',
});
});

it('with org set', () => {
const updated = setDefaultTestOptions({ path: '/', org: 'my-org' });
expect(updated).toEqual({
org: 'my-org',
path: '/',
showVulnPaths: 'some',
});
});
});
2 changes: 0 additions & 2 deletions test/snyk-code-test.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ describe('Test snyk code', () => {
const options: Options & TestOptions = {
path: '',
traverseNodeModules: false,
interactive: false,
showVulnPaths: 'none',
};
isFeatureFlagSupportedForOrgSpy.mockResolvedValueOnce({ code: 401 });
Expand All @@ -82,7 +81,6 @@ describe('Test snyk code', () => {
const options: Options & TestOptions = {
path: '',
traverseNodeModules: false,
interactive: false,
showVulnPaths: 'none',
code: true,
};
Expand Down