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

Use CI friendly commands in documentation #326

Merged
merged 1 commit into from May 5, 2022
Merged

Use CI friendly commands in documentation #326

merged 1 commit into from May 5, 2022

Conversation

jonkoops
Copy link
Contributor

@jonkoops jonkoops commented Sep 3, 2021

Replaces npm install with npm ci command and adds the --frozen-lockfile flag for Yarn and PNPM installation commands. This makes installation of packages faster (for NPM) and provides guarantees about lockfile correctness at install time.

Previous efforts for this are documented in #103.

@jonkoops jonkoops mentioned this pull request Dec 24, 2021
2 tasks
@privatenumber
Copy link

privatenumber commented Jan 10, 2022

@dmitry-shibanov Anything blocking this from getting merged?

@pi9min
Copy link

pi9min commented Mar 11, 2022

+1

@@ -86,7 +86,7 @@ steps:
node-version: '14'
cache: 'npm'
cache-dependency-path: '**/package-lock.json'
- run: npm install
- run: npm ci
Copy link

@hubgit hubgit Mar 31, 2022

Choose a reason for hiding this comment

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

Does it make sense to use npm ci when the dependencies are being restored from a cache?

Copy link
Contributor Author

@jonkoops jonkoops Mar 31, 2022

Choose a reason for hiding this comment

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

I believe the cache option only caches the global cache, not the node_modules directory. So it still makes sense.

Copy link

@hubgit hubgit Mar 31, 2022

Choose a reason for hiding this comment

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

It does look like it's running npm config get cache to get the path to cache, so that does make sense, thanks 👍

@jonkoops
Copy link
Contributor Author

jonkoops commented May 3, 2022

@dmitry-shibanov @brcrista @marko-zivic-93 is there any interest in getting this merged?

@dmitry-shibanov
Copy link
Contributor

dmitry-shibanov commented May 3, 2022

Hello @jonkoops. Thank you for your pull request. I think we can adjust these changes when caching is enabled, because it relies on hash from dependency files. For other cases I think we should leave it as npm install because some repositories do not have package-lock.json.

@jonkoops jonkoops requested a review from a team as a code owner May 4, 2022
@jonkoops
Copy link
Contributor Author

jonkoops commented May 4, 2022

For other cases I think we should leave it as npm install because some repositories do not have package-lock.json.

I think differently about this matter, checking in lockfiles is considered best practice for all package managers. Instead of appeasing users that might not have checked this file in we should encourage them to follow these practices.

For example, the NPM documentation says the following about the package-lock.json file:

This file is intended to be committed into source repositories, and serves various purposes:

  • Describe a single representation of a dependency tree such that teammates, deployments, and continuous integration are guaranteed to install exactly the same dependencies.
  • Provide a facility for users to "time-travel" to previous states of node_modules without having to commit the directory itself.
  • Facilitate greater visibility of tree changes through readable source control diffs.
  • Optimize the installation process by allowing npm to skip repeated metadata resolutions for previously-installed packages.
  • As of npm v7, lockfiles include enough information to gain a complete picture of the package tree, reducing the need to read package.json files, and allowing for significant performance improvements.

All of these points apply directly to Github and Github Actions and I believe that these best practices should be reflected here as well.

I propose that all commands are replaced with their appropriate CI friendly versions as per the original purpose of this PR and that the documentation is amended to have a dedicated section to lockfiles.

Replaces `npm install` with `npm ci` command and adds the `--frozen-lockfile` flag for Yarn and PNPM installation commands.
@jonkoops
Copy link
Contributor Author

jonkoops commented May 4, 2022

I've added some documentation about lockfiles as mentioned above.

@privatenumber
Copy link

privatenumber commented May 4, 2022

For an official action like this, I think it's important to use examples that adopt best practices.

There are many beginners that read these examples that are not aware of the trade-offs between npm install and npm ci.

The problem with using npm install or not using package-lock.json is that there's a chance their CI will unexpectedly fail because of inconsistent dependencies.

For example, a new dependency update that satisfies the package.json semver range can be installed on the CI before the developer tests it locally and fail (or even worse, fail silently and trigger a release).

npm ci was designed for Continuous Integration to prevent these problems.

@dmitry-shibanov dmitry-shibanov merged commit b067f78 into actions:main May 5, 2022
1 check passed
@dmitry-shibanov
Copy link
Contributor

dmitry-shibanov commented May 5, 2022

Thank you @jonkoops for improving the documentation and your work.

@jonkoops jonkoops deleted the feature/ci-friendly-commands branch May 8, 2022
@jonkoops
Copy link
Contributor Author

jonkoops commented May 8, 2022

Thanks for taking this into consideration and merging it!

jablko pushed a commit to jablko/setup-node that referenced this pull request May 10, 2022
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.

None yet

5 participants