Add automatic labelling of turbolift-raised PRs, with opt-out #197
Conversation
For ease of tracking (instead of relying on full-text search of PR descriptions)
There was a problem hiding this comment.
Pull request overview
This PR adds automatic labeling of turbolift-raised PRs with a "turbolift" label by default, while allowing users to opt out via a --no-apply-labels flag.
Key changes:
- Introduced
TurboliftLabelconstant andLabelsfield inPullRequeststruct - Added
--no-apply-labelsCLI flag to skip default labeling behavior - Updated tests to verify both default labeling and opt-out scenarios
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/github/github.go | Added TurboliftLabel constant, Labels field to PullRequest struct, and label application logic in CreatePullRequest |
| internal/github/github_test.go | Updated existing test expectations to include default label; added new test for label-free PR creation |
| internal/github/fake_github.go | Modified CreatePullRequest to capture labels in fake implementation for test verification |
| cmd/create_prs/create_prs.go | Added --no-apply-labels flag and logic to conditionally apply default label |
| cmd/create_prs/create_prs_test.go | Updated test expectations and added new test case for --no-apply-labels flag |
| README.md | Documented new default labeling behavior and opt-out flag; clarified development task descriptions |
| AGENTS.md | Added new documentation file with repository guidelines for AI agents |
💡 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 6 out of 6 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/github/github.go
Outdated
| func (r *RealGitHub) ensureLabelExists(output io.Writer, workingDir string, repo string, label string) error { | ||
| args := []string{ | ||
| "label", | ||
| "create", | ||
| label, | ||
| "--repo", | ||
| repo, | ||
| "--color", | ||
| turboliftLabelColor, | ||
| "--description", | ||
| turboliftLabelDescription, | ||
| } |
There was a problem hiding this comment.
The color and description are hardcoded for all labels, but the function accepts any label name. This creates inconsistency when non-turbolift labels are used. Consider either: (1) making this function turbolift-specific by removing the label parameter and using TurboliftLabel constant directly, or (2) accepting color and description as parameters if generic label support is intended.
internal/github/github.go
Outdated
| if len(pr.Labels) > 0 { | ||
| for _, label := range pr.Labels { | ||
| if err := r.ensureLabelExists(output, workingDir, pr.UpstreamRepo, label); err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
The outer if len(pr.Labels) > 0 check is unnecessary. The for-range loop over pr.Labels already handles empty slices gracefully by not iterating. This check can be removed to simplify the code.
| if len(pr.Labels) > 0 { | |
| for _, label := range pr.Labels { | |
| if err := r.ensureLabelExists(output, workingDir, pr.UpstreamRepo, label); err != nil { | |
| return false, err | |
| } | |
| for _, label := range pr.Labels { | |
| if err := r.ensureLabelExists(output, workingDir, pr.UpstreamRepo, label); err != nil { | |
| return false, err |
There was a problem hiding this comment.
good copilot
|
|
||
| "github.com/skyscanner/turbolift/internal/executor" | ||
| ) | ||
|
|
There was a problem hiding this comment.
funny, github doesn't show any syntax highlighting on this one?
| "--repo", | ||
| repo, | ||
| "--color", | ||
| turboliftLabelColor, |
There was a problem hiding this comment.
I wonder if we should set the colour here - is this a mandator option when creating the label?
- Remove unnecessary len check before iteration (for-range handles empty slices) - Rename ensureLabelExists to ensureTurboliftLabelExists and make it turbolift-specific - Use TurboliftLabel constant directly instead of passing as parameter - Keep fixed color (0366d6 - blue) for turbolift labels 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The Labels field was only ever set to []string{TurboliftLabel} or nil,
effectively acting as a boolean flag. This refactor:
- Replaces Labels []string with ApplyLabels bool in PullRequest struct
- Simplifies CreatePullRequest logic - no more iteration over labels
- Makes intent clearer in create_prs.go: ApplyLabels: !noApplyLabels
- Updates fake implementation to use boolean check
- Updates all tests to use the new boolean field
This eliminates unnecessary complexity and makes the code more maintainable.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <[email protected]>
Summary
Intended to make it easy to search for turbolift-raised PRs by tools that support querying by labels (e.g. Cortex).
--no-apply-labelsflag so create-prs can skip default labeling