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

feat: enable paket to be autodetected with --all-projects #978

Merged
merged 1 commit into from
Jan 28, 2020
Merged

feat: enable paket to be autodetected with --all-projects #978

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
1 change: 1 addition & 0 deletions src/lib/detect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export const AUTO_DETECTABLE_FILES: string[] = [
'Gemfile.lock',
'pom.xml',
'packages.config',
'paket.dependencies',
Copy link
Contributor

Choose a reason for hiding this comment

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

paket.lock needed here also?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only detect paket.dependencies, we won't do paket.lock by itself (the plugin will detect it)

Copy link
Contributor

Choose a reason for hiding this comment

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

But why then Gemfile we detect both? And for npm we detect package-lock.json and for yarn yarn.lock?

Copy link
Contributor

Choose a reason for hiding this comment

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

Gemfile is good point - plugin seems to throw error if no Gemfile.lock is found, so should Gemfile be in this list? I think npm plugin can handle either package-lock.json or package.json so that's ok

Copy link
Contributor

Choose a reason for hiding this comment

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

Gemfile is not really needed no, the rest is all determined by the plugin really and what it can find itself and what it needs to be detected by us. Me and Daniel spoke about this in a different PR, we can go back and tidy up some of these afterwards.

Copy link
Contributor

Choose a reason for hiding this comment

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

for ruby we find the Gemfile.lock later in the plugin so just Gemfile will be fine, see `gatherSpecs. Let's keep that a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, just wanted to clarify. We should do a tidy up PR afterward

'project.json',
'project.assets.json',
'Podfile',
Expand Down
74 changes: 68 additions & 6 deletions test/acceptance/cli-monitor/cli-monitor.all-projects.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ export const AllProjectsTests: AcceptanceTests = {
t.ok(spyPlugin.withArgs('rubygems').calledOnce, 'calls rubygems plugin');
t.ok(spyPlugin.withArgs('npm').calledOnce, 'calls npm plugin');
t.ok(spyPlugin.withArgs('maven').calledOnce, 'calls maven plugin');
t.ok(spyPlugin.withArgs('nuget').calledOnce, 'calls nuget plugin');
t.ok(spyPlugin.withArgs('paket').calledOnce, 'calls nuget plugin');
Copy link
Contributor

Choose a reason for hiding this comment

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

calls paket plugin

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 paket plugin is the nuget plugin

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok 👍


// npm
t.match(
result,
Expand All @@ -38,18 +41,39 @@ export const AllProjectsTests: AcceptanceTests = {
'rubygems/some/project-id',
'rubygems project was monitored',
);

// nuget
t.match(result, 'nuget/some/project-id', 'nuget project was monitored');
// paket
t.match(result, 'paket/some/project-id', 'paket project was monitored');
// maven
t.match(result, 'maven/some/project-id', 'maven project was monitored ');
// Pop all calls to server and filter out calls to `featureFlag` endpoint
const requests = params.server
.popRequests(4)
.popRequests(6)
.filter((req) => req.url.includes('/monitor/'));
t.equal(requests.length, 3, 'Correct amount of monitor requests');
t.equal(requests.length, 5, 'Correct amount of monitor requests');

const pluginsWithoutTragetFilesInBody = [
'snyk-nodejs-lockfile-parser',
'bundled:maven',
'bundled:rubygems',
];

requests.forEach((req) => {
t.match(req.url, '/monitor/', 'puts at correct url');
t.notOk(req.body.targetFile, "doesn't send the targetFile");
Copy link
Contributor

@lili2311 lili2311 Jan 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

maybe it's time we assert the relevant ones send the targetFile? I am not a fan of removing this because now we have something that will send it rather than change the assertion so we have introduced a blind spot for this plugin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I will add plugin specific check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

if (
pluginsWithoutTragetFilesInBody.includes(req.body.meta.pluginName)
) {
t.notOk(
req.body.targetFile,
`doesn\'t send the targetFile for ${req.body.meta.pluginName}`,
);
} else {
t.ok(
req.body.targetFile,
`does send the targetFile ${req.body.meta.pluginName}`,
);
}
t.equal(req.method, 'PUT', 'makes PUT request');
t.equal(
req.headers['x-snyk-cli-version'],
Expand Down Expand Up @@ -147,8 +171,22 @@ export const AllProjectsTests: AcceptanceTests = {
detectionDepth: 1,
});
// Pop all calls to server and filter out calls to `featureFlag` endpoint
const [rubyAll, npmAll, mavenAll] = params.server
.popRequests(4)
const [
rubyAll,
npmAll,
nugetAll,
paketAll,
mavenAll,
] = params.server
.popRequests(6)
.filter((req) => req.url.includes('/monitor/'));

// nuget
await params.cli.monitor('mono-repo-project', {
file: 'packages.config',
});
const [requestsNuget] = params.server
.popRequests(2)
.filter((req) => req.url.includes('/monitor/'));

