Skip to content

Commit 891f51f

Browse files
skarimskarim
committed
add to stack api
1 parent d4adefe commit 891f51f

5 files changed

Lines changed: 230 additions & 41 deletions

File tree

‎cmd/push.go‎

Lines changed: 66 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/cli/go-gh/v2/pkg/prompter"
1111
"github.com/github/gh-stack/internal/config"
1212
"github.com/github/gh-stack/internal/git"
13+
"github.com/github/gh-stack/internal/github"
1314
"github.com/github/gh-stack/internal/stack"
1415
"github.com/spf13/cobra"
1516
)
@@ -168,7 +169,7 @@ func runPush(cfg *config.Config, opts *pushOptions) error {
168169
}
169170

170171
// Create or update the stack on GitHub
171-
createStack(cfg, client, s)
172+
syncStack(cfg, client, s)
172173

173174
// Update base commit hashes and sync PR state
174175
updateBaseSHAs(s)
@@ -257,10 +258,13 @@ func pickRemote(cfg *config.Config, branch, remoteOverride string) (string, erro
257258
return multi.Remotes[selected], nil
258259
}
259260

260-
// createStack attempts to create a stack on GitHub from the active PRs.
261-
// It is a best-effort operation: failures are reported as warnings but do
261+
// syncStack creates or updates a stack on GitHub from the active PRs.
262+
// If the stack already exists (s.ID is set), it calls the PUT endpoint with
263+
// the full list of PRs to keep the remote stack in sync. If no stack exists
264+
// yet, it calls POST to create one.
265+
// This is a best-effort operation: failures are reported as warnings but do
262266
// not cause the push command to fail (the PRs are already created).
263-
func createStack(cfg *config.Config, client interface{ CreateStack([]int) (int, error) }, s *stack.Stack) {
267+
func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) {
264268
// Collect PR numbers in stack order (bottom to top).
265269
var prNumbers []int
266270
for _, b := range s.Branches {
@@ -277,34 +281,81 @@ func createStack(cfg *config.Config, client interface{ CreateStack([]int) (int,
277281
return
278282
}
279283

284+
if s.ID != "" {
285+
updateStack(cfg, client, s, prNumbers)
286+
} else {
287+
createNewStack(cfg, client, s, prNumbers)
288+
}
289+
}
290+
291+
// updateStack calls the PUT endpoint to sync the full PR list for an existing stack.
292+
func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) {
293+
if err := client.UpdateStack(s.ID, prNumbers); err != nil {
294+
var httpErr *api.HTTPError
295+
if errors.As(err, &httpErr) {
296+
switch httpErr.StatusCode {
297+
case 404:
298+
cfg.Warningf("Stack not found on GitHub (ID %s) — it may have been deleted", s.ID)
299+
cfg.Printf(" Run %s to re-create the stack on your next push.",
300+
cfg.ColorCyan("gh stack push"))
301+
s.ID = "" // Clear stale ID so the next push creates a new stack
302+
default:
303+
cfg.Warningf("Failed to update stack on GitHub: %s", httpErr.Message)
304+
}
305+
} else {
306+
cfg.Warningf("Failed to update stack on GitHub: %v", err)
307+
}
308+
return
309+
}
310+
cfg.Successf("Stack updated on GitHub with %d PRs", len(prNumbers))
311+
}
312+
313+
// createNewStack calls the POST endpoint to create a new stack, handling the
314+
// three types of 422 errors the API may return.
315+
func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) {
280316
stackID, err := client.CreateStack(prNumbers)
281317
if err == nil {
282318
s.ID = strconv.Itoa(stackID)
283319
cfg.Successf("Stack created on GitHub with %d PRs", len(prNumbers))
284320
return
285321
}
286322

287-
cfg.Warningf("Failed to create stack on GitHub: %v", err)
288323
var httpErr *api.HTTPError
289324
if !errors.As(err, &httpErr) {
290325
cfg.Warningf("Failed to create stack on GitHub: %v", err)
291326
return
292327
}
293328

294-
cfg.Warningf("error %s with code %s", httpErr.StatusCode, httpErr.Message)
295329
switch httpErr.StatusCode {
296330
case 422:
297-
if s.ID != "" {
298-
// Stack was already created in a previous push; the update API
299-
// (PUT) is needed to modify it but is not yet available.
300-
cfg.Infof("Stack already exists on GitHub")
301-
} else {
302-
cfg.Warningf("Could not create stack: %s", httpErr.Message)
303-
cfg.Printf(" Updating existing stacks will be supported in an upcoming release.")
304-
}
331+
handleCreate422(cfg, httpErr)
305332
case 404:
306-
cfg.Warningf("Stacked PRs are not enabled for this repository")
333+
cfg.Warningf("Stacked PRs are not yet available for this repository")
307334
default:
308335
cfg.Warningf("Failed to create stack on GitHub: %s", httpErr.Message)
309336
}
310337
}
338+
339+
// handleCreate422 handles 422 errors from the create stack endpoint.
340+
// The three known error messages are:
341+
// - "Stack must contain at least two pull requests"
342+
// - "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref"
343+
// - "Pull requests #123, #124, #125 are already stacked"
344+
func handleCreate422(cfg *config.Config, httpErr *api.HTTPError) {
345+
msg := httpErr.Message
346+
347+
if strings.Contains(msg, "already stacked") {
348+
cfg.Warningf("%s", msg)
349+
cfg.Printf(" The stack already exists on GitHub. Updating existing stacks will be supported in an upcoming release.")
350+
return
351+
}
352+
353+
if strings.Contains(msg, "must form a stack") {
354+
cfg.Errorf("Cannot create stack: %s", msg)
355+
cfg.Printf(" Each PR's base branch must match the previous PR's head branch.")
356+
return
357+
}
358+
359+
// "at least two" or any other validation error
360+
cfg.Warningf("Could not create stack: %s", msg)
361+
}

‎cmd/push_test.go‎

Lines changed: 129 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,7 @@ func TestPush_Humanize(t *testing.T) {
228228
}
229229
}
230230

