[no-ci] pr-metadata-check: restrict cuda_bindings changes to NVIDIA organization members#1871
[no-ci] pr-metadata-check: restrict cuda_bindings changes to NVIDIA organization members#1871rwgk wants to merge 3 commits intoNVIDIA:mainfrom
Conversation
Keep the PR policy workflow self-contained by using the pull request payload and files API to block non-members from modifying low-level bindings. Made-with: Cursor
rwgk
added
P1
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
cpcloud
left a comment
cpcloud
left a comment
There was a problem hiding this comment.
Exploration of using gh api instead of raw curl invocations and lots of bash is warranted here.
| page=1 | ||
| TOUCHES_CUDA_BINDINGS=false | ||
| while true; do | ||
| FILES_JSON=$(curl --silent --show-error --fail \ |
There was a problem hiding this comment.
Why are you using curl here instead of gh api --paginate? It seems like maybe you haven't told your agent about using gh instead of raw curl for things like this.
You can also have it run that jq script using --jq, make it even more readable and concise.
You can then avoid looping over API invocations and burning up gh rate limits as well as all the overwrought bash that hardcodes the page size and checks for pages with fewer files than that number.
There was a problem hiding this comment.
Done: commit a52b862
It seems like maybe you haven't told your agent about using gh instead of raw curl for things like this.
Yes. I assumed it'd know such things. Probably next month :-)
Align the PR metadata workflow with repo conventions by using gh to paginate the PR file list, keeping the bindings restriction check simpler and easier to review. Made-with: Cursor
| done <<<"$BLOCKED_LABELS" | ||
|
|
||
| # Only NVIDIA organization members may change code under cuda_bindings. | ||
| if [ "$AUTHOR_ASSOCIATION" != "MEMBER" ] && [ "$AUTHOR_ASSOCIATION" != "OWNER" ]; then |
There was a problem hiding this comment.
Q: I assume checking MEMBER is enough? Do we need OWNER here?
| flatten | ||
| | any( | ||
| .[]; | ||
| (.filename | startswith("cuda_bindings/")) |
There was a problem hiding this comment.
Important: Also include cuda_python/
| # | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| name: "CI: Enforce assignee/label/milestone on PRs" |
There was a problem hiding this comment.
As discussed in this team meeting, it would be awesome if the org check was a separate line item to make it obvious when it fails.
|
Superseded by #1878 |
4 participants
Closed: Superseded by #1878
Related issue: #1139
pr-metadata-checkto read the PR author's association and changed files from thepull_request_targetpayload/API.cuda_bindings/unless the author association isMEMBERorOWNER, including rename cases.cuda_bindings/LICENSE.Note: We don't have a practical way to end-to-end test this workflow before merging. If issues show up after merge, we can roll back quickly with a
[no-ci]revert PR.