Skip to content

fix: Error on PR creation #414

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.

Sign up for GitHub

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

Open
juanjosevazquezgil wants to merge 22 commits into
base: main
Choose a base branch
from
Open

fix: Error on PR creation #414

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
0b5863f
Merge branch 'main' of https://github.com/prefapp/daggerverse
juanjosevazquezgil Feb 20, 2026
7b75829
Merge branch 'main' of https://github.com/prefapp/daggerverse
juanjosevazquezgil Feb 25, 2026
3c1914c
test: Add sleeps to test error
juanjosevazquezgil Feb 25, 2026
1fa5ac5
fix: Updated gh dependency
juanjosevazquezgil Feb 25, 2026
0de040c
fix: Initial version of retry loops
juanjosevazquezgil Feb 25, 2026
3ef569e
Merge branch 'main' into fix/413-fix-error-on-pr-creation
juanjosevazquezgil Feb 25, 2026
c8b0e91
fix: Updated gh dependency
juanjosevazquezgil Feb 25, 2026
3a2a99d
Merge branch 'fix/413-fix-error-on-pr-creation' of https://github.com…
juanjosevazquezgil Feb 25, 2026
4d983b8
fix: Added sleep when PR creation and checking fails
juanjosevazquezgil Feb 25, 2026
56c2832
fix: Logs
juanjosevazquezgil Feb 25, 2026
bdf4fca
fix: Updated gh dependency
juanjosevazquezgil Feb 25, 2026
3ca4e9e
test: Fail case
juanjosevazquezgil Feb 25, 2026
bbbad60
fix: Updated gh dependency
juanjosevazquezgil Feb 25, 2026
50154f4
test
juanjosevazquezgil Feb 25, 2026
7b846b4
fix: Update gh dependency
juanjosevazquezgil Feb 25, 2026
e3b5e87
fix: Restored functionality
juanjosevazquezgil Feb 25, 2026
3712723
fix: Updated gh dependency
juanjosevazquezgil Feb 25, 2026
8d9a486
fix: Unneeded sleep
juanjosevazquezgil Feb 25, 2026
d1407a8
fix: Inverted for loops conditions
juanjosevazquezgil Feb 25, 2026
81a94b4
fix: Removed magical numbers
juanjosevazquezgil Feb 25, 2026
42c9136
fix: Improved timer
juanjosevazquezgil Feb 25, 2026
b13f451
fix: Check PR ID is actually an ID
juanjosevazquezgil Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 59 additions & 17 deletions gh/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"regexp"
"strconv"
"strings"
"time"

Expand All @@ -25,6 +26,8 @@ type Gh struct {
}

var ErrorNoNewCommits error = errors.New("no new commits created")
var MaxRetries int = 5
var WaitTimeBetweenRetries time.Duration = 2 * time.Second

