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

add --frozen-lockfile option as default for yarn install #5758

Merged
merged 4 commits into from May 9, 2018

Conversation

Projects
None yet
4 participants
@UnrememberMe
Copy link
Contributor

UnrememberMe commented Apr 26, 2018

Problem

documentation from yarn:

yarn install --frozen-lockfile
Don’t generate a yarn.lock lockfile and fail if an update is needed.

Adding this option and turn it on by default in essence force all node_modules that uses yarn as package manager to have an up-to-date yarn.lock. Since the default for npm and yarn is to update the lock file when a newer semver matched dependency is published, it can leads to surprises that the actual installed codes does not match checked in lock files.

Solution

Add --frozen-lockfile errors out when lock files needed to be updated. Unfortunately, there is no such option for npm.

Result

Installation of node_module targets with yarn is more strict.

Roger Jiang
@ity
Copy link
Member

ity left a comment

a few comments ...

"""Returns command line args for installing package.
:param install_optional: True to request install optional dependencies.
:param production_only: True to only install production dependencies, i.e.
ignore devDependencies.
:param force: True to force re-download dependencies.
:param frozen_lockfile: True to disallow True to disallow automatically update lock files.

This comment has been minimized.

@ity

ity May 1, 2018

Member

s/True to disallow True to disallow automatically update lock files./True to disallow automatic updates of lock files.

This comment has been minimized.

@UnrememberMe

UnrememberMe May 1, 2018

Contributor

thanks for noticing this.

node_paths=None):
"""Returns a command that when executed will install node package.
:param install_optional: True to install optional dependencies.
:param production_only: True to only install production dependencies, i.e.
ignore devDependencies.
:param force: True to force re-download dependencies.
:param frozen_lockfile: True to disallow automatically update lock files.

This comment has been minimized.

@ity

ity May 1, 2018

Member

update

This comment has been minimized.

@UnrememberMe

UnrememberMe May 1, 2018

Contributor

done.

This comment has been minimized.

@ity

ity May 3, 2018

Member

please fix this.
True to disallow automatic update of lock files.

This comment has been minimized.

@UnrememberMe

UnrememberMe May 7, 2018

Contributor

done.

return_args = ['install']
if not install_optional:
return_args.append('--no-optional')
if production_only:
return_args.append('--production')
if force:
return_args.append('--force')
if frozen_lockfile:

This comment has been minimized.

@ity

ity May 1, 2018

Member

shouldnt this be comparing to None instead?

This comment has been minimized.

@UnrememberMe

UnrememberMe May 1, 2018

Contributor

done.

Roger Jiang
@@ -47,7 +47,7 @@ def _get_installation_args(self, install_optional, production_only, force, froze
:param production_only: True to only install production dependencies, i.e.
ignore devDependencies.
:param force: True to force re-download dependencies.
:param frozen_lockfile: True to disallow True to disallow automatically update lock files.
:param frozen_lockfile: True to disallow automatically update lock files.

This comment has been minimized.

@ity

ity May 3, 2018

Member

please fix

This comment has been minimized.

@UnrememberMe

UnrememberMe May 4, 2018

Contributor

done.

return_args = ['install']
if not install_optional:
return_args.append('--no-optional')
if production_only:
return_args.append('--production')
if force:
return_args.append('--force')
if frozen_lockfile is not None:

This comment has been minimized.

@ity

ity May 3, 2018

Member

please revert this. I was asking if its not supported for npm, why not just raise NotImplementedError

This comment has been minimized.

@UnrememberMe

UnrememberMe May 4, 2018

Contributor

done.

Not using frozen lockfile is the current implementation. Here, we are changing Yarn's behavior to be more strict. Currently this option is not exposed at target level (actually, not exposed at all as an option), target owners can not do anything to fix such any error if they choose to use npm and thus I choose to only log a warning.

node_paths=None):
"""Returns a command that when executed will install node package.
:param install_optional: True to install optional dependencies.
:param production_only: True to only install production dependencies, i.e.
ignore devDependencies.
:param force: True to force re-download dependencies.
:param frozen_lockfile: True to disallow automatically update lock files.

This comment has been minimized.

@ity

ity May 3, 2018

Member

please fix this.
True to disallow automatic update of lock files.

Roger Jiang
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented May 7, 2018

@UnrememberMe : Ity should be able to review this tomorrow; if you need it sooner, please let me know.

@ity

ity approved these changes May 9, 2018

Copy link
Member

ity left a comment

lgtm - thanks!

@ity ity merged commit d0f9118 into pantsbuild:master May 9, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment