-
Notifications
You must be signed in to change notification settings - Fork 482
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
base: main
Are you sure you want to change the base?
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 1931 1982 +51
=======================================
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. |
Which problem is this PR solving?
Description of the changes
How was this change tested?
npm run start
Checklist
jaeger
:make lint test
jaeger-ui
:yarn lint
andyarn test