// Exit code 10 is used to indicate the 'no new commits' scenario.
// This value is chosen to distinguish it from standard exit codes (e.g., 0 for success, 1 for general errors).
Expand Down Expand Up @@ -349,26 +352,65 @@ func (m *Gh) CreatePR(
ctr = ctr.
WithExec(cmd)

_, err = ctr.Sync(ctx)
if err != nil {
return "", errors.New(
extractErrorMessage(err),
)
fmt.Println("\nCreating PR...")
for i := range MaxRetries {
_, err = ctr.Sync(ctx)

if err == nil {
break
}

if i == MaxRetries-1 {
return "", errors.New(
extractErrorMessage(err),
)
}

fmt.Printf("Error creating PR, retrying... (%d/%d)\n", i+1, MaxRetries-1)

timer := time.NewTimer(WaitTimeBetweenRetries)
select {
case <-timer.C:
case <-ctx.Done():
timer.Stop()
return "", ctx.Err()
}
Comment on lines +356 to +377
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry logic blindly retries all errors without distinguishing between transient failures (which should be retried) and permanent failures (which should fail immediately). For example, authentication errors, invalid repository errors, or permission errors should not be retried. Consider checking the error type or message to determine if the error is retryable before waiting and retrying.

Copilot uses AI. Check for mistakes.
Comment on lines +371 to +377
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry logic uses a fixed 2-second delay between retries. For handling timing issues with the GitHub API, an exponential backoff strategy would be more appropriate and efficient. This would reduce unnecessary load on the GitHub API and likely succeed faster in cases where GitHub needs more time to process the request. Consider implementing exponential backoff (e.g., 2s, 4s, 8s, 16s, 32s) instead of a fixed delay.

Copilot uses AI. Check for mistakes.
Comment on lines +371 to +377
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timer should be stopped after the select statement completes to free resources, regardless of which case is triggered. Currently, the timer is only stopped when the context is cancelled. If the timer fires normally (case <-timer.C), the timer resources are not explicitly cleaned up. While Go's garbage collector will eventually handle this, it's best practice to explicitly stop timers that are no longer needed. Add timer.Stop() after the select statement or use defer timer.Stop() right after creating the timer.

Copilot uses AI. Check for mistakes.
}

prId, err := ctr.
WithExec([]string{
"gh", "pr", "list",
"--head", branch,
"--json", "number",
"--jq", ".[0].number",
}).
Stdout(ctx)
var prId string
fmt.Println("Getting PR ID...")
for i := range MaxRetries {
prId, err = ctr.
WithExec([]string{
"gh", "pr", "list",
"--head", branch,
"--json", "number",
"--jq", ".[0].number",
}).
Stdout(ctx)

if err != nil {
return "", errors.New(
extractErrorMessage(err),
)
// Check if the prId is a valid integer (which means we got the PR ID successfully). If not, we retry.
_, convErr := strconv.Atoi(strings.TrimSpace(prId))

if err == nil && convErr == nil {
break
}

if i == MaxRetries-1 {
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When extractErrorMessage is called at line 401, if the error that caused the retry loop to fail was actually convErr (the strconv.Atoi error) and not err, then the wrong error message will be extracted. The code should capture and handle both error conditions. If convErr is not nil but err is nil, the error message should reflect the conversion failure rather than calling extractErrorMessage on a potentially nil err.

Suggested change
if i == MaxRetries-1 {
if i == MaxRetries-1 {
if err == nil && convErr != nil {
return "", convErr
}

Copilot uses AI. Check for mistakes.
return "", errors.New(
extractErrorMessage(err),
)
}

fmt.Printf("Error getting PR ID, retrying... (%d/%d)\n", i+1, MaxRetries-1)
Comment on lines +355 to +405
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using fmt.Println and fmt.Printf for logging in a library module is not ideal as it bypasses the caller's control over logging output and format. These print statements will always output to stdout, which may not be desired in all contexts. Consider either removing these debug statements for production code, or if logging is necessary, use a structured logging approach that can be configured by the caller (e.g., passing a logger interface).

Copilot uses AI. Check for mistakes.

timer := time.NewTimer(WaitTimeBetweenRetries)
select {
case <-timer.C:
case <-ctx.Done():
timer.Stop()
return "", ctx.Err()
}
Comment on lines +407 to +413
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timer should be stopped after the select statement completes to free resources, regardless of which case is triggered. Currently, the timer is only stopped when the context is cancelled. If the timer fires normally (case <-timer.C), the timer resources are not explicitly cleaned up. While Go's garbage collector will eventually handle this, it's best practice to explicitly stop timers that are no longer needed. Add timer.Stop() after the select statement or use defer timer.Stop() right after creating the timer.

Copilot uses AI. Check for mistakes.
}
Comment on lines +356 to 414
Copy link

Copilot AI Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry logic is duplicated between the PR creation and PR ID retrieval sections. This code duplication makes maintenance harder and increases the risk of inconsistencies. Consider extracting the retry logic into a reusable helper function that accepts a function to execute and returns its result, similar to: retryWithBackoff(ctx context.Context, operation func() error, maxRetries int, waitTime time.Duration) error. This would also make it easier to improve the retry strategy (e.g., adding exponential backoff) in one place.

Copilot uses AI. Check for mistakes.

prLink, err := ctr.
Expand Down
2 changes: 1 addition & 1 deletion update-claims-features/dagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
{
"name": "gh",
"source": "github.com/prefapp/daggerverse/gh",
"pin": "51911ece701e8151ccd8f2c29aacc71c8c4ffe5d"
"pin": "e3b5e873bdc92ce1462d1cd346abd2e255870558"
}
]
}
Loading