-
Notifications
You must be signed in to change notification settings - Fork 531
ci: Create Github Action to Automate CODEOWNER update #1870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 3934511.
Testing if the token has permission to create PRs from pull_request trigger. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Use workflow_dispatch for testing instead to ensure secrets are accessible. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary of ChangesHello @yzh119, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds configurability for directory depth and the number of top owners in the codeowner_analyzer.py
script. The changes are logical and correctly plumb the new parameters through the script. However, I've found a high-severity issue where negative values for the new command-line arguments can lead to incorrect and unexpected behavior. I've also noted that tests for this new functionality are missing. Please see my detailed comments for suggestions on how to address these points.
parser.add_argument( | ||
"--depth", | ||
type=int, | ||
default=3, | ||
help="Maximum directory depth for module detection (default: 3)", | ||
) | ||
parser.add_argument( | ||
"--top-n", | ||
type=int, | ||
default=3, | ||
help="Number of top owners to include in CODEOWNERS file (default: 3)", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new command-line arguments --depth
and --top-n
are defined to accept any integer. This allows negative values, which can lead to incorrect behavior:
- A negative
--depth
will causemin(len(path_parts), self.max_depth)
to likely return a negative number, resulting in no modules being detected without any warning. - A negative
--top-n
will be interpreted as a slice from the end of the owners list due to Python's slicing behavior (e.g.,-1
selects all but the last owner). This is not the intended behavior for selecting the top N owners and will produce an incorrectCODEOWNERS
file.
To prevent this, you should validate that these arguments are non-negative integers. You can do this by creating a custom type function for argparse
.
Here is an example of a validator function you could add before main()
:
import argparse
def non_negative_int(value):
"""Custom argparse type for non-negative integers."""
try:
ivalue = int(value)
if ivalue < 0:
raise argparse.ArgumentTypeError(f"{value} is an invalid non-negative int value")
return ivalue
except ValueError:
raise argparse.ArgumentTypeError(f"{value} is not an integer")
You can then use it in add_argument
like this:
type=non_negative_int
parser.add_argument( | ||
"--depth", | ||
type=int, | ||
default=3, | ||
help="Maximum directory depth for module detection (default: 3)", | ||
) | ||
parser.add_argument( | ||
"--top-n", | ||
type=int, | ||
default=3, | ||
help="Number of top owners to include in CODEOWNERS file (default: 3)", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While adding these new configuration options is a great improvement, the pull request is missing tests to verify the new functionality. Adding tests is crucial for ensuring the features work as expected and to prevent future regressions.
Please consider adding unit tests that cover:
- The
--depth
argument correctly limits the module hierarchy being generated. - The
--top-n
argument correctly selects the specified number of top owners. - Edge cases, such as
--depth=0
or--top-n=0
.
LGTM |
env: | ||
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
run: | | ||
python scripts/codeowner_analyzer.py \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use --allowed-users-file
here to explicitly control the group of trusted codeowners?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, do you want this list to be public or private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think public is ok. This will give people an opportunity to either ask to be removed (if they no longer want to be a codeowner) or ask to be included (if they want to be but aren't for some reason).
Should we add tests/ to the codeowners anyway (noticed it got intentionally excluded)? If a test fails, it may be good to find the person who is most knowledgeable about it. Sometimes it's easy to find the associated module, sometimes it may not be. Just a nitpick, since we could also use git blame. |
📌 Description
Duplicate of #1869 but created from flashinfer/workflow-update-codeowner to make sure we have permission.
This PR introduces a GitHub Action that automatically creates pull requests to update the CODEOWNERS file.
This PR also adds functionality to the
codeowner_analyzer.py
:--depth
: Maximum directory depth for module detection (default: 3)--top-n
: Number of top owners to include in CODEOWNERS file (default: 3)Example can be found at https://github.com/flashinfer-ai/flashinfer/pull/1871/files
🔍 Related Issues
🚀 Pull Request Checklist
Thank you for contributing to FlashInfer! Before we review your pull request, please make sure the following items are complete.
✅ Pre-commit Checks
pre-commit
by runningpip install pre-commit
(or used your preferred method).pre-commit install
.pre-commit run --all-files
and fixed any reported issues.🧪 Tests
unittest
, etc.).Reviewer Notes
cc @sricketts @yongwww @nvmbreughe @bkryu @dierksen