Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ci: remove vendored yarn, and use volta for Travis + GHA #19069
ci: remove vendored yarn, and use volta for Travis + GHA #19069
Changes from 24 commits
9a97e5c
f91caf0
80f55fe
7273fe3
8c76217
9e27872
93f8c69
95c3bf0
648284d
4c6678d
2e84e8e
c847483
a281bec
de10242
f72bcef
1654a31
26a9cc0
5f5d73d
d665676
2e99bfd
0406170
a6b7536
80dc4bd
c00bf13
d37d023
d75ddbc
38f39e4
8cd19de
4660f39
b36e750
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
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.
Blocker: I think this test should not exist or if it does, we should be reading this value from
package.json
file.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.
Yeah, unfortunately, I wouldn't want to have people install
jq
. Would you be fine with a shittygrep
for this?My opinion is that it's not worth the effort, given that we rarely change node versions. When we do, it'd be easy to change this value. (It'd error pretty quickly in CI anyways.)
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.
Yes, I'd be okay with a shitty grep. I'll even save you from the hassle:
awk 'BEGIN { RS=",|: {\n"; FS="\""; } $2 == "node" { print $4 }' package.json
Trust me, when we need to change the node version, and we will soon as they are releasing a new security update soon, this will become a problem. Even worse, 6 months later, when someone who is not you (or even you) wants to update this, it will again be a big headache.
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.
I just don't see why it would be a "big headache" to update one thing that is even caught by CI.
Thanks for the awk, I'll put it in. Just to clarify, I don't have a problem with doing this at all, I'm just interested in why you're so averse to this.
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.
Oh, your awk doesn't work.
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.
It works on my machine 😛 Joking aside, I think this is one of those shitty BSD/GNU/whatever differences as I tested this on Ubuntu and guessing you are trying this on macOS.
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.
For anyone reading this who is not me or Josh, we discussed this over Slack. Summary: because it is not obvious or easy to find/remember that this needs be changed until it fails.
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.
For posterity it's GNU awk vs BSD awk. I can come up with a fix.
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.
Oh, comments just refreshed.