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.

Sign up for GitHub

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

Jump to bottom

chore: change project name source #3570

Merged
merged 1 commit into from
Aug 15, 2022
Merged

chore: change project name source #3570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 1 addition & 4 deletions src/cli/commands/test/iac/v2/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export async function test(
options: Options & TestOptions,
): Promise<TestCommandResult> {
const testConfig = await prepareTestConfig(paths, options);
const { projectName, orgSettings } = testConfig;
const { orgSettings } = testConfig;

const testSpinner = buildSpinner({
options,
Expand All @@ -37,7 +37,6 @@ export async function test(
return buildOutput({
scanResult,
testSpinner,
projectName,
orgSettings,
options,
});
Expand All @@ -51,7 +50,6 @@ async function prepareTestConfig(
options: Options & TestOptions,
): Promise<TestConfig> {
const iacCachePath = pathLib.join(systemCachePath, 'iac');
const projectName = pathLib.basename(process.cwd());

const org = (options.org as string) || config.org;
const orgSettings = await getIacOrgSettings(org);
Expand All @@ -62,7 +60,6 @@ async function prepareTestConfig(
return {
paths,
iacCachePath,
projectName,
orgSettings,
userRulesBundlePath: config.IAC_BUNDLE_PATH,
userPolicyEnginePath: config.IAC_POLICY_ENGINE_PATH,
Expand Down
9 changes: 4 additions & 5 deletions src/lib/iac/test/v2/output.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,16 @@ import { convertEngineToSarifResults } from './sarif';
import { CustomError, FormattedCustomError } from '../../../errors';
import { SnykIacTestError } from './errors';
import stripAnsi from 'strip-ansi';
import * as path from 'path';

export function buildOutput({
scanResult,
testSpinner,
projectName,
orgSettings,
options,
}: {
scanResult: TestOutput;
testSpinner?: Ora;
projectName: string;
orgSettings: IacOrgSettings;
options: any;
}): TestCommandResult {
Expand All @@ -40,7 +39,6 @@ export function buildOutput({

const { responseData, jsonData, sarifData } = buildTestCommandResultData({
scanResult,
projectName,
orgSettings,
options,
});
Expand All @@ -62,15 +60,16 @@ export function buildOutput({

function buildTestCommandResultData({
scanResult,
projectName,
orgSettings,
options,
}: {
scanResult: TestOutput;
projectName: string;
orgSettings: IacOrgSettings;
options: any;
}) {
const projectName =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: I think this default value can cause more confusion than good.
If we considered projectName to be required and assert it with this default value, wouldn't it make more sense to make it required when receiving it from snyk-iac-test?
In what scenarios does snyk-iac-test not return a project name and we require this default value?

Copy link
Contributor Author

@YairZ101 YairZ101 Aug 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

If we considered projectName to be required and assert it with this default value, wouldn't it make more sense to make it required when receiving it from snyk-iac-test?

I agree with you that it would make sense if we would consider it required but currently, we do not.
as I said above it is an implementation detail so if in the future we see it as problematic we can always change it.

In what scenarios does snyk-iac-test not return a project name and we require this default value?

When we failed to generate results.
(BTW I might be wrong here but I'm pretty sure that if the PE failed to return results we are not even displaying the project name)

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case at the very least if results exists that means metadata should exist as well, meaning there'd be no need to have it as optional if it's 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 reason that metadata is optional is the same reason that resources is optional.
I agree with your logic here but I rather align with the current results type structure.

scanResult.results?.metadata?.projectName ?? path.basename(process.cwd());

const jsonData = jsonStringifyLargeObject(
convertEngineToJsonResults({
results: scanResult,
Expand Down
5 changes: 5 additions & 0 deletions src/lib/iac/test/v2/scan/results.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ export interface SnykIacTestOutput {
export interface Results {
resources?: Resource[];
vulnerabilities?: Vulnerability[];
metadata?: Metadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

Thought: Does the generation of the metadata depends on the existence of valid results?
If there are no results would there not be metadata?
Unless there's a direct coupling between the existence of results and metadata it makes more sense to me that metadata would be a top-level property of TestOutput.
If the project name generation is conditional, would it make sense to have the metadata property as required, and projectName within it as conditional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does the generation of the metadata depends on the existence of valid results?

Currently, it does because we are generating the metadata in the results processing stage which we enter only when there are valid results.
AFAIK when we don't generate results we are only displaying an error without any information about the tests so there is no reason to generate the metadata.

If the project name generation is conditional, would it make sense to have the metadata property as required, and projectName within it as conditional?

That's an implementation detail that we can always change in the future if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's an implementation detail, as it actually defines the contract we have with snyk-iac-test in the different flows and user stories.
On the other hand, the fact that we calculate the project name as part of the results processing step is an implementation detail, as it can be generated in another scope which then allows us to return it in all the desired flows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as it actually defines the contract we have with snyk-iac-test in the different flows and user stories

But the current flows and user stories do not display the project name if the test didn't produce results, this is why I think it is an implementation detail (similar to what you said in your 2nd half of the message).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition to that, personally, I believe we should stop displaying the project name in the test output because it is misleading because it doesn't match the platform definition of a "project".

}

export interface Metadata {
projectName: string;
}

export interface Vulnerability {
Expand Down
1 change: 0 additions & 1 deletion src/lib/iac/test/v2/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ export interface TestConfig {
iacCachePath: string;
userRulesBundlePath?: string;
userPolicyEnginePath?: string;
projectName: string;
orgSettings: IacOrgSettings;
report: boolean;
severityThreshold?: SEVERITY;
Expand Down