-
Notifications
You must be signed in to change notification settings - Fork 508
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
Migrate from yarn v1 to npm #2462
Migrate from yarn v1 to npm #2462
Conversation
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
@yurishkuro we are having issues building jaeger-ui internally with the new build system that does not support yarn1, therefore we would like to migrate. |
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2462 +/- ##
=======================================
Coverage 96.61% 96.61%
=======================================
Files 254 254
Lines 7679 7679
Branches 1995 1931 -64
=======================================
Hits 7419 7419
Misses 260 260 ☔ View full report in Codecov by Sentry. |
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.
Do we need changes to dependabot/renovate configs?
BUILD.md
Outdated
|
||
Tests for React components in `./packages/jaeger-ui` make extensive use of Jest's [snapshot testing](https://jestjs.io/docs/28.x/snapshot-testing) functionality. These snapshots can be regenerated by running `yarn test -u` from the package directory to regenerate all snapshots, or `yarn test -u --testNamePattern <test name>` to regenerate snapshots for a subset of tests only. | ||
Tests for React components in `./packages/jaeger-ui` make extensive use of Jest's [snapshot testing](https://jestjs.io/docs/28.x/snapshot-testing) functionality. These snapshots can be regenerated by running `npm run --workspaces test -- -u` from the package directory to regenerate all snapshots, or `npm run --workspaces test -- -u --testNamePattern <test name>` to regenerate snapshots for a subset of tests only. |
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 new commands are pretty unusable for something that we need to run often, like executing a single test. Is it possible to have some sort of shortcut? Why can't we use something like "npm test ..."?
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 command propagation changed, in yarn yarn run somescript arg1
forwards arg1
to somescript
, but with npm you need to write npm run somescript -- arg1
, so npm can differentiate between args for npm
and args for the script.
I updated the workspaces invocation, now additional args are correctly passed to the test
script in the root package.json, which then forwards it to the test
script of all workspaces. The invocation now is almost identical.
Is there a blog post about that? |
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
https://code.visualstudio.com/updates/v1_94#_use-npm-as-default-package-manager |
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.
Q: how did we ensure that package-lock.json
is equivalent to yarn.lock
? Specifically, there were some upgrades to yarn.lock
to address vulnerabilities in transitive dependencies.
BUILD.md
Outdated
|
||
Tests for React components in `./packages/jaeger-ui` make extensive use of Jest's [snapshot testing](https://jestjs.io/docs/28.x/snapshot-testing) functionality. These snapshots can be regenerated by running `yarn test -u` from the package directory to regenerate all snapshots, or `yarn test -u --testNamePattern <test name>` to regenerate snapshots for a subset of tests only. | ||
Tests for React components in `./packages/jaeger-ui` make extensive use of Jest's [snapshot testing](https://jestjs.io/docs/28.x/snapshot-testing) functionality. These snapshots can be regenerated by running `npm test -- -u` from the package directory to regenerate all snapshots, or `npm test -- -u --testNamePattern <test name>` to regenerate snapshots for a subset of tests only. |
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.
can we provide an example from the existing tests? It's not clear what <test name>
means, e.g. in this test file:
PASS src/utils/readJsonFile.test.js
fileReader.readJsonFile
✓ rejects when given an invalid file (2 ms)
✓ does not throw when given an invalid file (1 ms)
✓ loads JSON data, successfully (8 ms)
✓ loads JSON data (OTLP), successfully (2 ms)
✓ rejects an OTLP trace (4 ms)
✓ rejects malformed JSON (1 ms)
✓ loads JSON-per-line data (1 ms)
Is it the actual title/description? Is "pattern" a reference to some regex?
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's a regex matching the full test name, i.e. test name and all surrounding describe blocks: https://jestjs.io/docs/cli#--testnamepatternregex
I've updated the text and added a link to the docs.
That's a good point! I'm not sure if there is a way to prove the equivalence of the lock files. There is a tool which converts between lockfile formats called synp, but it uses the Another idea, if we regenerate the lockfile from scratch, we get the latest (as defined by the semver) versions of all (transitive) dependencies including all available CVE fixes. What do you think? |
Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Andreas Gerstmayr <andreas@gerstmayr.me>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Thinking more about it, I can just print a list of all dependencies and their version, sort it and then diff it with the versions of the previous lockfile. I'll do that, but in any case, I'll try to upgrade all dependencies first (in a separate PR). |
@andreasgerstmayr how do you want to handle conflicts and recent upgrades in main? I regress merging those, should've just merged this PR first. |
I'll rebase this PR once #2470 is merged. Then we're at the latest version of all dependencies in the yarn lockfile, and generating a new npm lockfile from scratch should get us the same results. |
Other upgrades merged. |
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Thanks! I updated the PR to resolve all conflicts and updated the PR description to add the dependency change table. |
Signed-off-by: Andreas Gerstmayr <agerstmayr@redhat.com>
Which problem is this PR solving?
Description of the changes
To not regress, I created a small script to compare the old and new version of the lockfile:
Dependency dashboard (excluding equal, upgraded)
Summary: 16 upgraded, 0 downgraded, 4 added, 0 removed, 1200 equal.
Full list of changes
Dependency dashboard
Summary: 16 upgraded, 0 downgraded, 4 added, 0 removed, 1200 equal.
How was this change tested?
npm run start
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test