ci: test if docs build and run task depending on the changes from PR #11761
Conversation
|
Warning Rate limit exceeded@pkuczynski has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRefactors the GitHub Actions workflow to add a Changes
Sequence Diagram(s)sequenceDiagram
participant Event as Push/PR
participant Detect as detect-changes
participant Format as formatting
participant Docs as docs
participant Build as build
participant TestLinux as tests-linux
participant TestWin as tests-windows
participant Coverage as coverage
Event->>Detect: start workflow
activate Detect
Detect->>Detect: evaluate path filters<br/>emit outputs (lint/docs/src-or-tests/...)
deactivate Detect
par Conditional branches
alt lint detected
Detect->>Format: run formatting
activate Format
Format-->>Format: format checks/changes
deactivate Format
end
alt docs detected
Detect->>Docs: run docs job (cwd: ./docs)
activate Docs
Docs-->>Docs: build docs
deactivate Docs
end
alt src-or-tests detected
Detect->>Build: run build
activate Build
Build-->>Build: install/compile/upload artifacts
Build->>TestLinux: trigger platform tests (depends on build)
Build->>TestWin: trigger platform tests (depends on build)
deactivate Build
par platform tests
TestLinux-->>Coverage: report results
TestWin-->>Coverage: report results
end
Coverage->>Coverage: aggregate/generate coverage
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/commit-validation.yml (1)
23-23: Remove temporary code before merge.Based on a prior review comment, the
.github/workflows/**path on line 23 in the docs filter is temporary and should be removed before this PR is merged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/commit-validation.yml(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/commit-validation.yml
106-106: property "detect-changes" is not defined in object type {tests-linux: {outputs: {}; result: string}; tests-windows: {outputs: {}; result: string}}
(expression)
🔇 Additional comments (2)
.github/workflows/commit-validation.yml (2)
8-9: Verify the detect-changes output format.Multiple jobs reference
needs.detect-changes.outputs.changeswithcontains(), assuming it's a string listing filter names. However, thedorny/paths-filter@v3action typically outputs individual boolean flags for each filter (e.g.,docs: 'true',src-or-tests: 'false'), not a combinedchangesstring.Verify that this output format is correct and that the conditions work as intended. If the action's default behavior has changed or you're using a custom configuration, this is fine; otherwise, the conditions may need adjustment (e.g.,
if: needs.detect-changes.outputs.src-or-tests == 'true').Also applies to: 44-44, 57-57, 72-72, 89-89, 100-100, 107-107
33-33: Verify the test workflows path.The path
workflows/test/**/*on line 33 appears unusual. Verify this is the correct path for matching test workflow files. Given the context, did you intend.github/workflows/tests-*.ymlor a similar pattern?
Co-authored-by: Mike Guida <mike@mguida.com>
Co-authored-by: Mike Guida <mike@mguida.com>
| @@ -1,22 +1,49 @@ | |||
| name: Commit Validation | |||
| on: | |||
| push: | |||
There was a problem hiding this comment.
This didn't make a lot of sense to me, as we always work via PRs, so no need to verify manual pushes, which should never happen...
| branches: | ||
| - "**" | ||
| paths-ignore: | ||
| - "docs/**" |
There was a problem hiding this comment.
Not sure why docs where excluded, but certainly they can't now. We want to test if docs still builds with the changes from the PR...
| - docs/**/* | ||
| - .github/workflows/** | ||
| src: &src |
There was a problem hiding this comment.
Possibly this could be named better as it's a bit wider than only the source. I typically name it app, but does not fit this repo. Any suggestions welcome!
# Conflicts: # .github/workflows/commit-validation.yml
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/commit-validation.yml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
.github/workflows/commit-validation.yml (6)
43-54: Formatting job structure looks good.Condition, dependency, and steps are correctly configured. The job will conditionally run when lint changes are detected.
Later in the file (lines 64–65), other jobs use
actions/checkout@v5andactions/setup-node@v5. Please verify these action versions are current and not outdated. The current versions of these actions may be v4 or higher.
56-69: Docs job structure is correct.Condition, dependency, and working directory are properly configured. The job will build documentation only when docs changes are detected, isolated to the ./docs directory.
71-86: Build job structure is correct.Condition, dependency, and artifact upload are properly configured. Compilation output is uploaded for downstream test jobs to consume.
88-97: Tests-linux job structure is correct.Condition and dependencies properly gated. The job will run matrix tests across Node versions 18 and 20 only when source/test changes are detected, and depends on build to consume artifacts.
99-104: Tests-windows job structure is correct.Condition and dependencies properly configured. The job will run on Node 20 only when source/test changes are detected.
106-113: Coverage job structure is correct.The previous critical issue (missing
detect-changesin needs) has been properly resolved. Dependencies now correctly includedetect-changes,tests-linux, andtests-windows. The job will run only after all test jobs complete when source/test changes are detected.
|
@pkuczynski The tests are no longer running after this PR was merged. Could you please take a look? |
|
in the master branch, this commit should have run all the tests: 2681051 |
|
They were run on PR (check here), so what's the point on runing them again on master? Our pipeline is already super slow... |
|
There is no way to merge PR currently if the CI did not run |
We want to ensure that the main branch in this repo is stable, regardless of the PR status. |
If nobody can merge a PR which is not ahead of |
Description of change
docsbuildsPull-Request Checklist
masterbranchFixes #00000Summary by CodeRabbit