231-
func TestCreateStack_Success(t *testing.T) {
231+
func TestSyncStack_NewStack_CreateSuccess(t *testing.T) {
232232
s := &stack.Stack{
233233
Trunk: stack.BranchRef{Branch: "main"},
234234
Branches: []stack.BranchRef{
@@ -246,7 +246,7 @@ func TestCreateStack_Success(t *testing.T) {
246246
}
247247

248248
cfg, _, errR := config.NewTestConfig()
249-
createStack(cfg, mock, s)
249+
syncStack(cfg, mock, s)
250250

251251
cfg.Err.Close()
252252
errOut, _ := io.ReadAll(errR)
@@ -257,9 +257,109 @@ func TestCreateStack_Success(t *testing.T) {
257257
assert.Contains(t, output, "Stack created on GitHub with 2 PRs")
258258
}
259259

260-
func TestCreateStack_AlreadyExists_WithLocalID(t *testing.T) {
260+
func TestSyncStack_ExistingStack_UpdateSuccess(t *testing.T) {
261261
s := &stack.Stack{
262262
ID: "99",
263+
Trunk: stack.BranchRef{Branch: "main"},
264+
Branches: []stack.BranchRef{
265+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}},
266+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}},
267+
{Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}},
268+
},
269+
}
270+
271+
var gotStackID string
272+
var gotNumbers []int
273+
createCalled := false
274+
mock := &github.MockClient{
275+
CreateStackFn: func([]int) (int, error) {
276+
createCalled = true
277+
return 0, nil
278+
},
279+
UpdateStackFn: func(stackID string, prNumbers []int) error {
280+
gotStackID = stackID
281+
gotNumbers = prNumbers
282+
return nil
283+
},
284+
}
285+
286+
cfg, _, errR := config.NewTestConfig()
287+
syncStack(cfg, mock, s)
288+
289+
cfg.Err.Close()
290+
errOut, _ := io.ReadAll(errR)
291+
output := string(errOut)
292+
293+
assert.False(t, createCalled, "CreateStack should not be called when s.ID is set")
294+
assert.Equal(t, "99", gotStackID)
295+
assert.Equal(t, []int{10, 11, 12}, gotNumbers)
296+
assert.Contains(t, output, "Stack updated on GitHub with 3 PRs")
297+
}
298+
299+
func TestSyncStack_ExistingStack_UpdateFails(t *testing.T) {
300+
s := &stack.Stack{
301+
ID: "99",
302+
Trunk: stack.BranchRef{Branch: "main"},
303+
Branches: []stack.BranchRef{
304+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}},
305+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}},
306+
},
307+
}
308+
309+
mock := &github.MockClient{
310+
UpdateStackFn: func(string, []int) error {
311+
return &api.HTTPError{
312+
StatusCode: 422,
313+
Message: "Validation failed",
314+
RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"},
315+
}
316+
},
317+
}
318+
319+
cfg, _, errR := config.NewTestConfig()
320+
syncStack(cfg, mock, s)
321+
322+
cfg.Err.Close()
323+
errOut, _ := io.ReadAll(errR)
324+
output := string(errOut)
325+
326+
assert.Contains(t, output, "Failed to update stack")
327+
}
328+
329+
func TestSyncStack_ExistingStack_Update404(t *testing.T) {
330+
s := &stack.Stack{
331+
ID: "99",
332+
Trunk: stack.BranchRef{Branch: "main"},
333+
Branches: []stack.BranchRef{
334+
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}},
335+
{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}},
336+
},
337+
}
338+
339+
mock := &github.MockClient{
340+
UpdateStackFn: func(string, []int) error {
341+
return &api.HTTPError{
342+
StatusCode: 404,
343+
Message: "Not Found",
344+
RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"},
345+
}
346+
},
347+
}
348+
349+
cfg, _, errR := config.NewTestConfig()
350+
syncStack(cfg, mock, s)
351+
352+
cfg.Err.Close()
353+
errOut, _ := io.ReadAll(errR)
354+
output := string(errOut)
355+
356+
assert.Contains(t, output, "Stack not found on GitHub")
357+
assert.Contains(t, output, "may have been deleted")
358+
assert.Equal(t, "", s.ID, "stale ID should be cleared so next push creates a new stack")
359+
}
360+
361+
func TestSyncStack_AlreadyStacked_422(t *testing.T) {
362+
s := &stack.Stack{
263363
Trunk: stack.BranchRef{Branch: "main"},
264364
Branches: []stack.BranchRef{
265365
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}},
@@ -271,24 +371,24 @@ func TestCreateStack_AlreadyExists_WithLocalID(t *testing.T) {
271371
CreateStackFn: func([]int) (int, error) {
272372
return 0, &api.HTTPError{
273373
StatusCode: 422,
274-
Message: "Pull requests are already in a stack",
374+
Message: "Pull requests #10, #11 are already stacked",
275375
RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"},
276376
}
277377
},
278378
}
279379

280380
cfg, _, errR := config.NewTestConfig()
281-
createStack(cfg, mock, s)
381+
syncStack(cfg, mock, s)
282382

283383
cfg.Err.Close()
284384
errOut, _ := io.ReadAll(errR)
285385
output := string(errOut)
286386

287-
assert.Contains(t, output, "Stack already exists on GitHub")
288-
assert.Equal(t, "99", s.ID)
387+
assert.Contains(t, output, "already stacked")
388+
assert.Contains(t, output, "Updating existing stacks will be supported in an upcoming release")
289389
}
290390

