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

Yarn: run with --frozen-lockfile #14640

Merged
merged 1 commit into from
Feb 12, 2020
Merged

Yarn: run with --frozen-lockfile #14640

merged 1 commit into from
Feb 12, 2020

Conversation

simison
Copy link
Member

@simison simison commented Feb 11, 2020

I haven't dug very much into this but I realized there was no --frozen-lockfile in package.json or in Travis CI config. 🤔

That's like running npm ci vs npm install. I haven't checked but it might also be faster.

That would ensure that package.json and yarn.lock are never out of date with each other. It's not like it happens often, but it has happened.

Note that if later on project updates to Yarn v2, this will need to be changed to --immutable:

If the --immutable option is set, Yarn will abort with an error exit code if anything in the install artifacts (yarn.lock, .pnp.js, ...) was to be modified. For backward compatibility we offer an alias under the name of --frozen-lockfile, but it will be removed in a later release.

(via)

Testing instructions

I haven't actually tested this yet since I basically wanted to hear your thoughts first. I think it would go something along the lines of:

  • Run yarn build, confirm it pass
  • Update a dependency version in package.json and then run yarn build - it should stall

@simison simison added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. Build labels Feb 11, 2020
@simison simison requested a review from a team February 11, 2020 15:42
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello simison! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer, review, and approve D38709-code before merging this PR. Thank you!

@simison simison requested a review from brbrr February 11, 2020 15:42
@jetpackbot
Copy link

Warnings
⚠️ "Testing instructions" are missing for this PR. Please add some
⚠️ "Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is an automated check which relies on PULL_REQUEST_TEMPLATE. We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS against 723c7a5

@kraftbj
Copy link
Contributor

kraftbj commented Feb 11, 2020

I really like this idea. I haven't timed it, but I confirmed it does fail out of the build after manually bumping a package version.

From a bug standpoint, it is good to have a human-readable package.json and have some assurance it is what is being built. From a security standpoint, it would help us ensure that we aren't building outdated insecure code unintentionally.

Copy link
Contributor

@kraftbj kraftbj left a comment

Choose a reason for hiding this comment

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

Works as I'd expect. Leaving it as "Needs Review" for second Crew opinion on the concept.

@kraftbj kraftbj added this to the 8.3 milestone Feb 11, 2020
@zinigor zinigor self-assigned this Feb 12, 2020
@zinigor zinigor merged commit a539930 into master Feb 12, 2020
@zinigor zinigor deleted the update/run-frozen-yarn branch February 12, 2020 16:24
@matticbot matticbot added [Status] Needs Changelog and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Feb 12, 2020
@kraftbj
Copy link
Contributor

kraftbj commented Feb 12, 2020

r202762-wpcom

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.

5 participants