Skip to content

ci: require nvskills status for skill changes - #372

Merged
mosheabr merged 4 commits into
mainfrom
yashraj/require-nvskills-status-for-skills
Jul 27, 2026
Merged

ci: require nvskills status for skill changes#372
mosheabr merged 4 commits into
mainfrom
yashraj/require-nvskills-status-for-skills

Conversation

@yashrajp22

Copy link
Copy Markdown
Collaborator

Summary

  • Add a reusable workflow guard that fails when watched skill paths changed but the current PR head SHA does not have a successful NVSkills CI status.
  • The guard covers skills/, team-skills/, rules/team-rules/, and plugins/.
  • Keeps the existing /nvskills-ci dispatch flow unchanged.

Validation

  • Parsed .github/workflows/team-request.yml as YAML
  • Ran bash -n on the inline guard script
  • Ran git diff --check

@mosheabr mosheabr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yashrajp22 — this closes a real gap and the overall shape is right. A few things before it can land:

Blockers

  1. DCO is failing — please git rebase --signoff origin/main && git push --force-with-lease.
  2. The guard needs to exempt the automated/sync-skills bot PRs (same pattern as the DCO/authors checks). Sync PRs touch skills/** with content signed in the source repos, so their head SHAs never carry an NVSkills CI status — as written this fails every daily sync PR.

Design

  1. 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-ci succeeds. Consider having the NVSkills CI pipeline refresh the check itself, or a workflow_run-based re-check.
  2. Please drop the paths: filter on the pull_request trigger. 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

  1. The push branch of the guard's if is unreachable (the caller only forwards signature pushes, which the guard excludes) — can be removed.
  2. The failure message should note that /nvskills-ci only 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.

@yashrajp22
yashrajp22 force-pushed the yashraj/require-nvskills-status-for-skills branch 2 times, most recently from f69c055 to cb798b4 Compare July 21, 2026 19:46
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>
@yashrajp22
yashrajp22 force-pushed the yashraj/require-nvskills-status-for-skills branch from cb798b4 to b04f8b6 Compare July 27, 2026 08:33

@mosheabr mosheabr left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🚀

@mosheabr
mosheabr merged commit b33996a into main Jul 27, 2026
5 checks passed
@mosheabr
mosheabr deleted the yashraj/require-nvskills-status-for-skills branch July 27, 2026 15:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants