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: add git metadata to monitor payload #462

Merged
merged 1 commit into from
May 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
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@
"configstore": "^3.1.2",
"debug": "^3.1.0",
"diff": "^4.0.1",
"git-url-parse": "11.1.2",
"glob": "^7.1.3",
"inquirer": "^6.2.2",
"lodash": "^4.17.11",
Expand Down
3 changes: 1 addition & 2 deletions src/cli/commands/monitor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ async function monitor(...args0: any[]): Promise<any> {
if (options['all-sub-projects'] && options['project-name']) {
throw new Error('`--all-sub-projects` is currently not compatible with `--project-name`');
}

await apiTokenExists('snyk monitor');
// Part 1: every argument is a scan target; process them sequentially
for (const path of args) {
Expand Down Expand Up @@ -153,7 +152,7 @@ async function monitor(...args0: any[]): Promise<any> {
// Post the project dependencies to the Registry
for (const depRootDeps of perDepRootResults) {
const res = await promiseOrCleanup(
snykMonitor(path, meta, depRootDeps),
snykMonitor(path, meta, depRootDeps, targetFile),
spinner.clear(postingMonitorSpinnerLabel));

await spinner.clear(postingMonitorSpinnerLabel)(res);
Expand Down
14 changes: 12 additions & 2 deletions src/lib/monitor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,17 @@ import * as _ from 'lodash';
import * as isCI from './is-ci';
import * as analytics from './analytics';
import { SingleDepRootResult, MonitorError } from './types';
import * as projectMetadata from './project-metadata';
import * as path from 'path';

// TODO(kyegupov): clean up the type, move to snyk-cli-interface repository

interface MonitorBody {
meta: Meta;
policy: string;
package: {}; // TODO(kyegupov): DepTree
target: {};
targetFileRelativePath: string;
targetFile: string;
}

Expand All @@ -35,7 +39,7 @@ interface Meta {
projectName: string;
}

export function monitor(root, meta, info: SingleDepRootResult): Promise<any> {
export function monitor(root, meta, info: SingleDepRootResult, targetFile): Promise<any> {
const pkg = info.package;
const pluginMeta = info.plugin;
let policyPath = meta['policy-path'];
Expand All @@ -52,8 +56,12 @@ export function monitor(root, meta, info: SingleDepRootResult): Promise<any> {
return snyk.policy.create();
}
return snyk.policy.load(policyLocations, opts);
}).then((policy) => {
}).then(async (policy) => {

Choose a reason for hiding this comment

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

why add async

Choose a reason for hiding this comment

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

this is returning a promise not using async await

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jonathan-fielding if you look a few lines below I'm using await there, an async function is a promise behind the scenes so this still holds true and works fine.

analytics.add('packageManager', packageManager);

const target = await projectMetadata.getInfo(pkg);
const targetFileRelativePath = targetFile ? path.relative(root, targetFile) : '';

// TODO(kyegupov): async/await
return new Promise((resolve, reject) => {
request({
Expand All @@ -79,7 +87,9 @@ export function monitor(root, meta, info: SingleDepRootResult): Promise<any> {
package: pkg,
// we take the targetFile from the plugin,
// because we want to send it only for specific package-managers
target,
targetFile: pluginMeta.targetFile,
targetFileRelativePath,
} as MonitorBody,
gzip: true,
method: 'PUT',
Expand Down
18 changes: 18 additions & 0 deletions src/lib/project-metadata/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import * as gitTargetBuilder from './target-builders/git';
import { GitTarget } from './types';

const TARGET_BUILDERS = [
gitTargetBuilder,
];

export async function getInfo(packageInfo): Promise<GitTarget|null> {
for (const builder of TARGET_BUILDERS) {
const target = await builder.getInfo(packageInfo);

if (target) {
return target;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is crossing into the specific-code territory, but while i'm here:

const TARGET_BUILDER = [gitMetadata, dockerImageMetadata];

// ...
for (const targetBuilder in TARGET_BUILDERS) {
  const target = targetBuilder.getInfo(packageInfo);
  if (target) {
    return target;
  }
}

an implementation like that lets you add new target builders (or some other term, not precious about the name) easily.

They return a target, or undefined if they can't, and you get to keep all target-specific knowledge out of this file, its responsibility becomes simply the registration and order-of-preference for each target builder.


return null;
}
33 changes: 33 additions & 0 deletions src/lib/project-metadata/target-builders/git.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import fs = require('fs');
import GitUrlParse = require('git-url-parse');

import subProcess = require('../../sub-process');
import { GitTarget } from '../types';

export async function getInfo(packageInfo): Promise<GitTarget|null> {
let origin: string|null|undefined;

if (packageInfo.docker) {
return null;
}

try {
origin = (await subProcess.execute('git', ['remote', 'get-url', 'origin'])).trim();

if (!origin) {
return null;
}

const parsedOrigin = GitUrlParse(origin);
const branch = (await subProcess.execute('git', ['rev-parse', '--abbrev-ref', 'HEAD'])).trim();

return {
remoteUrl: parsedOrigin.toString('http'),
branch,
};
} catch (err) {
// Swallowing exception since we don't want to break the monitor if there is a problem
// executing git commands.
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

can we handle it differently? maybe log reason for erroring

}
}
4 changes: 4 additions & 0 deletions src/lib/project-metadata/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
export interface GitTarget {
remoteUrl: string;
branch: string;
}
Copy link
Contributor

@gjvis gjvis May 1, 2019

Choose a reason for hiding this comment

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

this is still too hosted-git centric. A good test case (direct from https://git-scm.com/) is user@server:path/to/repo.git -> that should be easy to unambiguously represent in this GitTarget type

noticed that the hosted-git-info only supports things cloned from GitHub, GitLab, and Bitbucket so its not really appropriate here. Perhaps https://www.npmjs.com/package/git-url-parse is better? Or something else (I only searched for 2 minutes... 😂)

Maybe an https representation generated by git-url-parse is the most appropriate? there no perfect right answer here :'(

Copy link
Contributor

Choose a reason for hiding this comment

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

regarding your other comment about registry needing to do processing - I don't follow? As long as the targets are both standardised and hold the git upstream repo details then we can both group by targets and support the future desire to match up CLI with hosted source code projects in some way.

what am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, that's a good point... I think I was overcomplicating things here because my head was too hosted solution oriented...

73 changes: 73 additions & 0 deletions test/monitor-target.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
'use strict';

const test = require('tape').test;
const zlib = require('zlib');
const requestLib = require('needle');

import * as _ from 'lodash';
import * as sinon from 'sinon';

import * as cli from '../src/cli/commands';
import subProcess = require('../src/lib/sub-process');

test('Make sure that target is sent correctly', async (t) => {
const subProcessStub = sinon.stub(subProcess, 'execute');

subProcessStub.withArgs('git', ['remote', 'get-url', 'origin'])
.resolves('http:/snyk/project.git');

subProcessStub.withArgs('git', ['rev-parse', '--abbrev-ref', 'HEAD'])
.resolves('master');

const { data, spy } = await getMonitorRequestDataAndSpy();

t.true(spy.calledOnce, 'needle.request was called once');
t.true(!_.isEmpty(data.target), 'target passed to request');
t.true(!_.isEmpty(data.targetFileRelativePath), 'targetFileRelativePath passed to request');
t.equals(data.target.branch, 'master', 'correct branch passed to request');
t.equals(data.target.remoteUrl, 'http:/snyk/project.git', 'correct name passed to request');
t.equals(data.targetFileRelativePath, 'package.json', 'correct relative target file path passed to request');

subProcessStub.restore();
spy.restore();
t.end();
});

test('Make sure it\'s not failing monitor for non git projects', async (t) => {
const subProcessStub = sinon.stub(subProcess, 'execute').resolves('');
const { data, spy } = await getMonitorRequestDataAndSpy();

t.true(spy.calledOnce, 'needle.request was called once');
t.true(_.isEmpty(data.target), 'empty target passed to request');
t.equals(data.targetFileRelativePath, 'package.json', 'targetFileRelativePath passed to request');

subProcessStub.restore();
spy.restore();
t.end();
});

test('Make sure it\'s not failing if there is no remote configured', async (t) => {
const subProcessStub = sinon.stub(subProcess, 'execute').rejects();
const { data, spy } = await getMonitorRequestDataAndSpy();

t.true(spy.calledOnce, 'needle.request was called once');
t.true(_.isEmpty(data.target), 'empty target passed to request');
t.equals(data.targetFileRelativePath, 'package.json', 'targetFileRelativePath passed to request');

subProcessStub.restore();
spy.restore();
t.end();
});

async function getMonitorRequestDataAndSpy() {
const requestSpy = sinon.spy(requestLib, 'request');

await cli.monitor();

const data = JSON.parse(zlib.gunzipSync(requestSpy.getCall(0).args[2]).toString());

return {
data,
spy: requestSpy,
};
}