291-
func TestCreateStack_AlreadyExists_NoLocalID(t *testing.T) {
391+
func TestSyncStack_InvalidChain_422(t *testing.T) {
292392
s := &stack.Stack{
293393
Trunk: stack.BranchRef{Branch: "main"},
294394
Branches: []stack.BranchRef{
@@ -301,24 +401,24 @@ func TestCreateStack_AlreadyExists_NoLocalID(t *testing.T) {
301401
CreateStackFn: func([]int) (int, error) {
302402
return 0, &api.HTTPError{
303403
StatusCode: 422,
304-
Message: "Pull requests are already in a stack",
404+
Message: "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref",
305405
RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"},
306406
}
307407
},
308408
}
309409

310410
cfg, _, errR := config.NewTestConfig()
311-
createStack(cfg, mock, s)
411+
syncStack(cfg, mock, s)
312412

313413
cfg.Err.Close()
314414
errOut, _ := io.ReadAll(errR)
315415
output := string(errOut)
316416

317-
assert.Contains(t, output, "Could not create stack")
318-
assert.Contains(t, output, "Updating existing stacks will be supported in an upcoming release")
417+
assert.Contains(t, output, "must form a stack")
418+
assert.Contains(t, output, "base branch must match")
319419
}
320420

321-
func TestCreateStack_NotAvailable(t *testing.T) {
421+
func TestSyncStack_NotAvailable(t *testing.T) {
322422
s := &stack.Stack{
323423
Trunk: stack.BranchRef{Branch: "main"},
324424
Branches: []stack.BranchRef{
@@ -338,7 +438,7 @@ func TestCreateStack_NotAvailable(t *testing.T) {
338438
}
339439

340440
cfg, _, errR := config.NewTestConfig()
341-
createStack(cfg, mock, s)
441+
syncStack(cfg, mock, s)
342442

343443
cfg.Err.Close()
344444
errOut, _ := io.ReadAll(errR)
@@ -347,30 +447,36 @@ func TestCreateStack_NotAvailable(t *testing.T) {
347447
assert.Contains(t, output, "not yet available")
348448
}
349449

350-
func TestCreateStack_SkippedForSinglePR(t *testing.T) {
450+
func TestSyncStack_SkippedForSinglePR(t *testing.T) {
351451
s := &stack.Stack{
352452
Trunk: stack.BranchRef{Branch: "main"},
353453
Branches: []stack.BranchRef{
354454
{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}},
355455
},
356456
}
357457

358-
called := false
458+
createCalled := false
459+
updateCalled := false
359460
mock := &github.MockClient{
360461
CreateStackFn: func([]int) (int, error) {
361-
called = true
462+
createCalled = true
362463
return 42, nil
363464
},
465+
UpdateStackFn: func(string, []int) error {
466+
updateCalled = true
467+
return nil
468+
},
364469
}
365470

366471
cfg, _, _ := config.NewTestConfig()
367-
createStack(cfg, mock, s)
472+
syncStack(cfg, mock, s)
368473
cfg.Err.Close()
369474

370-
assert.False(t, called, "CreateStack should not be called with fewer than 2 PRs")
475+
assert.False(t, createCalled, "CreateStack should not be called with fewer than 2 PRs")
476+
assert.False(t, updateCalled, "UpdateStack should not be called with fewer than 2 PRs")
371477
}
372478

373-
func TestCreateStack_SkipsMergedBranches(t *testing.T) {
479+
func TestSyncStack_SkipsMergedBranches(t *testing.T) {
374480
s := &stack.Stack{
375481
Trunk: stack.BranchRef{Branch: "main"},
376482
Branches: []stack.BranchRef{
@@ -389,13 +495,13 @@ func TestCreateStack_SkipsMergedBranches(t *testing.T) {
389495
}
390496

391497
cfg, _, _ := config.NewTestConfig()
392-
createStack(cfg, mock, s)
498+
syncStack(cfg, mock, s)
393499
cfg.Err.Close()
394500

395501
assert.Equal(t, []int{11, 12}, gotNumbers, "should only include non-merged PRs")
396502
}
397503

398-
func TestCreateStack_SkipsBranchesWithoutPR(t *testing.T) {
504+
func TestSyncStack_SkipsBranchesWithoutPR(t *testing.T) {
399505
s := &stack.Stack{
400506
Trunk: stack.BranchRef{Branch: "main"},
401507
Branches: []stack.BranchRef{
@@ -414,7 +520,7 @@ func TestCreateStack_SkipsBranchesWithoutPR(t *testing.T) {
414520
}
415521

416522
cfg, _, _ := config.NewTestConfig()
417-
createStack(cfg, mock, s)
523+
syncStack(cfg, mock, s)
418524
cfg.Err.Close()
419525

420526
assert.Equal(t, []int{10, 12}, gotNumbers, "should skip branches without PRs")

0 commit comments

Comments
 (0)