-
Notifications
You must be signed in to change notification settings - Fork 594
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
[rush] Implementation of Cobuild feature #3949
Conversation
94cbbbd
to
dfc0456
Compare
dfc0456
to
dbf758f
Compare
libraries/rush-lib/src/logic/operations/OperationExecutionRecord.ts
Outdated
Show resolved
Hide resolved
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.
Way too much is happening in ShellOperationRunner that has nothing to do with its task of "execute a shell command and log its stdout/stderr". Cobuilds should be a wrapper, not part of ShellOperationRunner (so, for that matter, should the build cache, so that we get cache support for other runners).
libraries/rush-lib/src/logic/operations/ShellOperationRunner.ts
Outdated
Show resolved
Hide resolved
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.
Some more design feedback.
libraries/rush-lib/src/logic/operations/CacheableOperationRunnerPlugin.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/OperationExecutionManager.ts
Outdated
Show resolved
Hide resolved
This is looking really nice. I have a comment about configuration here -- I wonder if there's too much (potentially unneeded) functionality. Here's some (admittedly opinionated) suggestions:
Let me know what you think! |
First, thanks you for reviewing this! @elliot-nelson For 2 and 3, Do you mean that "context id" should be not configured in the "cobuid.json", instead, "context id" must be specified by environment variable ("RUSH_COBUILD_CONTEXT_ID") or CLI parameter(not designed yet)? If so, i am OK to remove the "context id pattern" part. A further question from my side: Should For 4, Do you mean that the configuration for CobuildRedisPlugin should get the functionality to specify a env var? such as rush-redis-cobuild-plugin.json {
"url": "redis://a.b.c.d:6379",
"password": "${REDIS_PASS}"
} where REDIS_PASS is a env var from predefined variable in CI. |
@chengcyber - I've come up with a repro for the issue we're seeing. Steps:
You should see output that looks like this:
|
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
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 safety, please also update OperationMetadataManager#tryRestoreAsync
to ensure that control cannot exit the function without cleaning up logReadStream
, e.g. by instantiating it outside of the try
and cleaning it up in a finally
block.
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
🚀 A prerelease of this branch is published as version
|
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
libraries/rush-lib/src/logic/operations/CacheableOperationPlugin.ts
Outdated
Show resolved
Hide resolved
[rush] Separate Skip and Build Cache, add flag
Summary
Closes #3485
Details
[ ] Add a new lifecycle to PhasedCommandHooks, which receives current results from operationManager. And periodically called for reporting current progress from a global view. The use case would be like sending operation status to a data server, and repo maintainer can build a cobuild dashboard with the realtime data.(will be another MR.How it was tested
rush-redis-cobuild-plugin-integration-test
a. test redisLockProvider
b. test cobuild for sandbox repo via running VS Code tasks.
Preparation
Integration test folder: build-tests/rush-redis-cobuild-plugin-integration-test
Sandbox repo folder: build-tests/rush-redis-cobuild-plugin-integration-test/sandbox/repo
cd sandbox/repo rush update
Case 1: Normal build, Cobuild is disabled because of missing RUSH_COBUILD_CONTEXT_ID
rm -rf common/temp/build-cache && node ../../lib/runRush.js --debug cobuild
Expected behavior: Cobuild feature is disabled. Build cache is saved/restored as normal.
Case 2: Cobuild enabled by specifying RUSH_COBUILD_CONTEXT_ID and Redis authentication
rm -rf common/temp/build-cache && RUSH_COBUILD_CONTEXT_ID=foo REDIS_PASS=redis123 RUSH_COBUILD_RUNNER_ID=runner1 node ../../lib/runRush.js --debug cobuild
Expected behavior: Cobuild feature is enabled. Run command successfully.
You can also see cobuild related logs in the terminal.
Case 3: Cobuild enabled, run two cobuild commands in parallel
Open predefined
.vscode/redis-cobuild.code-workspace
in Visual Studio Code.Clear redis server
# Under rushstack/build-tests/rush-redis-cobuild-plugin-integration-test/sandbox/repo rm -rf common/temp/build-cache
Tasks: Run Task
and selectcobuild
.Expected behavior: Cobuild feature is enabled, cobuild related logs out in both terminals.
Case 4: Cobuild enabled, run two cobuild commands in parallel, one of them failed
Open predefined
.vscode/redis-cobuild.code-workspace
in Visual Studio Code.Making the cobuild command of project "A" fails
sandbox/repo/projects/a/package.json
# Under rushstack/build-tests/rush-redis-cobuild-plugin-integration-test/sandbox/repo rm -rf common/temp/build-cache
Tasks: Run Task
and selectcobuild
.Expected behavior: Cobuild feature is enabled, cobuild related logs out in both terminals. These two cobuild commands fail because of the failing build of project "A". And, one of them restored the failing build cache created by the other one.
Impacted documentation
A brand new experimental feature!