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

ci: ensure npm ci does not dirty package-lock via npm-force-resolutions #694

Merged
merged 1 commit into from
Jun 12, 2020

Conversation

hkang1
Copy link
Contributor

@hkang1 hkang1 commented Jun 11, 2020

Description

There are security vulnerabilities that are sometimes discovered in npm packages that are dependencies of dependencies which we do not directly control. To overcome this limitation, we us npm-force-resolutions and the resolutions field under package.json to forcibly resolve the packages to use the proper dependencies that resolved the security vulnerabilities. The problem is that the resolution process should only occur during the package installation step (npm install) but NOT during CI gathering of dependencies (npm ci). Currently it occurs in both. This PR uses a postshrinkwrap script to run npm-force-resolutions, which only occurs for npm install and not npm ci.

@hamidzr discovered this post that provides basis of this solution:
rogeriochaves/npm-force-resolutions#10 (comment)

Test Plan

Check that...

  1. npm ci does not alter any files, especially package.json and package-lock.json.
  2. npm install does run npx npm-force-resolutions (postshrinkwrap script).

Commentary (optional)

Copy link
Contributor

@hamidzr hamidzr left a comment

Choose a reason for hiding this comment

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

Thanks!

@hamidzr hamidzr assigned hkang1 and unassigned hamidzr Jun 11, 2020
@hkang1 hkang1 merged commit ffb5de0 into determined-ai:master Jun 12, 2020
@hkang1 hkang1 deleted the 3323-npm-resolution branch June 12, 2020 01:28
@dannysauer dannysauer added this to the 0.12.10 milestone Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants