Improve performance of core acceptance/legacy CI jobs#27293
Conversation
no issue - nx cache hasn't been used since the update to Nx 22 when the legacy file cache was removed - removing the NX_REJECT_UNKNOWN_LOCAL_CACHE environment variable stops a warning from being printed every time nx is used
no issue This removes the explicit TS package build step in favor of nx automatic build triggers. Because of the way this command was invoked, all of the frontend packages were being built on every CI run of acceptance/legacy tests, which adds about 3-4 minutes per job. The only package ghost/core depends on is parse-email-address, and so by leveraging Nx's automatic dependent target running, we can ensure that only the necessary packages are built. Leveraging Nx now also means that if more lecal packages with build targets are added as ghost/core deps in the future, the jobs will continue to work as designed.
acburdine
changed the title
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe changes refactor CI/CD workflow and Nx task configuration. The GitHub Actions workflow was modified to remove explicit Nx cache handling and the separate "Build TS packages" steps from test jobs. Test command execution was updated to use Nx-targeted commands (e.g., 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27293 +/- ##
=======================================
Coverage 73.45% 73.45%
=======================================
Files 1545 1545
Lines 123675 123686 +11
Branches 14958 14958
=======================================
+ Hits 90846 90856 +10
- Misses 31809 31831 +22
+ Partials 1020 999 -21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@acburdine Is there any reason why we should not just merge this? Seems great to me! |
|
@ErisDS Nope, feel free to merge! |
no issue This removes the explicit TS package build step in favor of nx automatic build triggers. Because of the way this command was invoked, all of the frontend packages were being built on every CI run of acceptance/legacy tests, which adds about 3-4 minutes per job. The only package ghost/core depends on is parse-email-address, and so by leveraging Nx's automatic dependent target running, we can ensure that only the necessary packages are built. Leveraging Nx now also means that if more lecal packages with build targets are added as ghost/core deps in the future, the jobs will continue to work as designed. This is a repeat of #27293 which was partially reverted during the pnpm cutover)
## Summary - remove nx cache step + unnecessary env var from setup job that hasn't done anything since the Nx v22 update - remove explicit TS package build step in favor of nx automatic build tracking ## Why this change As part of TryGhost#27202, I was looking into ways to leverage the Nx build cache across jobs in a given CI run, since I noticed that certain packages (the frontend code in apps/* primarily) were being built multiple times throughout the various CI jobs. What I ultimately realized after a bit more investigation is that the Core Acceptance + Legacy tests jobs are building more of the repo than they need to, since the `--exclude=ghost-admin` doesn't actually prevent any work from being done (ghost-admin is a dependency of `@tryghost/admin` so it gets built anyways). ## What the fix does By updating the nx config in ghost/core to dependent on the built target for the test:ci:* scripts (and switching the jobs themselves to use `yarn nx run ghost:test:ci:*`), Nx will automatically build the dependencies it needs to. Overall, this should shave an average of 3-4 minutes off of all of the Acceptance/Legacy test builds.
no issue This removes the explicit TS package build step in favor of nx automatic build triggers. Because of the way this command was invoked, all of the frontend packages were being built on every CI run of acceptance/legacy tests, which adds about 3-4 minutes per job. The only package ghost/core depends on is parse-email-address, and so by leveraging Nx's automatic dependent target running, we can ensure that only the necessary packages are built. Leveraging Nx now also means that if more lecal packages with build targets are added as ghost/core deps in the future, the jobs will continue to work as designed. This is a repeat of TryGhost#27293 which was partially reverted during the pnpm cutover)
2 participants
Summary
Why this change
As part of #27202, I was looking into ways to leverage the Nx build cache across jobs in a given CI run, since I noticed that certain packages (the frontend code in apps/* primarily) were being built multiple times throughout the various CI jobs. What I ultimately realized after a bit more investigation is that the Core Acceptance + Legacy tests jobs are building more of the repo than they need to, since the
--exclude=ghost-admindoesn't actually prevent any work from being done (ghost-admin is a dependency of@tryghost/adminso it gets built anyways).What the fix does
By updating the nx config in ghost/core to dependent on the built target for the test:ci:* scripts (and switching the jobs themselves to use
yarn nx run ghost:test:ci:*), Nx will automatically build the dependencies it needs to. Overall, this should shave an average of 3-4 minutes off of all of the Acceptance/Legacy test builds.Note
Low Risk
Low risk CI-only change, but could cause unexpected rebuilds or missing prereqs if Nx target dependencies are misconfigured, affecting test job reliability/timings.
Overview
CI pipeline is simplified to reduce unnecessary work. The setup job drops the unused
NX_REJECT_UNKNOWN_LOCAL_CACHEenv var and removes the GitHub ActionsNx cachestep.Acceptance and legacy test jobs now rely on Nx dependency tracking. The explicit “build TS packages” step is removed, and Ghost core
test:ci:*scripts are executed viayarn nx run ghost:test:ci:*, withghost/core/package.jsonupdated sobuild:tsc,lint,test:unit, and thetest:ci:*targets depend on^buildinstead of a hardcoded@tryghost/parse-email-addressbuild.Reviewed by Cursor Bugbot for commit 130791d. Bugbot is set up for automated code reviews on this repo. Configure here.