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

build: update yarn version #27193

Closed
wants to merge 1 commit into from
Closed

Conversation

alexeagle
Copy link
Contributor

Some engineers were already on Yarn 0.10.x which was permitted by the range in our package.json#engines
However this introduced 'integrity sha512' lines into the yarn.lock files.
Then when engineers use yarn 0.9 (in particular, Bazel did this) then the lock files get tons of meaningless edits.
We could force everyone back to yarn 0.9 but this commit chooses to instead advance everyone past 0.10

@ocombe
Copy link
Contributor

ocombe commented Nov 20, 2018

Can you lock yarn version to 1.12.x in package.json to avoid this kind of errors in the future?

@ocombe
Copy link
Contributor

ocombe commented Nov 20, 2018

Sounds good, once the tests pass

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

Where does yarn that is installed on circleci come from? based on the CI errors we run 1.10.x on circle, which causes CI to fail.

In the past it came from the docker image but now I don't see where it's installed. Do we rely on yarn being provided by the default docker image? that seems wrong.

@IgorMinar IgorMinar added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews area: build & ci Related the build and CI infrastructure of the project labels Nov 20, 2018
@ngbot ngbot bot added this to the needsTriage milestone Nov 20, 2018
@mary-poppins
Copy link

You can preview baf3c7a at https://pr27193-baf3c7a.ngbuilds.io/.

@alexeagle
Copy link
Contributor Author

@IgorMinar yes we pick up a yarn version from docker circleci/node:10.12 (which is already 1.10.1)
We could manually install a different yarn, as we used to do. I looked through the history of the dockerfile but didn't find it.
It's much easier to make the yarn range >=1.10.1 <1.13.0 - this works with our current docker image as well as the defaults supplied by rules_nodejs

@mary-poppins
Copy link

You can preview cd120c7 at https://pr27193-cd120c7.ngbuilds.io/.

@IgorMinar
Copy link
Contributor

how about we add an additional step to our CI jobs that will install the exact version of yarn based on the variable set in https://github.com/angular/angular/blob/master/.circleci/env.sh ?

@alexeagle
Copy link
Contributor Author

sounds great but I think we should merge this to fix the breakage and do a fancier setup later

@alexeagle alexeagle added the target: patch This PR is targeted for the next patch release label Nov 20, 2018
Some engineers were already on Yarn 0.10.x which was permitted by the range in our package.json#engines
However this introduced 'integrity sha512' lines into the yarn.lock files.
Then when engineers use yarn 0.9 (in particular, Bazel did this) then the lock files get tons of meaningless edits.
We could force everyone back to yarn 0.9 but this commit chooses to instead advance everyone past 0.10
@alexeagle alexeagle removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Nov 20, 2018
@mary-poppins
Copy link

You can preview 95ff645 at https://pr27193-95ff645.ngbuilds.io/.

Copy link
Contributor

@IgorMinar IgorMinar left a comment

Choose a reason for hiding this comment

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

don't we need to also relock the files?

@alexeagle
Copy link
Contributor Author

@IgorMinar this commit also updates to latest rules_nodejs, where Bazel will no longer touch your lock file.

@IgorMinar
Copy link
Contributor

IgorMinar commented Nov 20, 2018 via email

mhevery pushed a commit that referenced this pull request Nov 21, 2018
Some engineers were already on Yarn 0.10.x which was permitted by the range in our package.json#engines
However this introduced 'integrity sha512' lines into the yarn.lock files.
Then when engineers use yarn 0.9 (in particular, Bazel did this) then the lock files get tons of meaningless edits.
We could force everyone back to yarn 0.9 but this commit chooses to instead advance everyone past 0.10

PR Close #27193
@mhevery mhevery closed this in 026b7e3 Nov 21, 2018
FrederikSchlemmer pushed a commit to FrederikSchlemmer/angular that referenced this pull request Jan 3, 2019
Some engineers were already on Yarn 0.10.x which was permitted by the range in our package.json#engines
However this introduced 'integrity sha512' lines into the yarn.lock files.
Then when engineers use yarn 0.9 (in particular, Bazel did this) then the lock files get tons of meaningless edits.
We could force everyone back to yarn 0.9 but this commit chooses to instead advance everyone past 0.10

PR Close angular#27193
ngfelixl pushed a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019
Some engineers were already on Yarn 0.10.x which was permitted by the range in our package.json#engines
However this introduced 'integrity sha512' lines into the yarn.lock files.
Then when engineers use yarn 0.9 (in particular, Bazel did this) then the lock files get tons of meaningless edits.
We could force everyone back to yarn 0.9 but this commit chooses to instead advance everyone past 0.10

PR Close angular#27193
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: build & ci Related the build and CI infrastructure of the project cla: yes target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants