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: enable paket to be autodetected with --all-projects #978

Merged
merged 1 commit into from
Jan 28, 2020

Conversation

orsagie
Copy link
Contributor

@orsagie orsagie commented Jan 27, 2020

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

Enables paket projets to be autodetected with the --all-projects option set.

@orsagie orsagie requested a review from a team as a code owner January 27, 2020 20:57
@orsagie orsagie self-assigned this Jan 27, 2020
@ghost ghost requested review from gitphill and lili2311 January 27, 2020 20:57
@@ -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

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

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


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

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

@orsagie orsagie force-pushed the feat/add-paket-to-all-projects branch from 8026454 to 3cf8799 Compare January 28, 2020 12:57
@orsagie orsagie force-pushed the feat/add-paket-to-all-projects branch from 3cf8799 to db34336 Compare January 28, 2020 13:23
@orsagie orsagie merged commit a295643 into master Jan 28, 2020
@orsagie orsagie deleted the feat/add-paket-to-all-projects branch January 28, 2020 15:13
@snyksec
Copy link

snyksec commented Jan 28, 2020

🎉 This PR is included in version 1.286.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants