Improve Nx base commit detection, remove unnecessary CI steps#27297
Improve Nx base commit detection, remove unnecessary CI steps#27297acburdine merged 1 commit intoTryGhost:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCI workflow: granted 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 65-69: The nx SHAs step "Set SHAs for Nx Commands (push)" using
nrwl/nx-set-shas@afb73a62d26e41464e9254689e1fd6122ee683c1 needs the
main-branch-name input set so NX_BASE is computed against the current branch
rather than always main; update the step to pass main-branch-name: ${{
github.ref_name }} (keep error-on-no-successful-workflow: true) so release
branches (e.g., v12.1.0 or 6.x) compute affected commits correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d25b1327-be61-4063-8bbe-e4759bb79e25
📒 Files selected for processing (2)
.github/workflows/ci.ymlnx.json
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ad3c8f7. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27297 +/- ##
=======================================
Coverage 73.46% 73.46%
=======================================
Files 1545 1545
Lines 123724 123724
Branches 14970 14970
=======================================
+ Hits 90893 90897 +4
+ Misses 31829 31806 -23
- Partials 1002 1021 +19
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
force-pushed
the
ci/nx-set-shas
branch
from
ad3c8f7 to
b8f430a
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
76-79:⚠️ Potential issue | 🟠 MajorAvoid live PR API lookup for
NX_BASEto prevent SHA drift.At Line 76,
NX_BASEis fetched from the live PR API. If the base branch advances after this workflow run is triggered, the returned SHA can diverge from the checked-out PR merge context (github.sha), causing incorrect affected ranges or missing-commit failures downstream. Prefer using the event payload SHA (github.event.pull_request.base.sha) and validate it before export.Suggested fix
- name: Set SHAs for Nx Commands (pull_request) if: github.event_name == 'pull_request' run: | - NX_BASE=$(curl --location --request GET 'https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}' --header 'Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}' | jq -r .base.sha) + NX_BASE='${{ github.event.pull_request.base.sha }}' + if ! [[ "$NX_BASE" =~ ^[0-9a-f]{40}$ ]]; then + echo "::error::Invalid pull_request.base.sha: $NX_BASE" + exit 1 + fi echo "Setting NX_BASE to $NX_BASE" - echo "NX_BASE=$NX_BASE" >> $GITHUB_ENV + echo "NX_BASE=$NX_BASE" >> "$GITHUB_ENV"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 76 - 79, The workflow currently computes NX_BASE by calling the live PR API via the curl command and jq, which can drift from the checked-out merge SHA; replace that lookup with the event payload value github.event.pull_request.base.sha, validate it is non-empty, and then export it to GITHUB_ENV as NX_BASE (falling back to the existing curl result only if the event payload is missing) so NX_BASE matches the workflow's checked-out context.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 76-79: The workflow currently computes NX_BASE by calling the live
PR API via the curl command and jq, which can drift from the checked-out merge
SHA; replace that lookup with the event payload value
github.event.pull_request.base.sha, validate it is non-empty, and then export it
to GITHUB_ENV as NX_BASE (falling back to the existing curl result only if the
event payload is missing) so NX_BASE matches the workflow's checked-out context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0dedc098-b6ca-44b8-ad6b-e5e96e8709d3
📒 Files selected for processing (2)
.github/workflows/ci.ymlnx.json
🚧 Files skipped from review as they are similar to previous changes (1)
- nx.json
|
note: now that #27295 is merged this change is even more important; main branch CI runs have the potential to be canceled which could potentially caused lint/unit test runs to be skipped since each main branch CI run only looks at the commits that were a part of its own push |
|
sonarqube failure is a red herring I believe, it's flagging code that was already there. (might make sense to refactor that part of the job at some point but it's not a new issue) |
acburdine
force-pushed
the
ci/nx-set-shas
branch
from
b8f430a to
4fada68
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 71-77: Validate the NX_BASE value returned from the GitHub API
before exporting it: after extracting NX_BASE (the variable set by
`NX_BASE=$(curl ... | jq -r .base.sha)`), check if it's empty or the literal
"null" and fail the job with a clear error message (or optionally
retry/fallback), e.g. test `if [[ -z "$NX_BASE" || "$NX_BASE" == "null" ]]; then
echo "Failed to determine NX_BASE from GitHub API" >&2; exit 1; fi`; ensure this
guard is added to the same GitHub Actions step so downstream commands like `nx
affected` never run with an invalid NX_BASE.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffb609f6-5170-4851-a835-06de35c20ae5
📒 Files selected for processing (2)
.github/workflows/ci.ymlnx.json
✅ Files skipped from review due to trivial changes (1)
- nx.json
acburdine
force-pushed
the
ci/nx-set-shas
branch
from
4fada68 to
b524b12
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
71-77:⚠️ Potential issue | 🟠 MajorHarden PR
NX_BASEresolution before exporting it.Line 75 can still produce empty/
nullNX_BASEon API failure, which then propagates into downstream diff/affected steps.Suggested fix
- name: Set SHAs for Nx Commands (pull_request) if: github.event_name == 'pull_request' run: | - NX_BASE=$(curl --location --request GET 'https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}' --header 'Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}' | jq -r .base.sha) + NX_BASE=$(curl --fail --silent --show-error --location --request GET 'https://api.github.com/repos/${{ github.repository }}/pulls/${{ github.event.pull_request.number }}' --header 'Authorization: Bearer ${{ secrets.GITHUB_TOKEN }}' | jq -r '.base.sha // empty') + if [ -z "$NX_BASE" ]; then + echo "::error::Failed to resolve NX_BASE for pull_request" + exit 1 + fi echo "Setting NX_BASE to $NX_BASE" echo "NX_BASE=$NX_BASE" >> $GITHUB_ENV🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 71 - 77, The PR step "Set SHAs for Nx Commands (pull_request)" can export an empty or "null" NX_BASE when the GitHub API fails; update the run block that sets NX_BASE to validate the value after the curl + jq command (check NX_BASE is non-empty and not "null") and if validation fails, print a clear error including the raw API response and exit non‑zero so downstream steps don't receive an invalid NX_BASE; modify the run script that defines NX_BASE to perform this check and fail fast (or optionally implement a simple retry) before echoing "NX_BASE=$NX_BASE" into $GITHUB_ENV.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 71-77: The PR step "Set SHAs for Nx Commands (pull_request)" can
export an empty or "null" NX_BASE when the GitHub API fails; update the run
block that sets NX_BASE to validate the value after the curl + jq command (check
NX_BASE is non-empty and not "null") and if validation fails, print a clear
error including the raw API response and exit non‑zero so downstream steps don't
receive an invalid NX_BASE; modify the run script that defines NX_BASE to
perform this check and fail fast (or optionally implement a simple retry) before
echoing "NX_BASE=$NX_BASE" into $GITHUB_ENV.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 216c3905-3f48-42a5-b07d-c34f67fdb369
📒 Files selected for processing (2)
.github/workflows/ci.ymlnx.json
🚧 Files skipped from review as they are similar to previous changes (1)
- nx.json
acburdine
force-pushed
the
ci/nx-set-shas
branch
from
b524b12 to
0d1a4a9
Compare
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
Looks great! If you'd like to move the setup job to a deeper clone, depth: 1000 would only take ~7MB more which wouldn't appreciably slow us down. I was just looking at this a couple days ago and was shocked at the full depth being close to 400MB.
acburdine
force-pushed
the
ci/nx-set-shas
branch
2 times, most recently
from
9cfce1e to
46665f5
Compare
|
@9larsons tried 500 which should definitely have been enough, but I think the way the nx-set-shas action works for Pull Requests, it requires a full git tree in order to find the merge-base. The way it's working now (push events fetch from last successful run, prs from API) should be ok for now - the paths-filter actions actually end up fetching all the necessary commits between HEAD and BASE to do the diff so it ends up resolving whatever commits the job needs |
no issue - add nrwl/nx-set-shas github action to set HEAD/BASE commit SHAs on main branch CI runs. This would work on pull-requests too, except we're only doing a shallow clone in the setup job and the set-shas action requires a deeper clone to be able to inspect the commit tree on PRs. The advantage of the set-shas action (on the main branch at least) is it doesn't look at the commits merged as part of a particular CI run, it iterates through the last CI action runs and finds the last commit the CI workflow was successful on. This ensures that if the build fails for any reason, we won't inadvertently skip any lint/unit test runs - simplify unit test job by adding build as a dependent target for test:unit scripts. This way, build jobs and unit test jobs can all run in parallel in the same Nx command rather than being split up over multiple ones
acburdine
force-pushed
the
ci/nx-set-shas
branch
from
e9adbca to
1bf11b0
Compare
|
|
Ok, finally got everything working as intended I think: switched to using |
9larsons
left a comment
9larsons
left a comment
There was a problem hiding this comment.
Nice changes! Aside from cleaning up affected_projects it's good to merge.
| - name: Install dependencies | ||
| run: bash .github/scripts/install-deps.sh | ||
|
|
||
| - name: Determine Affected Projects |
There was a problem hiding this comment.
This is no longer used, right?
There was a problem hiding this comment.
Actually this is prep for future PRs 😅 added it here to verify Nx affected worked with the treeless clone
I removed the affected detection in the unit test job, the idea in the setup job is to use affected to determine whether certain jobs need to run at all
2 participants
Summary
Why this change
Another split-out from #27202, this simplifies the CI jobs running that use
nx affectedto conditionally run targets on changed packages.First, it adds the nrwl/nx-set-shas action for determining the BASE_COMMIT on main branch CI runs. The advantage of the set-shas action (on the main branch at least) is that it doesn't look at the commits merged as part of a particular CI run, it iterates through the CI action runs to find the last commit the CI workflow was successful on. This ensures that if the build fails for any reason, we won't inadvertently skip any lint/unit test runs (whereas the current implementation would end up skipping those checks). The set-shas action would work on pull-requests too, except we're only doing a shallow clone in the setup job and the set-shas action requires a deeper clone to be able to inspect the commit tree on PRs, so I left the pr base commit detection as-is.
Additionally, this PR simplifies the unit test job by adding build as a dependent target for test:unit scripts. This way, build jobs and unit test jobs can all run in parallel in the same Nx command rather than being split up over multiple ones.
Potential Drawbacks
test:unitmeans that for local devs, running test:unit with nx will end up needingbuildto be run as well. Operating under the assumption that build needs to be run before unit tests anyways, this shouldn't be much of an issue. If unit tests don't require the build step to be run, then this PR could be simplified even further.nx affectedto cover additional types of tests, so having the base commit be the one with the last successful CI run seems prudent to cover future use-cases.Note
Medium Risk
Changes CI SHA detection and how
nx affectedis parameterized, which can alter which projects get linted/tested (risk of unexpected skips or extra work). Also makestest:unitalways run afterbuild, affecting both CI and local workflows.Overview
Improves CI reliability for
nx affectedby switching push runs tonrwl/nx-set-shasand standardizing onNX_BASE/NX_HEADenv vars (including exposingnx_basefromjob_setup).Simplifies lint/unit-test jobs by removing explicit
--basearguments and (for unit tests) dropping the pre-step that computed affected projects + built them.Updates
nx.jsonsotest:unitdepends onbuild, ensuring required builds happen automatically when running affected unit tests.Reviewed by Cursor Bugbot for commit b8f430a. Bugbot is set up for automated code reviews on this repo. Configure here.