Skip to content

ci: require nvskills status for skill changes#372

Open
yashrajp22 wants to merge 4 commits into
mainfrom
yashraj/require-nvskills-status-for-skills
Open

ci: require nvskills status for skill changes#372
yashrajp22 wants to merge 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
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