Fix subdirectory tree SHA lookups in TagBot #433
Conversation
Add _check_rate_limit() method to query GitHub API rate limit status and log remaining/limit/reset information when 403 errors are encountered. This helps distinguish between rate limiting and permission issues.
…#315) TagBot now normalizes UUIDs to lowercase when looking them up in the registry, matching the registry's behavior. This fixes the issue where packages with uppercase UUIDs were being ignored by TagBot. - Normalize UUID in _registry_path property - Normalize UUID in _registry_pr method - Add test for uppercase UUID handling
TagBot was not properly handling tree SHA lookups for packages in subdirectories. The registry stores the tree SHA for the subdirectory, but TagBot was comparing it against the root repository tree SHA, causing it to fail to find matching commits. This fixes the issue where TagBot would report "tag missing" errors for packages in subdirectories even when tags had already been created. Changes: - Add subdirectory support to _commit_sha_of_tree_from_branch method - Add subdirectory support to _commit_sha_of_tree fallback method
IanButterworth
requested review from
Copilot
and removed request for
IanButterworth
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where TagBot incorrectly compared tree SHAs for packages located in subdirectories, resulting in false "tag missing" notifications. The fix implements proper subdirectory tree SHA lookups using git rev-parse commit:subdir instead of comparing against the root repository tree SHA.
Key Changes
- Modified
_commit_sha_of_tree_from_branchto check subdirectory tree hashes when__subdiris set - Added fallback logic in
_commit_sha_of_treeto handle subdirectory tree hash lookups when searching through all commits - Implemented exception handling to gracefully skip commits where the subdirectory doesn't exist
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test_commit_sha_of_tree_from_branch_subdir verifies matching when __subdir is set and the correct rev-parse calls are made. test_commit_sha_of_tree_from_branch_subdir_rev_parse_failure checks that an Abort from rev-parse is handled, logged, and iteration continues to later commits.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
test/action/test_repo.py:759
- The test expects
logger.errorto be called withexc_info=True, but the corresponding implementation change appears to be missing fromtagbot/action/repo.pyline 738. The implementation still only callslogger.error("Issue reporting failed")without theexc_info=Trueparameter. Either this test change should be reverted, or the implementation inrepo.pyline 738 needs to be updated tologger.error("Issue reporting failed", exc_info=True).
r.handle_error(RuntimeError("?"))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Remove exception info from warning log for hack usage.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tagbot/action/repo.py:321
- The new subdirectory handling in
_commit_sha_from_registry_pr(lines 314-321) lacks test coverage. Consider adding a test case that verifies the method correctly uses_subdir_tree_hashwhen a subdir is configured, similar to the tests added for_commit_sha_of_tree_from_branch_subdir.
if self.__subdir:
subdir_tree_hash = self._subdir_tree_hash(commit.sha, suppress_abort=False)
if subdir_tree_hash == tree:
return cast(str, commit.sha)
else:
msg = "Subdir tree SHA of commit from registry PR does not match"
logger.warning(msg)
return None
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
|
@IanButterworth added/improved all the copilot suggestions. |
- Add -n 10000 limit to git log fallback to prevent perf issues on large repos - Remove unused logger mock decorators from subdir fallback tests Co-authored-by: Claude <[email protected]>
2 participants
The fix ensures that when TagBot processes packages in subdirectories, it correctly compares tree SHAs by using
git rev-parse commit:subdirinstead of comparing against the root repository tree SHA. This prevents false "tag missing" notifications for subpackages that have already been released.Fixes #241
Fixes #376