-
Notifications
You must be signed in to change notification settings - Fork 914
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 yarn install --frozen-lockfile in CI #1676
Conversation
How about We used to set to always download the latest version, but the problem with that is if a new yarn version gets released, we will not be aware of such update in our Travis build. And that has caused some trouble in the past. Also see #1351 and #1481. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Done. It would be the best if we could just get the yarn version from devDependencies (and actually add yarn to devDependencies), but I don't know how we can do that easily. |
Agreed. The current way of managing yarn version isn't very pretty. I guess the problem is that |
567e2dc
to
7907682
Compare
Yeah exactly what I'm thinking as well 😅 |
Thanks for looking into this problem! |
Before I opened this PR I had two assumptions:
Turns out, both of my assumptions were wrong 😄 There is a script just like you mentioned here. I found out about this when I opened this PR and Travis failed with this error. It went away after I rebased this PR on top of #1675. |
I think we still need a separate script to check if yarn.lock is changed after We already do it as part of the release process https://github.com/firebase/firebase-js-sdk/blob/master/scripts/release/utils/git.js#L78 And thanks again for looking into it. Let me know if you want me to do it. |
No problem 🙂
|
Sounds good as long as it works. Can you please add some comments? then I think we are good. |
Friendly ping. I ran into this problem again, I'd like to get this merged. |
Copy pasted the yarn documentation. |
hmm, you mentioned it doesn't work the way it was documented, right? Can we just describe why we have it here, and how it works in its current state? |
dbd3c7c
to
08cd4c7
Compare
I see, done. I was hesitant to mention that it doesn't work as expected, because if they fix it, the comment will be wrong and that's worse than having no comment at all 😄. But I see the benefit of mentioning it here, in case someone else notices the same issue and wonders how this works. I subscribed to the yarn issue. Hopefully I'll remember to change this comment if it ever gets fixed 🙂 |
From yarn documentation:
With this change, our CI build should fail during the install phase if the lock file wasn't updated properly. But it won't, because this doesn't actually work 😐. I guess if they ever fix this bug, it will start working. And then we can remove the integrity check from
tools/pretest.js
.Also changes the fixed yarn version to
latest
, so that we won't have to change it manually after new yarn releases.