-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
chore: optimize task execution #27848
Conversation
This reverts commit 0a86ec6.
This reverts commit 9e60aeb.
* chore: update build-npm-modules script * chore: update build-npm-modules script * chore: update build-npm-modules script * chore: update build-npm-modules script * chore: update lerna to 6 * [run ci] * try caching build step * we can't clean without building after * add dependencies on scripts for npm packages * update commands * add config for data-context build step * fix output configurations for npm packages, add gitignores * revert changes to config and data-context build steps * fix outputs * run with cache * fix outputs for cli * actually fix outputs * test with cache --------- Co-authored-by: astone123 <[email protected]>
* chore: simplify build script * update CI workflows * fix workflows * empty commit because Percy weirdness
@@ -14,6 +14,7 @@ const { getNextVersionForBinary } = require('../get-next-version') | |||
} | |||
|
|||
const body = JSON.stringify({ | |||
branch: 'lerna-optimize-tasks', |
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.
This ensures that the pipeline that we trigger runs on the lerna-optimize-tasks
branch in the publish binary workflow. I'll remove this before merging and merge my corresponding PR https://github.com/cypress-io/cypress-publish-binary/pull/28
@@ -102,7 +102,12 @@ export async function buildCypressApp (options: BuildCypressAppOpts) { | |||
if (!keepBuild) { | |||
log('#buildPackages') | |||
|
|||
await execa('yarn', ['lerna', 'run', 'build-prod', '--ignore', 'cli', '--concurrency', '4'], { | |||
await execa('yarn', ['lerna', 'run', 'build', '--concurrency', '4'], { |
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.
We want to make sure that everything is built here before we build the binary. These will be cached because we already built them in a previous step. We can probably remove this in the future
@@ -116,30 +115,6 @@ gulp.task('open', startCypressWatch) | |||
* Tasks that aren't watched. Usually composed together with other tasks. | |||
*------------------------------------------------------------------------**/ | |||
|
|||
gulp.task('buildProd', |
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 targeted this task because it was being called in CI, but I want to come back and clean up more of these tasks - we don't need to use Gulp for these tasks. Each task should be isolated to its related package if possible
@@ -1,5 +1,6 @@ | |||
{ | |||
"name": "internal-scripts", | |||
"version": "0.0.0-development", |
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.
This gets rid of that annoying warning saying that scripts doesn't have a version
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.
Looks good to me overall. Left a couple of minor changes that aren't show stoppers but ideally we can resolve them before merging. Also left a few future improvements comments along the way that we can use to inform future improvements
"nexus-build" | ||
], | ||
"outputs": [ | ||
"{workspaceRoot}/packages/frontend-shared/cypress/support/generated/test-graphql-types.gen.ts", |
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 worth noting that we should eventually move away from one lib having outputs in another. It's fine for now but we should consider refactoring this at some point
@@ -170,9 +145,6 @@ gulp.task('watchForE2E', gulp.series( | |||
*------------------------------------------------------------------------**/ | |||
|
|||
gulp.task('cyRunLaunchpadE2E', gulp.series( |
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.
Is this no longer needed because the build
has already been cached? This probably works as is in CI but ideally those gulp script that call the build-prod
would have a depends on build
in case someone tries to run this locally. Probably ok for now but we want to get to a place where local and CI run the same way
Released in This comment thread has been locked. If you are still experiencing this issue after upgrading to |
This PR makes various improvements to task execution both locally and in CI. Main changes are
build
targets for all packages (note that the CLI packagecypress
is excluded from this and uses its own commandbuild-cli
)buildProd
gulp command and incorporate steps into build commands inside of the packages that are related to each step. This allows for caching these steps with Lernabuild-cli
so that Lerna doesn't cache itbuild-prod
targets for packagestest-binary-against-recipes
job in CIThe results of these changes are:
Testing
Related work
This work requires changes to the cypress-publish-binary project as well
https://github.com/cypress-io/cypress-publish-binary/pull/28