ci: require nvskills status for skill changes - #372
Conversation
mosheabr
left a comment
There was a problem hiding this comment.
Thanks @yashrajp22 — this closes a real gap and the overall shape is right. A few things before it can land:
Blockers
- DCO is failing — please
git rebase --signoff origin/main && git push --force-with-lease. - The guard needs to exempt the
automated/sync-skillsbot PRs (same pattern as the DCO/authors checks). Sync PRs touchskills/**with content signed in the source repos, so their head SHAs never carry anNVSkills CIstatus — as written this fails every daily sync PR.
Design
- The
status:trigger won't clear a red check on the PR:status-event runs execute against the default branch, so the new check run doesn't attach to the PR head SHA. Authors would need a manual re-run after/nvskills-cisucceeds. Consider having the NVSkills CI pipeline refresh the check itself, or aworkflow_run-based re-check. - Please drop the
paths:filter on thepull_requesttrigger. Once this check is required in branch protection, PRs that don't touch watched paths (e.g.components.d/onboarding PRs) would never report the check and sit at "Expected" forever. The job already self-skips via the files API, so it's safe to run on every PR.
Minor
- The
pushbranch of the guard'sifis unreachable (the caller only forwards signature pushes, which the guard excludes) — can be removed. - The failure message should note that
/nvskills-cionly works on branches in NVIDIA/skills, not forks, so fork authors know to move their branch rather than re-commenting.
Happy to re-review once updated.
f69c055 to
cb798b4
Compare
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
cb798b4 to
b04f8b6
Compare
mosheabr
left a comment
There was a problem hiding this comment.
Re-reviewed, and I tested the reworked guard locally against real PRs: #379 and #361 (the trailing-benchmarks-commit cases) now pass — the guard correctly walks back to the signature commit's status and treats the head's benchmarks/metadata commit as trailing. A genuinely-failing PR (#370) still blocks, and no-watched-change / bot-branch PRs skip. Design #3 is resolved. Two minor, non-blocking notes remain (the no-trailing-commit timing edge without a status:/workflow_run: refresh, and the unreachable push branch), but those don't gate this. Approving — nice work. 🚀
Summary
NVSkills CIstatus.skills/,team-skills/,rules/team-rules/, andplugins/./nvskills-cidispatch flow unchanged.Validation
.github/workflows/team-request.ymlas YAMLbash -non the inline guard scriptgit diff --check