feat: add git metadata to monitor payload #462

odinn1984 · 2019-04-30T12:40:16Z

Ready for review
Follows CONTRIBUTING rules
Reviewed by Snyk internal team

What does this PR do?

We are adding git metadata to monitor when sending the payload to snyk.io for grouping CLI projects

darscan · 2019-04-30T13:09:26Z

src/lib/project-metadata/git.ts

+  };
+}
+
+function findGitRoot(): string {


please consider git rev-parse --show-toplevel

lili2311 · 2019-04-30T13:19:29Z

src/lib/monitor.ts

      analytics.add('packageManager', packageManager);
+
+      const gitTarget = await projectMetadata.getInfo('git', root, targetFile) as GitInfo;


Can this targetfile ever be different to the https://github.com/snyk/snyk/pull/462/files#diff-e2e0f6cf5732ba336dc3ba0de17258faR88?

Will it matter? Not all plugins send targetFile and some choose what is best to process in the end

@lili2311 the difference here would be that this is the relative path to the target file (relative to the git project root) and we pass it to monitor later in the chain. i think it would make more sense to present the file in this way in snyk.io. wdyt? basically meaning that if u have a monorepo for example we will save the target file as the relative path from the top level dir to the file even if ur running it inside of a sub dir

What happens when a user runs a snyk test with a file ../../somefile? After checking analytics this happens today!

gjvis

See inline, plus one general comment:

whatever we do here needs to be very easy to work into test and protect as well, as this data will be used very soon as part of the "locate the relevant project" code server-side

gjvis · 2019-04-30T16:50:41Z

src/lib/project-metadata/types.ts

@@ -0,0 +1,7 @@
+export interface GitInfo {


It think this should be a GitTarget, and should not include commitSha or targetFile, as those aren't part of the identify where the canonical place the source code lives.

gjvis · 2019-04-30T16:51:43Z

src/lib/project-metadata/types.ts

@@ -0,0 +1,7 @@
+export interface GitInfo {
+  repo: string;
+  owner: string;


owner is a GitHub specific thing, git itself does not have this concept.

gjvis · 2019-04-30T16:53:09Z

src/lib/project-metadata/types.ts

@@ -0,0 +1,7 @@
+export interface GitInfo {
+  repo: string;


perhaps remoteUrl would be better?

also, we need to normalise this, e.g. always using the ssh url, so maybe "remoteSSHUrl` or similar (depending on how we choose to normalise it)

if we do a remote url (which was origin URL in michael's original PR) it means that we're leaving the parsing part to registry to build the target... or we can just use the URL somehow to represent the project at registry?

gjvis · 2019-04-30T16:59:43Z

src/lib/project-metadata/index.ts

+import * as gitMetadata from './git';
+import { GitInfo } from './types';
+
+export async function getInfo(metadataSource: string, root: string, targetFile: string): Promise<GitInfo|null> {


I think this needs to return just the target, and potentially some monitor meta (although that happens elsewhere already, right?)

Its weird that this component has to be told what the metadataSource is, surely this needs to happen before we get to the command and work out based on the CLI args, the env, the process.pwd(), etc. what the metadataSource and target are?

Related to 2, how would we work command-line based overrides into this structure? E.g. a git remote-url specified on the command line would still need to be normalised, which is going to end up in a completely different place to this? If the broad boundary is "CLI-args, env, process.pwd() as input -> target as output" then command line overrides would be possible too

We also need to decide how we cope with situations where the "root" directory here changes based on us being able to inspect the git repo metadata, right? That change (and any resulting impact on any --file or similar argument) needs to be propagated to the plugin somehow

yes the monitor is responsible for adding the monitor data, I wanted this factory to return only target related data
2 and 3. so would it make sense to do something like... does a git command work then it's git if not try to see if we have a docker argument etc?

if the root dir changes that's fine no since we're just using it to get the relative path to the target file?

gjvis · 2019-04-30T17:00:12Z

src/lib/monitor.ts

      analytics.add('packageManager', packageManager);
+
+      const gitTarget = await projectMetadata.getInfo('git', root, targetFile) as GitInfo;


see other comment, but commenting here too: how does this know already that this is git?

gjvis

Interesting to put this after the plugins have done their thing, seems a better direction.

Another question: should we make sure that the targetFile we're passing up is relative to the root directory of the git repo, if there is one?

(@lili2311 your thoughts on this seem relevant too given your familiarity with the CLI and targetFiles, etc.?).

gjvis · 2019-05-01T15:46:59Z

src/lib/monitor.ts

@@ -74,6 +81,7 @@ export function monitor(root, meta, info: SingleDepRootResult): Promise<any> {
              dockerImageId: pluginMeta.dockerImageId,
              dockerBaseImage: pkg.docker ? pkg.docker.baseImage : undefined,
              projectName: meta['project-name'],
+              projectTarget,


I think this should be alongside targetFile, in the top-level body, and called plain old target (to closely match the agent-side of things)

gjvis · 2019-05-01T16:09:27Z

src/lib/project-metadata/types.ts

+  user: string;
+  project: string;
+  branch: string;
+}


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 :'(

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?

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

gjvis · 2019-05-01T16:16:26Z

src/lib/project-metadata/index.ts

+    return null;
+  } else if ((await subProcess.execute('git', ['remote', 'get-url', 'origin'])).trim()) {
+    return gitMetadata.getInfo();
+  }


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.

Comments addressed, discussion has moved on

lili2311 · 2019-05-02T11:59:33Z

@gjvis @odinn1984

The plugin will always return a file that was used in the end to generate the Tree, the plugin has better understanding to do this.

However not all plugins return it relevant to the git root of course.

I think if we for now send what you propose we can then check the data for discrepancies. For example some users run test via snyk test ../../somefile What will this translate to in the git target?

Do we have a test for this?

odinn1984 · 2019-05-02T13:49:04Z

@lili2311 this would be translated to the relative path from the root if we chose to send that, the question here is if it's useful to send that as well.

We're not in the tests stage yet, I will add tests once all the questions are answered :)

ArturSnyk · 2019-05-06T13:19:30Z

src/lib/project-metadata/target-builders/git.ts

+import subProcess = require('../../sub-process');
+import { GitTarget } from '../types';
+
+/* tslint:disable:no-unused-variable */


is this necessary?

ArturSnyk · 2019-05-06T13:20:33Z

src/lib/project-metadata/target-builders/git.ts

+      branch,
+    };
+  } catch (err) {
+    return null;


can we handle it differently? maybe log reason for erroring

jonathan-fielding · 2019-05-07T13:47:08Z

src/lib/monitor.ts

@@ -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) => {


why add async

this is returning a promise not using async await

@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.

odinn1984 added 🌹 enhancement 🐣 in progress ⚠️ DO NOT MERGE ⚠️ labels Apr 30, 2019

odinn1984 self-assigned this Apr 30, 2019

darscan reviewed Apr 30, 2019

View reviewed changes

lili2311 reviewed Apr 30, 2019

View reviewed changes

odinn1984 force-pushed the feat/send-gitinfo-on-monitor branch from 54d5719 to 803a28f Compare April 30, 2019 14:19

gjvis previously requested changes Apr 30, 2019

View reviewed changes

gjvis reviewed May 1, 2019

View reviewed changes

odinn1984 force-pushed the feat/send-gitinfo-on-monitor branch 3 times, most recently from 62dc4d2 to c7920d3 Compare May 6, 2019 13:14

ArturSnyk reviewed May 6, 2019

View reviewed changes

odinn1984 closed this May 6, 2019

odinn1984 reopened this May 6, 2019

odinn1984 closed this May 6, 2019

odinn1984 reopened this May 6, 2019

odinn1984 force-pushed the feat/send-gitinfo-on-monitor branch 4 times, most recently from ef74bd0 to 368329f Compare May 7, 2019 07:40

odinn1984 removed 🐣 in progress ⚠️ DO NOT MERGE ⚠️ labels May 7, 2019

odinn1984 requested a review from gjvis May 7, 2019 08:22

feat: send target data on monitor to registry

868a907

odinn1984 force-pushed the feat/send-gitinfo-on-monitor branch from 368329f to 868a907 Compare May 7, 2019 12:45

jonathan-fielding reviewed May 7, 2019

View reviewed changes

This was referenced Mar 9, 2022

[Snyk] Upgrade debug from 2.2.0 to 2.6.9 #2879

Closed

[Snyk] Upgrade debug from 2.2.0 to 2.6.9 #2989

Closed

snyk-bot mentioned this pull request Jul 27, 2022

[Snyk-dev] Security upgrade debug from 1.0.5 to 2.6.9 #3503

Closed

snyk-bot mentioned this pull request Aug 30, 2022

[Snyk] Upgrade debug from 2.2.0 to 2.6.9 #3695

Closed

snyk-bot mentioned this pull request Sep 30, 2022

[Snyk] Security upgrade debug from 1.0.5 to 2.6.9 #3886

Closed

soniqua mentioned this pull request Sep 30, 2022

[Snyk] Security upgrade debug from 2.2.0 to 2.6.9 #3934

Closed

This was referenced Sep 30, 2022

[Snyk] Fix for 3 vulnerabilities #3976

Closed

[Snyk-dev] Security upgrade debug from 2.2.0 to 2.6.9 #4068

Closed

allRounderWiser mentioned this pull request Oct 6, 2022

[Snyk] Security upgrade debug from 2.2.0 to 2.6.9 allRounderWiser/snyk#135

Open

leongold mentioned this pull request Nov 30, 2022

[Snyk] Fix for 5 vulnerabilities leongold/snyk#25

Open

This was referenced Nov 30, 2022

[Snyk] Fix for 7 vulnerabilities mikolajroszak/mikolaj.com-snyk#38

Open

[Snyk] Fix for 6 vulnerabilities allRounderWiser/snyk#153

Open

david-j-bailey mentioned this pull request Nov 30, 2022

[Snyk] Fix for 7 vulnerabilities david-j-bailey/snyk#35

Open

snyk-bot mentioned this pull request Nov 30, 2022

[Snyk] Fix for 7 vulnerabilities carrot1213/cli#10

Open

yinhaijiao mentioned this pull request Nov 30, 2022

[Snyk] Fix for 7 vulnerabilities yinhaijiao/snyk#43

Open

This was referenced Dec 1, 2022

[Snyk] Fix for 6 vulnerabilities thexdesk/snyk#14

Open

[Snyk] Fix for 7 vulnerabilities cloudgeek7/snyk#394

Open

froscheimer mentioned this pull request Dec 1, 2022

[Snyk] Fix for 6 vulnerabilities froscheimer/snyk#186

Open

sapans30 mentioned this pull request Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities sapans30/snyk#36

Open

This was referenced Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities dawit-snyk/snyk#15

Open

[Snyk] Fix for 7 vulnerabilities jonesjim/cli#8

Open

hiep98 mentioned this pull request Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities hiep98/snyk#28

Open

This was referenced Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities MaxMood96/snyk#197

Open

[Snyk] Fix for 7 vulnerabilities toharzand/snyk#41

Open

[Snyk] Fix for 3 vulnerabilities RyanPHigh/snyk#157

Open

romathur mentioned this pull request Dec 1, 2022

[Snyk] Fix for 5 vulnerabilities romathur/snyk#147

Open

This was referenced Dec 1, 2022

[Snyk] Fix for 6 vulnerabilities mmmd03/snyk#19

Open

[Snyk] Fix for 7 vulnerabilities laozhudetui/snyk#41

Open

RotemMiara mentioned this pull request Dec 1, 2022

[Snyk] Fix for 7 vulnerabilities RotemMiara/cli#13

Open

robin-qualityengineer mentioned this pull request Dec 2, 2022

[Snyk] Fix for 6 vulnerabilities robin-qualityengineer/snyk#239

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add git metadata to monitor payload #462

feat: add git metadata to monitor payload #462

odinn1984 commented Apr 30, 2019

darscan Apr 30, 2019

odinn1984 Apr 30, 2019

lili2311 Apr 30, 2019

odinn1984 Apr 30, 2019

lili2311 May 2, 2019

gjvis left a comment

gjvis Apr 30, 2019

gjvis Apr 30, 2019

gjvis Apr 30, 2019

odinn1984 May 1, 2019

gjvis Apr 30, 2019 •

edited

odinn1984 May 1, 2019

gjvis Apr 30, 2019

gjvis left a comment

gjvis May 1, 2019

gjvis May 1, 2019 •

edited

gjvis May 1, 2019

odinn1984 May 2, 2019

gjvis May 1, 2019

lili2311 commented May 2, 2019

odinn1984 commented May 2, 2019

ArturSnyk May 6, 2019

ArturSnyk May 6, 2019

jonathan-fielding May 7, 2019

jonathan-fielding May 7, 2019

odinn1984 May 7, 2019

		analytics.add('packageManager', packageManager);

		const gitTarget = await projectMetadata.getInfo('git', root, targetFile) as GitInfo;

feat: add git metadata to monitor payload #462

feat: add git metadata to monitor payload #462

Conversation

odinn1984 commented Apr 30, 2019

What does this PR do?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjvis Apr 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjvis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjvis May 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lili2311 commented May 2, 2019

odinn1984 commented May 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjvis Apr 30, 2019 •

edited

gjvis May 1, 2019 •

edited