Updated i18n/admin jobs to leverage nx affected#27348
Updated i18n/admin jobs to leverage nx affected#27348acburdine wants to merge 2 commits intoTryGhost:mainfrom
Conversation
|
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 (7)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughCI workflow 🚥 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 |
|
Also worth mentioning that the code as written doesn't detect changes to the github actions files yet. I have that locally in |
![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 287-289: The current CI job condition only checks
needs.job_setup.outputs.is_tag and whether 'ghost-admin' is in
needs.job_setup.outputs.affected_projects, but misses shared-file-only changes;
update the if condition used for the admin-tests trigger to also consider the
paths-filter's shared-file indicator (e.g.,
needs.job_setup.outputs.changed_shared) by adding an OR branch so the job runs
when changed_shared == 'true' OR when
contains(fromJSON(needs.job_setup.outputs.affected_projects), 'ghost-admin');
ensure you reference the existing outputs (needs.job_setup.outputs.is_tag,
needs.job_setup.outputs.affected_projects) and the new output name
(changed_shared) in the condition.
🪄 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: 10097c1e-dda7-4d8c-92d4-165c737b1f87
📒 Files selected for processing (6)
.github/workflows/ci.ymlapps/comments-ui/package.jsonapps/portal/package.jsonapps/signup-form/package.jsonapps/sodo-search/package.jsonghost/core/package.json
| if: | | ||
| needs.job_setup.outputs.is_tag != 'true' | ||
| && contains(fromJSON(needs.job_setup.outputs.affected_projects), 'ghost-admin') |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /dev/shm && git clone --depth=1 https://github.com/TryGhost/Ghost.git 2>&1 | head -20Repository: TryGhost/Ghost
Length of output: 116
🏁 Script executed:
cd Ghost && wc -l .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 113
🏁 Script executed:
cd Ghost && sed -n '287,295p' .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 113
🏁 Script executed:
cd Ghost && sed -n '94,110p' .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 113
🏁 Script executed:
cd Ghost && git log --oneline -20 -- .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 113
🏁 Script executed:
cd Ghost && git show HEAD~5:.github/workflows/ci.yml | sed -n '280,300p'Repository: TryGhost/Ghost
Length of output: 113
🏁 Script executed:
wc -l .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 87
🏁 Script executed:
sed -n '287,295p' .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 312
🏁 Script executed:
sed -n '94,110p' .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 624
🏁 Script executed:
sed -n '250,320p' .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 2503
🏁 Script executed:
git log --oneline -20 -- .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 114
🏁 Script executed:
git show HEAD~1:.github/workflows/ci.yml | sed -n '280,310p'Repository: TryGhost/Ghost
Length of output: 94
🏁 Script executed:
git log --all --oneline | head -20Repository: TryGhost/Ghost
Length of output: 1256
🏁 Script executed:
git show 42dcfeaa:.github/workflows/ci.yml | sed -n '280,310p'Repository: TryGhost/Ghost
Length of output: 1023
🏁 Script executed:
git show 42dcfeaa | head -50Repository: TryGhost/Ghost
Length of output: 2407
🏁 Script executed:
rg "changed_admin|paths-filter" .github/workflows/ci.yml -B 3 -A 3Repository: TryGhost/Ghost
Length of output: 1002
🏁 Script executed:
sed -n '100,180p' .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 3223
🏁 Script executed:
git show 42dcfeaa^:.github/workflows/ci.yml | sed -n '85,115p'Repository: TryGhost/Ghost
Length of output: 97
🏁 Script executed:
git show 42dcfeaa^:.github/workflows/ci.yml | sed -n '260,300p'Repository: TryGhost/Ghost
Length of output: 97
🏁 Script executed:
git show 42dcfeaa^:.github/workflows/ci.yml | rg "changed_admin|admin-tests" -B 5 -A 5Repository: TryGhost/Ghost
Length of output: 97
🏁 Script executed:
git show 42dcfeaa | grep -A 30 "job_admin-tests" | head -50Repository: TryGhost/Ghost
Length of output: 1717
🏁 Script executed:
git show 42dcfeaa | grep -B 10 "changed_admin"Repository: TryGhost/Ghost
Length of output: 1223
🏁 Script executed:
git show 42dcfeaa -- .github/workflows/ci.yml | head -200Repository: TryGhost/Ghost
Length of output: 8087
🏁 Script executed:
git log -p --all -- .github/workflows/ci.yml | grep -A 5 "changed_admin" | head -40Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
git diff 42dcfeaa HEAD -- .github/workflows/ci.yml | head -100Repository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
rg "changed_admin|ghost-admin" .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 374
🏁 Script executed:
rg "affects|admin" .nxrc.json nx.json -A 5 -B 5Repository: TryGhost/Ghost
Length of output: 108
🏁 Script executed:
fd "nx.json|project.json" | head -20Repository: TryGhost/Ghost
Length of output: 65
🏁 Script executed:
fd "nx.json" --type fRepository: TryGhost/Ghost
Length of output: 65
🏁 Script executed:
cat nx.jsonRepository: TryGhost/Ghost
Length of output: 1062
🏁 Script executed:
find . -name "project.json" -path "*/ghost/admin/*" -type fRepository: TryGhost/Ghost
Length of output: 40
🏁 Script executed:
git log --all --oneline --follow -- .github/workflows/ci.yml | head -30Repository: TryGhost/Ghost
Length of output: 1930
🏁 Script executed:
git log --all --diff-filter=A -- .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 1042
🏁 Script executed:
git show 42dcfeaa | grep -A 100 "job_admin-tests" | grep -B 5 "if:"Repository: TryGhost/Ghost
Length of output: 3787
🏁 Script executed:
git show fe4ef54b:.github/workflows/ci.yml | grep -A 10 "job_admin-tests:" | head -20Repository: TryGhost/Ghost
Length of output: 349
🏁 Script executed:
git show 8eb0a3e7:.github/workflows/ci.yml | grep -A 10 "job_admin-tests:" | head -20Repository: TryGhost/Ghost
Length of output: 349
🏁 Script executed:
git show 32d42308:.github/workflows/ci.yml | grep -A 10 "job_admin-tests:" | head -20Repository: TryGhost/Ghost
Length of output: 349
🏁 Script executed:
git show 9505bace:.github/workflows/ci.yml | grep -A 10 "changed_admin" | head -30Repository: TryGhost/Ghost
Length of output: 1456
🏁 Script executed:
git show 9505bace:.github/workflows/ci.yml | grep -A 3 "admin:" | head -10Repository: TryGhost/Ghost
Length of output: 425
🏁 Script executed:
git show 9505bace:.github/workflows/ci.yml | sed -n '85,130p'Repository: TryGhost/Ghost
Length of output: 1516
🏁 Script executed:
git show 9505bace:.github/workflows/ci.yml | grep -B 5 -A 10 "^\s*admin:"Repository: TryGhost/Ghost
Length of output: 565
🏁 Script executed:
git show 9505bace:.github/workflows/ci.yml | sed -n '1,50p' | grep -E "shared|lock|npmrc"Repository: TryGhost/Ghost
Length of output: 136
🏁 Script executed:
sed -n '130,145p' .github/workflows/ci.ymlRepository: TryGhost/Ghost
Length of output: 788
🏁 Script executed:
cat > /tmp/verify_shared.md << 'EOF'
# Verification Summary
## Historical Evidence (CONFIRMED)
**Commit 9505bace (before 42dcfeaa):**
- `admin:` paths-filter defined with:
- `- *shared` (includes `.github/**`, `package.json`, lock files)
- `- 'ghost/admin/**'`
- Job trigger: `needs.job_setup.outputs.changed_admin == 'true'`
- Shared file changes WOULD trigger admin tests
**Commit 42dcfeaa (the change):**
- Removed entire `admin:` paths-filter
- Removed `changed_admin` output
- Changed job trigger to Nx affected only: `contains(fromJSON(needs.job_setup.outputs.affected_projects), 'ghost-admin')`
- Shared file changes NO LONGER trigger admin tests
## Current State (HEAD)
- Admin tests gate: `contains(fromJSON(needs.job_setup.outputs.affected_projects), 'ghost-admin')`
- NO `changed_admin` output
- NO `changed_shared` output
- Shared filter still exists (lines 94-100) but only used by other jobs
## Impact Assessment
Shared files that now won't trigger admin tests:
- `.github/**` (workflow changes!)
- `.npmrc` (dependency config)
- `pnpm-lock.yaml` (dependency lock)
- `pnpm-workspace.yaml` (workspace config)
These changes can affect build/test behavior across all projects.
## Inconsistency Check
Other admin-like jobs still use shared fallback:
- `job_admin_x_settings`: uses `changed_admin_x_settings` from paths-filter with shared
- `job_core`: uses `changed_core` from paths-filter with shared
- But `job_admin-tests`: ONLY uses Nx affected (no shared fallback)
## Conclusion
The review's claims are VERIFIED:
1. ✓ Old gate inherited shared filter
2. ✓ New gate lost shared coverage
3. ✓ This is a legitimate coverage regression
4. ✓ Suggested fix is appropriate and consistent with other jobs
EOF
cat /tmp/verify_shared.mdRepository: TryGhost/Ghost
Length of output: 1747
Add shared-file fallback for admin-tests trigger.
The previous admin tests gate used a paths-filter that inherited the shared filter, covering changes to .github/**, .npmrc, pnpm-lock.yaml, and pnpm-workspace.yaml. The new Nx-only check misses these shared non-project files, since Nx affected is driven by changed files mapped through the project graph. Shared files only affect projects when explicitly wired in configuration. Add a changed_shared output from the paths-filter and combine it with the affected check using OR to preserve coverage for shared file changes.
Suggested fix
outputs:
+ changed_shared: ${{ steps.changed.outputs.shared }}
affected_projects: ${{ steps.affected.outputs.affected_projects }}
changed_i18n: ${{ steps.affected.outputs.affected_i18n_projects != '' }}
changed_core: ${{ steps.changed.outputs.core }}
changed_admin_x_settings: ${{ steps.changed.outputs.admin-x-settings }}
changed_activitypub: ${{ steps.changed.outputs.activitypub }}
@@
if: |
needs.job_setup.outputs.is_tag != 'true'
- && contains(fromJSON(needs.job_setup.outputs.affected_projects), 'ghost-admin')
+ && (
+ needs.job_setup.outputs.changed_shared == 'true'
+ || contains(fromJSON(needs.job_setup.outputs.affected_projects), 'ghost-admin')
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 287 - 289, The current CI job
condition only checks needs.job_setup.outputs.is_tag and whether 'ghost-admin'
is in needs.job_setup.outputs.affected_projects, but misses shared-file-only
changes; update the if condition used for the admin-tests trigger to also
consider the paths-filter's shared-file indicator (e.g.,
needs.job_setup.outputs.changed_shared) by adding an OR branch so the job runs
when changed_shared == 'true' OR when
contains(fromJSON(needs.job_setup.outputs.affected_projects), 'ghost-admin');
ensure you reference the existing outputs (needs.job_setup.outputs.is_tag,
needs.job_setup.outputs.affected_projects) and the new output name
(changed_shared) in the condition.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27348 +/- ##
==========================================
- Coverage 73.46% 73.46% -0.01%
==========================================
Files 1545 1545
Lines 123724 123724
Branches 14970 14971 +1
==========================================
- Hits 90897 90893 -4
- Misses 31827 31829 +2
- Partials 1000 1002 +2
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:
|
no issue - remove unused dorny/paths-filter actions/output - remove additional unused path-filter outputs - update i18n/admin tests to use nx affected to determine if they should run
acburdine
force-pushed
the
ci/nx-affect-pt1
branch
from
42dcfea to
bb7d413
Compare
|
1 participant
Summary
Why
For i18n, we had to check if several different paths had changed to determine if the job should run. If more packages start to trigger i18n tests, the condition has to be updated and get more complicated. By using nx
tagsto dictate to the task graph which packages should trigger i18n tests, the configuration gets a lot simpler.For admin, it was a simple matter of updating the condition to look in the affected packages list.
Note about tags
The i18n dependency is a bit of a tricky one as far as Nx is concerned because there's a bit of "cyclical dependency" going on. Each of the packages with translations depends on the code in i18n, but we also want i18n tests to run whenever one of the packages that depend on it changes.
My original approach to solving this was to add each of the i18n-using packages as
implicitDependenciesin the i18n package (which would tell the nx task graph that if any of the packages changes, i18n tests should run). The downside of that approach is it creates cyclical dependencies; while nx allows this, it means that now a change in any of the i18n apps ends up telling nx that all of the i18n apps have changed. (i.e. a change in portal triggers a "change" in i18n, which triggers a change in all packages that depend on i18n). Nx's package isolation boundaries also make it so we can't say "i18n depends on the files in these other packages".Tagging seemed like ultimately the simplest approach that leverages the task graph well and requires minimal configuration on the part of each package - we'll just need to remember in the future if more i18n packages are added that they will also need the i18n tag.