-
Notifications
You must be signed in to change notification settings - Fork 480
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
Upgrade yarn to v3.8.5 #2459
base: main
Are you sure you want to change the base?
Upgrade yarn to v3.8.5 #2459
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2459 +/- ##
=======================================
Coverage 96.61% 96.61%
=======================================
Files 254 254
Lines 7679 7679
Branches 1931 1995 +64
=======================================
Hits 7419 7419
Misses 260 260 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Andreas Gerstmayr <[email protected]>
bbd9000
to
866c893
Compare
Signed-off-by: Andreas Gerstmayr <[email protected]>
!.yarn/releases | ||
!.yarn/sdks | ||
!.yarn/versions |
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.
Why would we not ignore these?
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 copied this list straight from the yarn docs here: https://yarnpkg.com/getting-started/qa#which-files-should-be-gitignored
There's an explanation in that FAQ entry for each path.
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.
Regarding checking in yarn sources, afaics we have two options when using yarn v2+:
- use the experimental corepack feature
- or commit the yarn sources
see yarnpkg/berry#5748 for more details
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.
corepack
seems preferable, I am strongly against vendoring yarn code in the repo (feels insane to me that they would even propose this as a default setup).
In the issue there were mentions of problems with how corepack operates depending on versions. Probably not an issue for jaeger-ui.
Q: why yarn 3.8 and not 4.x?
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.
The reason for yarn 3.8 was that we want to use cachi2 (used to ensure the build is hermetic, i.e. it caches dependencies to support building Jaeger UI without network access), which at the moment only supports yarn v3 (I assume v4 might also be supported, but I didn't test it).
Alternatively, we could also use npm
. It also supports workspaces now (in case this was the reason to switch to yarn in the first place). What do you think?
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 don't really follow. We have a yarn.lock file that ensures repeatable build. Are we worried about supply chain attack?
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, it's about supply chain attacks. Our downstream builds need to be hermetic according to SLSA and include a Software Bill of Materials (SBOM) etc.
You're right, effectively the yarn package manager already supports these properties. We need to support multiple languages and package managers though, so this part of the build pipeline is handled by an external tool like cachi2 for our downstream builds.
Hm, somehow the s390x build is stuck because of the upgrade to yarn v3. I'll see with IBM if we can get this resolved. |
Which problem is this PR solving?
Description of the changes
Thelint
step is broken at the moment, because TypeScript incorrectly resolves the typings for react-router v5 with the types from react-router v6 (installed as a dependency ofreact-router-dom-v5-compat
) instead of@types/react-router-dom
. I'm not sure why this works correctly with yarn v1 though. See remix-run/react-router#9892How was this change tested?
yarn start
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test