t295: Issue-sync label passthrough — replace hardcoded allowlist with on-the-fly label creation#1155
t295: Issue-sync label passthrough — replace hardcoded allowlist with on-the-fly label creation#1155marcusquinn wants to merge 1 commit intomainfrom
Conversation
- Replace hardcoded tag-to-label allowlist with passthrough that creates labels on-the-fly - Add create_label_if_needed() function using gh label create --force - Update map_tags_to_labels() to accept repo_slug and create labels during issue creation - Add label sync to cmd_enrich() to update existing issues with new labels from TODO.md tags - All tags now become labels instead of silently dropping unknown ones
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @marcusquinn, 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 significantly enhances the issue synchronization process by transitioning from a restrictive, hardcoded tag-to-label mapping to a flexible, on-the-fly label creation system. This change ensures that all tags from TODO.md are accurately reflected as labels on GitHub issues, preventing silent drops of important metadata. It also introduces automatic label creation with consistent coloring and improves the synchronization of labels for existing issues, making the system more robust and user-friendly. Highlights
Changelog
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, replacing the hardcoded label allowlist with a dynamic, on-the-fly label creation system. This enhances the flexibility and robustness of the issue synchronization process. The implementation is well-structured, but I have identified a few areas for improvement. My review comments focus on enhancing shell script portability, adhering to the repository's style guide for error handling, and simplifying a piece of redundant logic. These suggestions aim to make the script more robust and easier to debug.
| local color | ||
| color=$(echo -n "$label_name" | md5sum | cut -c1-6) |
There was a problem hiding this comment.
md5sum is a GNU coreutils command and is not available on macOS by default, which uses md5. This could lead to portability issues. The style guide emphasizes portability with the sed_inplace wrapper (line 20). To ensure cross-platform compatibility, it would be best to handle both cases, for example by checking for the command's existence.
| local color | |
| color=$(echo -n "$label_name" | md5sum | cut -c1-6) | |
| local color | |
| if command -v md5sum >/dev/null; then | |
| color=$(echo -n "$label_name" | md5sum | cut -c1-6) | |
| else | |
| color=$(echo -n "$label_name" | md5 | cut -c1-6) | |
| fi |
References
- The style guide recommends using wrappers like
sed_inplacefor macOS/Linux portability, suggesting that cross-platform compatibility is a concern. (link)
| local color | ||
| color=$(echo -n "$label_name" | md5sum | cut -c1-6) | ||
|
|
||
| gh label create "$label_name" --repo "$repo_slug" --color "$color" --force 2>/dev/null || true |
There was a problem hiding this comment.
The use of 2>/dev/null here suppresses all error messages from gh label create. This violates the repository style guide (line 50), which states that 2>/dev/null should not be used for blanket suppression. While --force makes the command idempotent for existing labels, other errors (like authentication failures or an invalid repository slug) will be silently ignored, making debugging difficult. The || true already prevents the script from exiting on failure.
| gh label create "$label_name" --repo "$repo_slug" --color "$color" --force 2>/dev/null || true | |
| gh label create "$label_name" --repo "$repo_slug" --color "$color" --force || true |
References
- The style guide states that
2>/dev/nullis acceptable only when redirecting to log files, and not for blanket suppression of errors. (link)
| if [[ -n "$labels" ]]; then | ||
| labels="${labels},${status_label}" | ||
| else | ||
| labels="$status_label" | ||
| fi |
There was a problem hiding this comment.
The if [[ -n "$labels" ]] check on line 961 is redundant because it's nested inside an identical check on line 955. This makes the else block on line 964 unreachable. The code can be simplified by removing the inner conditional.
| if [[ -n "$labels" ]]; then | |
| labels="${labels},${status_label}" | |
| else | |
| labels="$status_label" | |
| fi | |
| labels="${labels},${status_label}" |
| labels="$status_label" | ||
| fi | ||
| # Update issue labels | ||
| if gh issue edit "$issue_number" --repo "$repo_slug" --add-label "$labels" 2>/dev/null; then |
There was a problem hiding this comment.
Similar to a previous comment, the use of 2>/dev/null here suppresses potentially useful error messages from gh issue edit. This violates the repository style guide (line 50). If the command fails due to issues like invalid permissions or network problems, the error will be hidden, complicating troubleshooting. Since gh issue edit --add-label doesn't fail if labels already exist, simply checking the command's success in the if statement is sufficient.
| if gh issue edit "$issue_number" --repo "$repo_slug" --add-label "$labels" 2>/dev/null; then | |
| if gh issue edit "$issue_number" --repo "$repo_slug" --add-label "$labels"; then |
References
- The style guide states that
2>/dev/nullis acceptable only when redirecting to log files, and not for blanket suppression of errors. (link)
|
Stale duplicate — t295 was already merged via PR #1154. Closing. |
1 participant
Summary
Implements label passthrough for issue-sync to eliminate the hardcoded tag-to-label allowlist (~20 tags) and instead create labels on-the-fly via
gh label create --force.Changes
1. New
create_label_if_needed()functiongh label create --force2. Updated
map_tags_to_labels()functionrepo_slugparameter3. Enhanced
cmd_enrich()functionBenefits
Testing
Related Issues
Ref #295