// Ruby
Expand All @@ -175,6 +213,14 @@ export const AllProjectsTests: AcceptanceTests = {
.popRequests(2)
.filter((req) => req.url.includes('/monitor/'));

// paket
await params.cli.monitor('mono-repo-project', {
file: 'paket.dependencies',
});
const [requestsPaket] = params.server
.popRequests(2)
.filter((req) => req.url.includes('/monitor/'));

// Ruby project

t.deepEqual(
Expand All @@ -191,13 +237,29 @@ export const AllProjectsTests: AcceptanceTests = {
'Same body for --all-projects and --file=package-lock.json',
);

// NUGET project

t.deepEqual(
nugetAll.body,
requestsNuget.body,
'Same body for --all-projects and --file=packages.config',
);

// Maven project

t.deepEqual(
mavenAll.body,
requestsMaven.body,
'Same body for --all-projects and --file=pom.xml',
);

// Paket project

t.deepEqual(
paketAll.body,
requestsPaket.body,
'Same body for --all-projects and --file=paket.dependencies',
);
},
'`monitor mono-repo-project with lockfiles --all-projects --json`': (
params,
Expand Down
31 changes: 28 additions & 3 deletions test/acceptance/cli-test/cli-test.all-projects.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,10 @@ export const AllProjectsTests: AcceptanceTests = {
t.ok(spyPlugin.withArgs('rubygems').calledOnce, 'calls rubygems plugin');
t.ok(spyPlugin.withArgs('npm').calledOnce, 'calls npm plugin');
t.ok(spyPlugin.withArgs('maven').calledOnce, 'calls maven plugin');
t.ok(spyPlugin.withArgs('nuget').calledOnce, 'calls nuget plugin');
t.ok(spyPlugin.withArgs('paket').calledOnce, 'calls nuget plugin');
Copy link
Contributor

Choose a reason for hiding this comment

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

calls paket plugin


params.server.popRequests(3).forEach((req) => {
params.server.popRequests(5).forEach((req) => {
t.equal(req.method, 'POST', 'makes POST request');
t.equal(
req.headers['x-snyk-cli-version'],
Expand All @@ -33,7 +35,7 @@ export const AllProjectsTests: AcceptanceTests = {
t.ok(req.body.depGraph, 'body contains depGraph');
t.match(
req.body.depGraph.pkgManager.name,
/(npm|rubygems|maven)/,
/(npm|rubygems|maven|nuget|paket)/,
'depGraph has package manager',
);
});
Expand Down Expand Up @@ -86,14 +88,26 @@ export const AllProjectsTests: AcceptanceTests = {
const [
rubyAllProjectsBody,
npmAllProjectsBody,
nugetAllProjectsBody,
paketAllProjectsBody,
mavenAllProjectsBody,
] = params.server.popRequests(3).map((req) => req.body);
] = params.server.popRequests(5).map((req) => req.body);

await params.cli.test('mono-repo-project', {
file: 'Gemfile.lock',
});
const { body: rubyFileBody } = params.server.popRequest();

await params.cli.test('mono-repo-project', {
file: 'paket.dependencies',
});
const { body: paketFileBody } = params.server.popRequest();

await params.cli.test('mono-repo-project', {
file: 'packages.config',
});
const { body: nugetFileBody } = params.server.popRequest();

await params.cli.test('mono-repo-project', {
file: 'package-lock.json',
});
Expand All @@ -116,6 +130,17 @@ export const AllProjectsTests: AcceptanceTests = {
'Same body for --all-projects and --file=package-lock.json',
);

t.same(
paketAllProjectsBody,
paketFileBody,
'Same body for --all-projects and --file=package-lock.json',
);

t.same(
nugetAllProjectsBody,
nugetFileBody,
'Same body for --all-projects and --file=package-lock.json',
);
t.same(
mavenAllProjectsBody,
mavenFileBody,
Expand Down
4 changes: 4 additions & 0 deletions test/acceptance/workspaces/mono-repo-project/packages.config
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
<?xml version="1.0" encoding="utf-8"?>
<packages>
<package id="Microsoft.AspNet.Mvc" version="5.2.3" targetFramework="net45" />
</packages>
5 changes: 5 additions & 0 deletions test/acceptance/workspaces/mono-repo-project/paket.dependencies
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
redirects: on
source https://nuget.org/api/v2

nuget FSharp.Formatting
nuget FAKE
10 changes: 10 additions & 0 deletions test/acceptance/workspaces/mono-repo-project/paket.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
REDIRECTS: ON
NUGET
remote: https://www.nuget.org/api/v2
FAKE (5.8.4)
FSharp.Compiler.Service (2.0.0.6)
FSharp.Formatting (2.14.4)
FSharp.Compiler.Service (2.0.0.6)
FSharpVSPowerTools.Core (>= 2.3 < 2.4)
FSharpVSPowerTools.Core (2.3)
FSharp.Compiler.Service (>= 2.0.0.3)