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

Lockfile autodetection for test, wizard, protect and monitor #228

Merged
merged 9 commits into from
Oct 8, 2018

Conversation

lili2311
Copy link
Contributor

@lili2311 lili2311 commented Oct 2, 2018

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

What does this PR do?

  • Introduce yarn & package-lock.json autodetection to use lockfile only (without node_modules traversal)
  • Leave node_module traversal as fall back for package.json & shrinkwrap projects

The state of NPM lockfile support in this pr:

  • projects with package.json & package-lock.json will be tested via the new lockfile parser lib for snyk protect, test, monitor

The state of Yarn lockfile support in this pr:

  • projects with package.json & yarn.lock will be tested via the new lockfile parser lib for snyk test, monitor
  • projects with package.json & yarn.lock will be tested via the old node_modules traversal lib (resolve-deps) for snyk protect and wizard
  • projects with package.json & yarn.lock on node@4 will be tested via the old node_modules traversal lib (resolve-deps) for all snyk commands. Yarn parser lib we use is not compatible with node@4

Where should the reviewer start?

  • most changes are in the npm & yarn plugin to test if a project is lockfile based before choosing which way to generate the tree

How should this be manually tested?

  • run test, monitor, protect and wizard locally on a lockfile based project

What are the relevant tickets?

https://snyksec.atlassian.net/browse/SC-6240
https://snyksec.atlassian.net/browse/SC-6277

@lili2311 lili2311 self-assigned this Oct 2, 2018
@lili2311 lili2311 force-pushed the fix/lockfiles-monitor branch 2 times, most recently from 97535e1 to 5a32fd3 Compare October 2, 2018 10:49
@lili2311
Copy link
Contributor Author

lili2311 commented Oct 2, 2018

WORK IN PROGRESS
Need to add some changes to wizard to better support this

if (!nodeModulesExist) {
// throw a custom error
throw new Error(
'Missing node_modules folder: we can\'t patch without having installed packages.' +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

test can now be performed without node_modules so change the wording to better reflect why we need node_modules during wizard for all node lockfiles & non lockfiles project types

if (isLockFileBased) {
// we need to trigger a lockfile update after adding snyk
// as a dep
return protect.update(['snyk'], live, packageManager);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

if a project is lockfile based we need to also update the lockfile after adding in snyk as a dep, without this the files go out of sync and fail a re-test on monitor command

@@ -10,6 +10,7 @@ var chalk = require('chalk');

var DETECTABLE_FILES = [
'yarn.lock',
'package-lock.json',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

change order to ensure lockfile based projects get processed as suck

// but not the latest node-lockfile-parser
// HACK: if yarn set traverseNodeModules option to
// bypass lockfile test for wizard
options.traverseNodeModules = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fallback for yarn projects where we currently do not support traversing node_modules with the new lockfile-parser

// but not the latest node-lockfile-parser
// HACK: if yarn set traverseNodeModules option to
// bypass lockfile test for wizard
if (targetFile.endsWith('yarn.lock')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fallback for yarn projects where we currently do not support traversing node_modules with the new lockfile-parser

@miiila
Copy link
Contributor

miiila commented Oct 4, 2018

Great job, waiting for tests to pass before final approvement.

Copy link
Contributor

@miiila miiila left a comment

Choose a reason for hiding this comment

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

just a small comment for now :-)

return snyk.test(cwd, options).then(function (res) {
if (alerts.hasAlert('tests-reached') && res.isPrivate) {
return;
var intro = __dirname + '/../../../../help/wizard-intro.txt';
Copy link
Contributor

Choose a reason for hiding this comment

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

- var
+ const

@lili2311 lili2311 force-pushed the fix/lockfiles-monitor branch 7 times, most recently from d41a593 to 81f337e Compare October 5, 2018 15:19
@lili2311 lili2311 deleted the fix/lockfiles-monitor branch October 8, 2018 13:52
@snyksec
Copy link

snyksec commented Oct 8, 2018

🎉 This PR is included in version 1.102.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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants