feat(scripts): [Branching Tagging] Add GitHub permission check - #90
feat(scripts): [Branching Tagging] Add GitHub permission check#90amd-chiranjeevi wants to merge 8 commits into
Conversation
There was a problem hiding this comment.
Unit tests must run on CI for them to be useful. Setting up a unit tests CI job should have blocked the original merge of this work.
There was a problem hiding this comment.
I will open new PR to set up the unit tests.
There was a problem hiding this comment.
Hi @ScottTodd , I tested the unit tests manually and they're working as expected. Regarding the changes you suggested, would it be okay if I picked those up in a follow-up #103 to keep this one focused on enabling the tests?
There was a problem hiding this comment.
This script is getting very complex.... I'm considering forking/replacing it (main...ScottTodd:rockrel:release-branching for some ideas, still deciding on bash vs python)
There was a problem hiding this comment.
How about adding a separate script/module that performs permissions checks instead of baking that into this branching script directly?
These scripts should really be closer to 50-100 LOC, not 500+.
There was a problem hiding this comment.
Thanks Scott for suggestion, I will modularize the code.
|
Failing case when user dont have permission: $$ uv run create_release_branch.py \
--branch-name users/amd-chiranjeevi/test_rscript \
--commitid 1e735be18adaa4aae8a88fe1e53bb7439cd15ad3 \
--cache-dir ~/cache_dir
2026-08-24 13:07:25,168 INFO Authentication Mode: SSH
2026-08-24 13:07:25,168 INFO Dry run mode = True
2026-08-24 13:07:25,509 INFO ============================================================
2026-08-24 13:07:25,510 INFO GitHub Permission Check
2026-08-24 13:07:25,510 INFO Verifying push/admin access for 11 repo(s)
2026-08-24 13:07:25,510 INFO ============================================================
2026-08-24 13:07:30,162 INFO ============================================================
2026-08-24 13:07:30,163 INFO Permission Check Summary
2026-08-24 13:07:30,163 INFO Passed : 0 / 11 repo(s)
2026-08-24 13:07:30,163 INFO Failed : 11 / 11 repo(s)
2026-08-24 13:07:30,163 INFO ============================================================
ERROR: Permission check failed for 11 repo(s). Aborting before any branches are created.
half: Insufficient permissions for ROCm/half: push=False, admin=False
hipify: Insufficient permissions for ROCm/HIPIFY: push=False, admin=False
amd-llvm: Insufficient permissions for ROCm/llvm-project: push=False, admin=False
rocm-cmake: Insufficient permissions for ROCm/rocm-cmake: push=False, admin=False
rocm-libraries: Insufficient permissions for ROCm/rocm-libraries: push=False, admin=False
rocm-systems: Insufficient permissions for ROCm/rocm-systems: push=False, admin=False
spirv-llvm-translator: Insufficient permissions for ROCm/SPIRV-LLVM-Translator: push=False, admin=False
mesa-fork: Insufficient permissions for ROCm/mesa-fork: push=False, admin=False
libhipcxx: Insufficient permissions for ROCm/libhipcxx: push=False, admin=False
source: Insufficient permissions for ROCm/rocgdb: push=False, admin=False
TheRock: Insufficient permissions for ROCm/TheRock: push=False, admin=FalsePassing Case when user have the sufficient access: $$ uv run create_release_branch.py \
--branch-name users/amd-chiranjeevi/test_rscript \
--commitid 1e735be18adaa4aae8a88fe1e53bb7439cd15ad3 \
--cache-dir ~/cache_dir
2026-08-24 14:45:13,445 INFO Authentication Mode: SSH
2026-08-24 14:45:13,445 INFO Dry run mode = True
2026-08-24 14:45:13,758 INFO ============================================================
2026-08-24 14:45:13,758 INFO GitHub Permission Check
2026-08-24 14:45:13,758 INFO Verifying push/admin access for 11 repo(s)
2026-08-24 14:45:13,758 INFO ============================================================
2026-08-24 14:45:14,302 INFO Permission check OK: ROCm/half
2026-08-24 14:45:14,733 INFO Permission check OK: ROCm/HIPIFY
2026-08-24 14:45:15,243 INFO Permission check OK: ROCm/llvm-project
2026-08-24 14:45:15,659 INFO Permission check OK: ROCm/rocm-cmake
2026-08-24 14:45:16,099 INFO Permission check OK: ROCm/rocm-libraries
2026-08-24 14:45:16,557 INFO Permission check OK: ROCm/rocm-systems
2026-08-24 14:45:16,950 INFO Permission check OK: ROCm/SPIRV-LLVM-Translator
2026-08-24 14:45:17,413 INFO Permission check OK: ROCm/mesa-fork
2026-08-24 14:45:17,828 INFO Permission check OK: ROCm/libhipcxx
2026-08-24 14:45:18,235 INFO Permission check OK: ROCm/rocgdb
2026-08-24 14:45:18,689 INFO Permission check OK: ROCm/TheRock
2026-08-24 14:45:18,689 INFO ============================================================
2026-08-24 14:45:18,689 INFO Permission Check Summary
2026-08-24 14:45:18,689 INFO Passed : 11 / 11 repo(s)
2026-08-24 14:45:18,689 INFO Failed : 0 / 11 repo(s)
2026-08-24 14:45:18,689 INFO ============================================================ |
|
Branching Test Result: uv run create_release_branch.py \
--branch-name users/amd-chiranjeevi/test_rscript2 \
--commitid 1e735be18adaa4aae8a88fe1e53bb7439cd15ad3 \
--cache-dir ~/cache_dir \
--force-clone \
--exclude-list TheRock,amd-llvm,half,hipify,libhipcxx,mesa-fork,rocm-cmake,rocm-systems,source,spirv-llvm-translator \
--no-dry-run
.
.
.
.
TheRock/rocm-libraries]$ git checkout -B users/amd-chiranjeevi/test_rscript2 416eae89dcee3fb1e0c434f795edcf8cef40a80d
2026-08-25 11:13:35,055 INFO Switched to a new branch 'users/amd-chiranjeevi/test_rscript2'
2026-08-25 11:13:35,055 INFO ++ Exec [/home/cpattigi/test_branching_tagging_scripts_permissions_checks/cache_dir/TheRock/rocm-libraries]$ git push rocm-github users/amd-chiranjeevi/test_rscript2
2026-08-25 11:14:16,403 INFO Uploading LFS objects: 100% (104/104), 37 GB | 0 B/s, done.
2026-08-25 11:14:16,403 INFO remote:
remote: Create a pull request for 'users/amd-chiranjeevi/test_rscript2' on GitHub by visiting:
remote: https://github.com/ROCm/rocm-libraries/pull/new/users/amd-chiranjeevi/test_rscript2
remote:
remote: GitHub found 653 vulnerabilities on ROCm/rocm-libraries's default branch (469 high, 158 moderate, 26 low). To find out more, visit:
remote: https://github.com/ROCm/rocm-libraries/security/dependabot
remote:
To github.com:ROCm/rocm-libraries
* [new branch] users/amd-chiranjeevi/test_rscript2 -> users/amd-chiranjeevi/test_rscript2
2026-08-25 11:14:16,403 INFO Summary: 1 succeeded, 0 skipped, 0 failed out of 1 repos
2026-08-25 11:14:16,403 INFO Successful repos: {'rocm-libraries': RepoInfo(url='https://github.com/ROCm/rocm-libraries',
commit='416eae89dcee3fb1e0c434f795edcf8cef40a80d',
path=PosixPath('/home/cpattigi/test_branching_tagging_scripts_permissions_checks/cache_dir/TheRock/rocm-libraries'))} |
|
Tagging Script Test result: $$uv run rock_tagging.py \
--branch-name users/amd-chiranjeevi/test_rscript \
--release-version 25082026-test \
--commitid 1e735be18adaa4aae8a88fe1e53bb7439cd15ad3 \
--exclude-list TheRock,amd-llvm,half,hipify,libhipcxx,mesa-fork,rocm-cmake,rocm-systems,source,spirv-llvm-translator \
--cache-dir ~/cache_dir \
--no-dry-run
.
.
.
[INFO] Execution plan: {'rocm-libraries': RepoInfo(url='https://github.com/ROCm/rocm-libraries',
commit='416eae89dcee3fb1e0c434f795edcf8cef40a80d',
path=PosixPath('/home/cpattigi/cache_dir/TheRock/rocm-libraries'))}
[INFO] Working directory: /home/cpattigi/cache_dir
[INFO] ++ Exec [/home/cpattigi/cache_dir/TheRock/rocm-libraries]$ git remote set-url rocm-github git@github.com:ROCm/rocm-libraries
[INFO] ++ Exec [/home/cpattigi/cache_dir/TheRock/rocm-libraries]$ git tag -a therock-25082026-test 416eae89dcee3fb1e0c434f795edcf8cef40a80d -m 'therock release v25082026-test'
[INFO] ++ Exec [/home/cpattigi/cache_dir/TheRock/rocm-libraries]$ git push rocm-github therock-25082026-test:refs/tags/therock-25082026-test
[INFO] Uploading LFS objects: 100% (104/104), 37 GB | 0 B/s, done.
[INFO] To github.com:ROCm/rocm-libraries
* [new tag] therock-25082026-test -> therock-25082026-test |
ScottTodd
left a comment
There was a problem hiding this comment.
This PR is still quite large and complicated. I'd rather reset back to the fundamentals and build scripts one at a time to do one focused job well:
check_release_branch_state.[py,sh]: read local git state, list remotes, check github permissions, report configuration issues. Always safe to run, mutates nothing.create_release_branches.[py,sh]: trust that state is stable or run the check script at startup, then create and push branchesprepare_release_branches.[py,sh]: apply branch initialization changes to release branches in each repository (pinning workflows, setting up versions, etc.)tag_release_branches.[py,sh]: create and push tags
The target for the scripts that create branches/tags should be 50-100 lines. Certainly not 500+. Anyone should be able to read the code as quickly see what the scripts are doing to review their side effects and how safe they are to run. Right now that is getting obfuscated behind layers of python wrapper code and error handling for conditions that could be checked as preconditions.
|
|
@arjun-raj-kuppala @ScottTodd |
Motivation
When creating release branches across TheRock and its ROCm submodules, the script could partially succeed — creating branches in some repos but failing silently in others due to insufficient permissions. This leaves the release in an inconsistent state that is difficult to recover from.
This PR adds an upfront GitHub permission check that verifies the authenticated user has push or admin access to every repo before any branches are created. If any repo fails, the entire run aborts with a clear summary.
As part of this work, shared logic between
create_release_branch.pyandrock_tagging.pyhas been extracted into a newrelease_utils.pymodule, directly addressing the reviewer feedback about script complexity.Note: The permission check runs in both
--dry-runand--no-dry-runmodes. Only the actualgit pushis skipped in dry-run.Changes
scripts/release_utils.py(new)Shared module extracted from both automation scripts. Contains:
RepoInfo— dataclass describing a single repo in an execution planextract_owner_repo(url)— parses(owner, repo)from HTTPS or SSH GitHub URLsget_gh_token()— retrieves the activeghCLI token; noGITHUB_TOKENenv var requiredfetch_lightweight_plan(token, commitid, exclude_list)— reads.gitmodulesdirectly from the GitHub API at the requested commit, building a repo-name→ URL map without cloning anything locally
check_permissions(token, repo_map, logger, action)— callsGET /repos/{owner}/{repo}for every repo, validatespushoradminaccess, collects allfailures before aborting so the user sees the full list in one run
RockBase— base class with subprocess helpers (run_command,run_command_output), git helpers (convert_to_ssh,_setup_remote,get_submodule_url_map), andbuild_plan()for cloning/reusing a TheRock cache and reading submodule statescripts/create_release_branch.pyRockBase; imports permission helpers fromrelease_utilsrun()checks permissions viaget_gh_token+fetch_lightweight_plan+check_permissionsbefore the expensive clone stepscripts/rock_tagging.pyRockBase; imports the same permission helpers fromrelease_utilscreate_release_branch.py(previously absent)scripts/tests/test_release_utils.py(new)44 unit tests covering all functions and classes in
release_utils:TestExtractOwnerRepo— HTTPS/SSH, with/without.git, invalid URLsTestGetGhToken— success,ghnot found, not authenticated, empty tokenTestFetchLightweightPlan— ROCm filtering, TheRock always included, exclude list, SSH URLs, HTTP/network errorsTestCheckPermissions— push/admin pass, 403/404/5xx/network/invalid-URL failures, all-repos-checked-before-abort,actionlabel in messageTestConvertToSsh— HTTPS conversion, SSH/non-GitHub passthroughTestGetSubmoduleUrlMap— no file, correct parsing, missing URL entry skippedTestBuildPlanSubmoduleParsing— ROCm included/excluded, exclude list, TheRock always present, SHA prefix strippingTestRunCommand— buffered success/failure, streaming success/nonzero-exit/timeoutscripts/tests/test_create_release_branch.pyTestExtractOwnerRepoandTestCheckPermissionsupdated to call standalone functions fromrelease_utils(no longer methods on the class)Test Plan
gh auth loginand a token that has push access to all repos → should printPermission Check Passed: all N repo(s)and proceed--dry-run(default) and--no-dry-runmodes → permission check fires in bothTest Result
Unit tests
scripts/tests/test_release_utils.py::TestExtractOwnerRepo::test_valid_urls[...] PASSED
scripts/tests/test_release_utils.py::TestGetGhToken::test_returns_token_on_success PASSED
scripts/tests/test_release_utils.py::TestCheckPermissions::test_push_access_passes PASSED
scripts/tests/test_release_utils.py::TestCheckPermissions::test_all_repos_checked_before_abort PASSED
... (44 tests total)
scripts/tests/test_create_release_branch.py::TestExtractOwnerRepo::... PASSED
scripts/tests/test_create_release_branch.py::TestCheckPermissions::... PASSED
... (existing tests updated and passing)
Manual test (all repos passing)
============================================================
GitHub Permission Check
Verifying push/admin access for 11 repo(s)
INFO Permission check OK: ROCm/TheRock
INFO Permission check OK: ROCm/llvm-project
...
Submission Checklist