build: migrate to Yarn v3 #2212
Conversation
a442cc9 to
7f3cf03
Compare
7f3cf03 to
2130e27
Compare
2130e27 to
e506dcf
Compare
e506dcf to
8c924bf
Compare
There was a problem hiding this comment.
@zondervancalvez Thank you!
I added a few comments/questions and change requests, please see above!
| @@ -43,14 +43,14 @@ | |||
| "dist/*" | |||
| ], | |||
| "scripts": { | |||
| "codegen": "run-p 'codegen:*'", | |||
| "codegen": "yarn run codegen:openapi && yarn run codegen:proto", | |||
There was a problem hiding this comment.
@zondervancalvez Could you please explain why was this specific change necessary?
There was a problem hiding this comment.
After few tests, we revert this to original and added minor changes.
| "codegen": "run-p 'codegen:*'", | ||
| "codegen:openapi": "run-p generate-sdk", | ||
| "generate-sdk": "run-p generate-sdk:*", | ||
| "codegen": "yarn run codegen:openapi", |
There was a problem hiding this comment.
@zondervancalvez Same question here as in my other comment. Why run only a specific script instead of the pattern?
There was a problem hiding this comment.
After few tests, we revert this to original and added minor changes. We added single quote to generate-sdk:*
tools/ci.sh
Outdated
| @@ -116,6 +116,7 @@ function mainTask() | |||
| if [ "${DEV_BUILD_DISABLED:-false}" = "true" ]; then | |||
| echo "$(date +%FT%T%z) [CI] Dev build disabled. Skipping..." | |||
| else | |||
| yarn install | |||
There was a problem hiding this comment.
@zondervancalvez The configure script already runs yarn install, must we run it twice, e.g. during and right before the configure script as well?
There was a problem hiding this comment.
We updated the yarn run configure script so that it will run first the yarn install command.
BUILD.md
Outdated
| @@ -135,6 +133,12 @@ cd cactus | |||
| npm run enable-corepack | |||
| ``` | |||
|
|
|||
| * Run this command to migrate lockfile (Also, run this everytime you have changes before running `yarn run cofugure`) | |||
There was a problem hiding this comment.
@zondervancalvez I'm guessing that's a typo in the script name, please fix!
There was a problem hiding this comment.
After some changes and re-testing, we already removed this.
There was a problem hiding this comment.
@zondervancalvez It is still in the diff.
BUILD.md
Outdated
| @@ -135,6 +133,12 @@ cd cactus | |||
| npm run enable-corepack | |||
| ``` | |||
|
|
|||
| * Run this command to migrate lockfile (Also, run this everytime you have changes before running `yarn run cofugure`) | |||
There was a problem hiding this comment.
@zondervancalvez Please go into a little more detail about the lockfile migration: explain that it is needed when someone was using yarn v1 but once it was migrated then there is no need. Also explain in a few words how to recognize which version it is (the lockfile has a version declaration in it somewhere IIRC)
Also, run this everytime you have changes before running
yarn run cofugure
Is this true though? If I change some .ts code and then want to run the configure script, what difference does the extra yarn install will make in that case in general if I already migrated my lockfile earlier?
There was a problem hiding this comment.
@zondervancalvez A couple more asks:
- Please write a legible commit message body explaining in detail the what/why/how/etc. of the changes.
- Rebase onto upstream/main and fix the merge conflicts
387a49f to
4348716
Compare
Fixes hyperledger-cacti#1142 Signed-off-by: zondervancalvez <zondervan.v.calvez@accenture.com>
4348716 to
71671ca
Compare
Signed-off-by: zondervancalvez zondervan.v.calvez@accenture.com
Fixes #1142