Skip to content

Commit f62b3f2

Browse files
abhinavclaude
andcommitted
submit: Add spice.submit.reviewers.addWhen config option
Add a new configuration option to control when configured reviewers are added to change requests. When set to "ready", reviewers configured via spice.submit.reviewers are skipped for draft CRs. Reviewers specified via the --reviewer flag are always added regardless of this setting. Accepted values: - "always" (default): add configured reviewers to all CRs - "ready": only add configured reviewers when the CR is not a draft Fixes #968 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent a57205a commit f62b3f2

File tree

5 files changed

+439
-10
lines changed

5 files changed

+439
-10
lines changed
Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
kind: Added
2+
body: >-
3+
submit: Add `spice.submit.reviewers.addWhen` configuration option,
4+
which allows controlling whether reviewers configured with
5+
`spice.submit.reviewers` are added to draft change requests.
6+
time: 2026-02-01T18:10:01.526545-08:00

doc/src/cli/config.md

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -363,9 +363,24 @@ The value must be a comma-separated list of reviewers.
363363
For GitHub, use usernames for individual reviewers
364364
or `org/team` format for team reviewers.
365365

366-
Reviewers specified with the `--reviewers` flag
366+
Reviewers specified with the `--reviewer` flag
367367
will be combined with the configured reviewers.
368368

369+
### spice.submit.reviewers.addWhen
370+
371+
<!-- gs:version unreleased -->
372+
373+
Controls when $$spice.submit.reviewers|configured reviewers$$
374+
are added to change requests.
375+
376+
When set to `ready`, configured reviewers are skipped for draft CRs,
377+
except when added explicitly via the `--reviewer` flag.
378+
379+
**Accepted values:**
380+
381+
- `always` (default): add configured reviewers to all CRs
382+
- `ready`: only add configured reviewers when the CR is not a draft
383+
369384
### spice.submit.listTemplatesTimeout
370385

371386
<!-- gs:version v0.8.0 -->

internal/handler/submit/handler.go

