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: [OSM-1040] Added pnpm support under 'enablePnpmCli' feature flag #5181

Merged
merged 4 commits into from
May 10, 2024

Conversation

gemaxim
Copy link
Contributor

@gemaxim gemaxim commented Apr 19, 2024

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
image
CLI response
image

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
image
response example
image

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.

@gemaxim gemaxim requested a review from a team as a code owner April 19, 2024 07:56
@gemaxim gemaxim self-assigned this Apr 19, 2024
Copy link
Contributor

github-actions bot commented Apr 19, 2024

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

@gemaxim gemaxim changed the title feat: [OSM-1040] pnpm support feat: [OSM-1040] Added pnpm support under 'enablePnpmCli' feature flag Apr 19, 2024
@gemaxim gemaxim added the pnpm label Apr 19, 2024
validateProjectType(options, projectType);
return runTest(projectType, root, options);
validateProjectType(options, projectType, featureFlags);
return runTest(projectType, root, options, featureFlags);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@gemaxim gemaxim Apr 23, 2024

Choose a reason for hiding this comment

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

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.

[],
new Set<string>([]),
6,
);
Copy link
Member

Choose a reason for hiding this comment

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

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

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 configuration object.

@thisislawatts
Copy link
Member

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

@gemaxim gemaxim force-pushed the feat/OSM-1040/pnpm-support branch 6 times, most recently from 060879c to fdb1b13 Compare April 24, 2024 08:37
Copy link
Member

@JamesPatrickGill JamesPatrickGill left a comment

Choose a reason for hiding this comment

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

From an Open Source product perspective this PR is sound 👍

@gemaxim gemaxim force-pushed the feat/OSM-1040/pnpm-support branch 3 times, most recently from dd21adb to 4099953 Compare April 29, 2024 23:00
@gemaxim gemaxim force-pushed the feat/OSM-1040/pnpm-support branch 2 times, most recently from 317e4dd to 22b7957 Compare May 8, 2024 14:26
@gemaxim gemaxim force-pushed the feat/OSM-1040/pnpm-support branch from 22b7957 to c9519d2 Compare May 10, 2024 06:54
@gemaxim gemaxim force-pushed the feat/OSM-1040/pnpm-support branch from c9519d2 to 042c986 Compare May 10, 2024 06:56
@gemaxim gemaxim merged commit 46769cc into main May 10, 2024
15 checks passed
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.

3 participants