feat: [OSM-1040] Added pnpm support under 'enablePnpmCli' feature flag

[gemaxim]

Pull Request Submission

Please check the boxes once done.

The pull request must:

• Reviewer Documentation
  • follow CONTRIBUTING rules
  • be accompanied by a detailed description of the changes
  • contain a risk assessment of the change (Low | Medium | High) with regards to breaking existing functionality. A change e.g. of an underlying language plugin can completely break the functionality for that language, but appearing as only a version change in the dependencies.
  • highlight breaking API if applicable
  • contain a link to the automatic tests that cover the updated functionality.
  • contain testing instructions in case that the reviewer wants to manual verify as well, to add to the manual testing done by the author.
  • link to the link to the PR for the User-facing documentation
• User facing Documentation
  • update any relevant documentation in gitbook by submitting a gitbook PR, and including the PR link here
  • ensure that the message of the final single commit is descriptive and prefixed with either feat: or fix: , others might be used in rare occasions as well, if there is no need to document the changes in the release notes. The changes or fixes should be described in detail in the commit message for the changelog & release notes.
• Testing
  • Changes, removals and additions to functionality must be covered by acceptance / integration tests or smoke tests - either already existing ones, or new ones, created by the author of the PR.

Pull Request Review

All pull requests must undergo a thorough review process before being merged.
The review process of the code PR should include code review, testing, and any necessary feedback or revisions.
Pull request reviews of functionality developed in other teams only review the given documentation and test reports.

Manual testing will not be performed by the reviewing team, and is the responsibility of the author of the PR.

For Node projects: It’s important to make sure changes in package.json are also affecting package-lock.json correctly.

If a dependency is not necessary, don’t add it.

When adding a new package as a dependency, make sure that the change is absolutely necessary. We would like to refrain from adding new dependencies when possible.
Documentation PRs in gitbook are reviewed by Snyk's content team. They will also advise on the best phrasing and structuring if needed.

Pull Request Approval

Once a pull request has been reviewed and all necessary revisions have been made, it is approved for merging into
the main codebase. The merging of the code PR is performed by the code owners, the merging of the documentation PR
by our content writers.

What does this PR do?

Adds pnpm support for test and monitor. The whole functionality is available under the 'enablePnpmCli' registry feature flag.

The feature flag is retrieved through the v1/cli-config/feature-flags endpoint at the beginning of the dependencies retrieval process (through a function was already implemented). The aims is that, with the feature flag not enabled, we don't retrieve any manifest files specific to pnpm or do any operations specific for pnpm (e.q. inspect and workspaces).

Wit feature flag enabled, 'pnpm-lock.yaml' lockfiles are also retrieved as target files.

For pnpm, the feature flag enables a flow similar to npm and yarn. No changes were made to the nodejs-plugin inside the CLI. The functionality was extracted in an open-sourced plugin: https://github.com/snyk/snyk-nodejs-plugin. Only inspect with pnpm and processPnpmWorkspaces are called for this repo inside the cli for now. On a later iteration, we should be able to completely remove https://github.com/snyk/cli/tree/main/src/lib/plugins/nodejs-plugin.

For multiple projects (with --all-projects), without feature flag, the functionality remains the same. With feature flag enabled, pnpm workspaces are processed first, then yarn for the remaining unprocessed files and then npm.

All other local flags (--dev, --strictOutOfSync, ...) used with yarn can be used with pnpm.

Where should the reviewer start?

Maybe look at the backgroud context.
See pnpm tests. Changes in legacy tap tests are due to another makeRequest call being performed at the beggining of the test and monitor flow, for retrieving the 'enablePnpmCli' feature flag.

No feature flag functionality is covered by existing tests, making sure nothing breaks (especially npm and yarn).
Pnpm tests: https://github.com/snyk/cli/pull/5181/files#diff-a2394b27276334b82b65cdd5e24f336a54b2c4e8f5c896eb894c8c00922554d4 and https://github.com/snyk/cli/pull/5181/files#diff-8d21d95cfc87ecfb764610e6b84fdc4810ecd351e76a34120e2137f387ec34e7.

Other pnpm functionality covered in nodejs-plugin tests: https://github.com/snyk/snyk-nodejs-plugin/tree/main/test and nodejs-lockfile-parser: https://github.com/snyk/nodejs-lockfile-parser/tree/master/test/jest/dep-graph-builders/fixtures see pnpm-lock-v5 and pnpm-lock-v5 fixtures.

How should this be manually tested?

I added some tests for pnpm project for the following scenarios:

• feature flag enabled, single pnpm project: lockfile 'pnpm-lock.yaml' is parsed and dep graph, 'pnpm' is returned as package manager
• feature flag enabled, --all-projects flag: pnpm, yarn and npm workspaces are processed and returned
• feature flag disabled, single pnpm project: 'package.json' file is processed as part of the npm flow
• feature flag disabled, --all-projects flag: only npm and yarn workspaces are processed

Locally: enable feature flag for organization.
Run 'snyk test' or 'snyk monitor' for a project having 'package.json' and 'pnpm-lock.yaml' files.
Monitor UI view

CLI response

Run 'snyk test --all-projects' or 'snyk monitor --all-projects' for workspaces. pnpm workspaces need to have 'pnpm-workspace.yaml' file in the root of the workspace.
debug logs example

response example

Any background context you want to provide?

Pitch: https://snyksec.atlassian.net/wiki/spaces/RD/pages/1922334813/pnpm+CLI+support
Lockfile parser supporting pnpm lockfiles v5 and v6: snyk/nodejs-lockfile-parser#218
Nodejs plugin calling the nodejs-lockfile-parser: https://github.com/snyk/snyk-nodejs-plugin
New plugin for pnpm projects in UI for monitor: https://github.com/snyk/app-ui/pull/3400.
Vuln changes to cover retrieval of issues for pnpm: https://github.com/snyk/vuln/pull/844
Registry changes to support pnpm on /v1/test-dep-graph and /v1/monitor: https://github.com/snyk/registry/pull/35561 (writing ADR atm).

What are the relevant tickets?

https://snyksec.atlassian.net/browse/OSM-1040

Additional questions

Since this is under a registry feature flag, should the cli docs for test and monitor be updated at the moment? I believe only once it goes open beta (anyone can enable the registry ff), but correct me if I'm wrong.

[github-actions]

Warnings

⚠️

Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones. A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax. Files found: src/cli/args.ts src/cli/commands/monitor/index.ts src/lib/plugins/get-deps-from-plugin.ts src/lib/plugins/get-single-plugin-result.ts test/tap/monitor-target.test.ts

Generated by 🚫 dangerJS against 042c986

[thisislawatts]

suggestion: It seems like we could short circuit here if the customer does not have the necessary access to the pnpm scanner, this could be based on feature flags or entitlements.

[gemaxim]

Did this. Still need to pass on the featureFlag to runTest for later computations (e.g. workspaces), but removed those conditions for the plugin load.

[thisislawatts]

suggestion: Switch to configuration object as we introduce more params. easier to reason with and makes default values simple to use.

[gemaxim]

Added configuration object.

[thisislawatts]

suggestion: Introduce an acceptance test which validates the flow by running the binary as a "closed box" test.

[JamesPatrickGill]

From an Open Source product perspective this PR is sound 👍