Lines changed: 77 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -160,8 +160,9 @@ type Options struct {
160160
Labels []string `name:"label" short:"l" help:"Add labels to the change request. Pass multiple times or separate with commas."`
161161
ConfiguredLabels []string `name:"configured-labels" help:"Default labels to add to change requests." hidden:"" config:"submit.label"` // merged with Labels
162162

163-
Reviewers []string `short:"r" name:"reviewer" help:"Add reviewers to the change request. Pass multiple times or separate with commas." released:"v0.21.0"`
164-
ConfiguredReviewers []string `name:"configured-reviewers" help:"Default reviewers to add to change requests." hidden:"" config:"submit.reviewers" released:"v0.21.0"` // merged with Reviewers
163+
Reviewers []string `short:"r" name:"reviewer" help:"Add reviewers to the change request. Pass multiple times or separate with commas." released:"v0.21.0"`
164+
ConfiguredReviewers []string `name:"configured-reviewers" help:"Default reviewers to add to change requests." hidden:"" config:"submit.reviewers" released:"v0.21.0"`
165+
ReviewersAddWhen ReviewersAddWhen `name:"reviewers-add-when" help:"When to add configured reviewers." hidden:"" config:"submit.reviewers.addWhen" default:"always" released:"unreleased"`
165166

166167
Assignees []string `short:"a" name:"assign" placeholder:"ASSIGNEE" help:"Assign the change request to these users. Pass multiple times or separate with commas." released:"v0.21.0"`
167168
ConfiguredAssignees []string `name:"configured-assignees" help:"Default assignees to add to change requests." hidden:"" config:"submit.assignees" released:"v0.21.0"` // merged with Assignees
@@ -181,10 +182,24 @@ func mergeConfiguredValues(values []string, configured []string) []string {
181182

182183
func mergeConfiguredOptions(opts *Options) {
183184
opts.Labels = mergeConfiguredValues(opts.Labels, opts.ConfiguredLabels)
184-
opts.Reviewers = mergeConfiguredValues(opts.Reviewers, opts.ConfiguredReviewers)
185+
// Note: Reviewers are merged conditionally by effectiveReviewers
186+
// based on draft status and ReviewersAddWhen setting.
185187
opts.Assignees = mergeConfiguredValues(opts.Assignees, opts.ConfiguredAssignees)
186188
}
187189

190+
// effectiveReviewers returns the reviewers to add to a change request.
191+
// Flag-specified reviewers are always included.
192+
// Configured reviewers are included based on the ReviewersAddWhen setting
193+
// and the draft status of the change request.
194+
func effectiveReviewers(opts *Options, isDraft bool) []string {
195+
// If addWhen is "ready" and the CR is a draft,
196+
// skip configured reviewers.
197+
if opts.ReviewersAddWhen == ReviewersAddWhenReady && isDraft {
198+
return opts.Reviewers
199+
}
200+
return mergeConfiguredValues(opts.Reviewers, opts.ConfiguredReviewers)
201+
}
202+
188203
// NavCommentSync specifies the scope of navigation comment updates.
189204
type NavCommentSync int
190205

@@ -271,6 +286,50 @@ func (d *NavCommentDownstack) UnmarshalText(bs []byte) error {
271286
return nil
272287
}
273288

289+
// ReviewersAddWhen specifies when configured reviewers
290+
// should be added to change requests.
291+
type ReviewersAddWhen int
292+
293+
const (
294+
// ReviewersAddWhenAlways adds configured reviewers
295+
// to all change requests regardless of draft status.
296+
//
297+
// This is the default.
298+
ReviewersAddWhenAlways ReviewersAddWhen = iota
299+
300+
// ReviewersAddWhenReady adds configured reviewers
301+
// only when the change request is not a draft.
302+
ReviewersAddWhenReady
303+
)
304+
305+
var _ encoding.TextUnmarshaler = (*ReviewersAddWhen)(nil)
306+
307+
// String returns the string representation of the ReviewersAddWhen.
308+
func (r ReviewersAddWhen) String() string {
309+
switch r {
310+
case ReviewersAddWhenAlways:
311+
return "always"
312+
case ReviewersAddWhenReady:
313+
return "ready"
314+
default:
315+
return "unknown"
316+
}
317+
}
318+
319+
// UnmarshalText decodes ReviewersAddWhen from text.
320+
// It supports "always" and "ready" values.
321+
func (r *ReviewersAddWhen) UnmarshalText(bs []byte) error {
322+
switch string(bs) {
323+
case "always":
324+
*r = ReviewersAddWhenAlways
325+
case "ready":
326+
*r = ReviewersAddWhenReady
327+
default:
328+
return fmt.Errorf("invalid value %q: expected always or ready", bs)
329+
}
330+
return nil
331+
}
332+
274333
// BatchOptions defines options
275334
// that are only available to batch submit operations.
276335
type BatchOptions struct {
@@ -824,6 +883,15 @@ func (h *Handler) submitBranch(
824883
updates = append(updates, "set draft to "+strconv.FormatBool(*opts.Draft))
825884
}
826885

886+
// Determine the effective draft status for reviewer handling.
887+
// If user is changing draft status, use the new value;
888+
// otherwise, use the current draft status.
889+
effectiveDraft := pull.Draft
890+
if opts.Draft != nil {
891+
effectiveDraft = *opts.Draft
892+
}
893+
reviewers := effectiveReviewers(opts.Options, effectiveDraft)
894+
827895
// TODO:
828896
// We _probably_ don't need to check for existing
829897
// reviewers, assignees, etc. because the API contract
@@ -866,13 +934,13 @@ func (h *Handler) submitBranch(
866934
}
867935

868936
// Check for reviewers that would be added.
869-
if len(opts.Reviewers) > 0 {
937+
if len(reviewers) > 0 {
870938
existingReviewerSet := make(map[string]struct{}, len(pull.Reviewers))
871939
for _, reviewer := range pull.Reviewers {
872940
existingReviewerSet[reviewer] = struct{}{}
873941
}
874942
var reviewersToAdd []string
875-
for _, reviewer := range opts.Reviewers {
943+
for _, reviewer := range reviewers {
876944
if _, exists := existingReviewerSet[reviewer]; !exists {
877945
reviewersToAdd = append(reviewersToAdd, reviewer)
878946
}
@@ -921,11 +989,11 @@ func (h *Handler) submitBranch(
921989
}
922990

923991
if len(updates) > 0 {
924-
opts := forge.EditChangeOptions{
992+
editOpts := forge.EditChangeOptions{
925993
Base: upstreamBase,
926994
Draft: opts.Draft,
927995
AddLabels: opts.Labels,
928-
AddReviewers: opts.Reviewers,
996+
AddReviewers: reviewers,
929997
AddAssignees: opts.Assignees,
930998
}
931999

@@ -935,7 +1003,7 @@ func (h *Handler) submitBranch(
9351003
return status, fmt.Errorf("edit CR %v: %w", pull.ID, err)
9361004
}
9371005

938-
if err := remoteRepo.EditChange(ctx, pull.ID, opts); err != nil {
1006+
if err := remoteRepo.EditChange(ctx, pull.ID, editOpts); err != nil {
9391007
return status, fmt.Errorf("edit CR %v: %w", pull.ID, err)
9401008
}
9411009
}
@@ -1129,7 +1197,7 @@ func (h *Handler) prepareBranch(
11291197
store: h.Store,
11301198
log: h.Log,
11311199
labels: opts.Labels,
1132-
reviewers: opts.Reviewers,
1200+
reviewers: effectiveReviewers(opts.Options, draft),
11331201
assignees: opts.Assignees,
11341202
}, nil
11351203
}

internal/handler/submit/handler_test.go

Lines changed: 139 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"time"
88

99
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
1011
"go.abhg.dev/gs/internal/forge"
1112
"go.abhg.dev/gs/internal/silog/silogtest"
1213
gomock "go.uber.org/mock/gomock"
@@ -54,3 +55,141 @@ func TestBranchSubmit_listChangeTemplates(t *testing.T) {
5455
assert.Empty(t, got)
5556
})
5657
}
58+
59+
func TestReviewersAddWhen_UnmarshalText(t *testing.T) {
60+
tests := []struct {
61+
name string
62+
input string
63+
want ReviewersAddWhen
64+
wantErr string
65+
}{
66+
{
67+
name: "Always",
68+
input: "always",
69+
want: ReviewersAddWhenAlways,
70+
},
71+
{
72+
name: "Ready",
73+
input: "ready",
74+
want: ReviewersAddWhenReady,
75+
},
76+
{
77+
name: "Invalid",
78+
input: "never",
79+
wantErr: `invalid value "never": expected always or ready`,
80+
},
81+
}
82+
83+
for _, tt := range tests {
84+
t.Run(tt.name, func(t *testing.T) {
85+
var got ReviewersAddWhen
86+
err := got.UnmarshalText([]byte(tt.input))
87+
88+
if tt.wantErr != "" {
89+
require.EqualError(t, err, tt.wantErr)
90+
return
91+
}
92+
93+
require.NoError(t, err)
94+
assert.Equal(t, tt.want, got)
95+
})
96+
}
97+
}
98+
99+
func TestReviewersAddWhen_String(t *testing.T) {
100+
tests := []struct {
101+
name string
102+
value ReviewersAddWhen
103+
want string
104+
}{
105+
{name: "Always", value: ReviewersAddWhenAlways, want: "always"},
106+
{name: "Ready", value: ReviewersAddWhenReady, want: "ready"},
107+
{name: "Unknown", value: ReviewersAddWhen(99), want: "unknown"},
108+
}
109+
110+
for _, tt := range tests {
111+
t.Run(tt.name, func(t *testing.T) {
112+
assert.Equal(t, tt.want, tt.value.String())
113+
})
114+
}
115+
}
116+
117+
func TestEffectiveReviewers(t *testing.T) {
118+
tests := []struct {
119+
name string
120+
addWhen ReviewersAddWhen
121+
isDraft bool
122+
flagReviewers []string
123+
configuredReviewers []string
124+
want []string
125+
}{
126+
{
127+
name: "AlwaysDraft",
128+
addWhen: ReviewersAddWhenAlways,
129+
isDraft: true,
130+
flagReviewers: []string{"alice"},
131+
configuredReviewers: []string{"bob"},
132+
want: []string{"alice", "bob"},
133+
},
134+
{
135+
name: "AlwaysReady",
136+
addWhen: ReviewersAddWhenAlways,
137+
isDraft: false,
138+
flagReviewers: []string{"alice"},
139+
configuredReviewers: []string{"bob"},
140+
want: []string{"alice", "bob"},
141+
},
142+
{
143+
name: "ReadyDraft",
144+
addWhen: ReviewersAddWhenReady,
145+
isDraft: true,
146+
flagReviewers: []string{"alice"},
147+
configuredReviewers: []string{"bob"},
148+
want: []string{"alice"},
149+
},
150+
{
151+
name: "ReadyNotDraft",
152+
addWhen: ReviewersAddWhenReady,
153+
isDraft: false,
154+
flagReviewers: []string{"alice"},
155+
configuredReviewers: []string{"bob"},
156+
want: []string{"alice", "bob"},
157+
},
158+
{
159+
name: "ReadyDraftNoFlags",
160+
addWhen: ReviewersAddWhenReady,
161+
isDraft: true,
162+
flagReviewers: nil,
163+
configuredReviewers: []string{"bob"},
164+
want: nil,
165+
},
166+
{
167+
name: "Deduplication",
168+
addWhen: ReviewersAddWhenAlways,
169+
isDraft: false,
170+
flagReviewers: []string{"alice", "bob"},
171+
configuredReviewers: []string{"bob", "charlie"},
172+
want: []string{"alice", "bob", "charlie"},
173+
},
174+
{
175+
name: "EmptyBoth",
176+
addWhen: ReviewersAddWhenAlways,
177+
isDraft: false,
178+
flagReviewers: nil,
179+
configuredReviewers: nil,
180+
want: nil,
181+
},
182+
}
183+
184+
for _, tt := range tests {
185+
t.Run(tt.name, func(t *testing.T) {
186+
opts := &Options{
187+
Reviewers: tt.flagReviewers,
188+
ConfiguredReviewers: tt.configuredReviewers,
189+
ReviewersAddWhen: tt.addWhen,
190+
}
191+
got := effectiveReviewers(opts, tt.isDraft)
192+
assert.Equal(t, tt.want, got)
193+
})
194+
}
195+
}

0 commit comments

Comments
 (0)