Skip to content
13 changes: 4 additions & 9 deletions cmd/deploy_azure.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package cmd
import (
"encoding/json"
"fmt"
"net/http"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -304,14 +303,10 @@ func runDeployAzure(cmd *cobra.Command, args []string) error {

if backendReady {
fmt.Println(" ✅ Backend is responding!")
fmt.Println("\n🔄 Triggering database migration...")
httpClient := &http.Client{Timeout: 5 * time.Second}
resp, err := httpClient.Get(deployment.BackendEndpoint + "/proceed-db-migration")
if err == nil {
resp.Body.Close()
fmt.Println(" ✅ Migration triggered")
} else {
fmt.Printf(" ⚠️ Migration may need manual trigger: %v\n", err)
if err := triggerAndWaitForMigration(deployment.BackendEndpoint); err != nil {
fmt.Printf(" ⚠️ %v\n", err)
fmt.Printf(" Trigger migration manually if needed: GET %s/proceed-db-migration\n", deployment.BackendEndpoint)
fmt.Println(" Migration may still be running — proceeding anyway")
}
} else {
fmt.Println(" Backend not ready after 30 attempts.")
Expand Down
16 changes: 4 additions & 12 deletions cmd/deploy_local.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"strings"
"time"

"github.com/DevExpGBB/gh-devlake/internal/devlake"
dockerpkg "github.com/DevExpGBB/gh-devlake/internal/docker"
"github.com/DevExpGBB/gh-devlake/internal/download"
"github.com/DevExpGBB/gh-devlake/internal/gitclone"
Expand Down Expand Up @@ -196,17 +195,10 @@ func runDeployLocal(cmd *cobra.Command, args []string) error {
}
cfgURL = backendURL

fmt.Println("\n🔄 Triggering database migration...")
migClient := devlake.NewClient(backendURL)
if err := migClient.TriggerMigration(); err != nil {
fmt.Printf(" ⚠️ Migration may need manual trigger: %v\n", err)
} else {
fmt.Println(" ✅ Migration triggered")
fmt.Println("\n⏳ Waiting for migration to complete...")
if err := waitForMigration(backendURL, 60, 5*time.Second); err != nil {
fmt.Printf(" ⚠️ %v\n", err)
fmt.Println(" Migration may still be running — proceeding anyway")
}
if err := triggerAndWaitForMigration(backendURL); err != nil {
fmt.Printf(" ⚠️ %v\n", err)
fmt.Printf(" Trigger migration manually if needed: GET %s/proceed-db-migration\n", backendURL)
fmt.Println(" Migration may still be running — proceeding anyway")
}

if !deployLocalQuiet {
Expand Down
50 changes: 48 additions & 2 deletions cmd/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,19 +221,65 @@ func waitForReadyAny(baseURLs []string, maxAttempts int, interval time.Duration)
// During migration the API returns 428 (Precondition Required).
func waitForMigration(baseURL string, maxAttempts int, interval time.Duration) error {
httpClient := &http.Client{Timeout: 5 * time.Second}
lastStatus := 0
for attempt := 1; attempt <= maxAttempts; attempt++ {
resp, err := httpClient.Get(baseURL + "/ping")
if err == nil {
lastStatus = resp.StatusCode
resp.Body.Close()
if resp.StatusCode == http.StatusOK {
fmt.Println(" ✅ Migration complete!")
return nil
}
}
fmt.Printf(" Migrating... (%d/%d)\n", attempt, maxAttempts)
statusSuffix := ""
if lastStatus != 0 {
statusSuffix = fmt.Sprintf(", status=%d", lastStatus)
}
fmt.Printf(" Migrating... (%d/%d%s)\n", attempt, maxAttempts, statusSuffix)
Comment on lines +224 to +239
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

waitForMigration keeps lastStatus from the last successful HTTP response; if a later attempt errors (timeout/DNS/etc.), the progress output can still show a stale status=... even though the current attempt didn’t get a response. Consider resetting lastStatus to 0 on request errors (or tracking/printing the last error separately) so the status suffix always reflects an actual response.

Copilot uses AI. Check for mistakes.
time.Sleep(interval)
}
return fmt.Errorf("migration did not complete after %d attempts", maxAttempts)
statusSuffix := ""
if lastStatus != 0 {
statusSuffix = fmt.Sprintf(" (last status %d)", lastStatus)
}
return fmt.Errorf("migration did not complete after %d attempts%s", maxAttempts, statusSuffix)
}

func triggerAndWaitForMigration(baseURL string) error {
return triggerAndWaitForMigrationWithClient(devlake.NewClient(baseURL), 3, 10*time.Second, 60, 5*time.Second)
}

func triggerAndWaitForMigrationWithClient(devlakeClient *devlake.Client, triggerAttempts int, triggerInterval time.Duration, waitAttempts int, waitInterval time.Duration) error {
fmt.Println("\n🔄 Triggering database migration...")

Comment on lines +249 to +255
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

triggerAndWaitForMigrationWithClient takes both baseURL and a devlakeClient that already carries a BaseURL. If these ever diverge, migration trigger and migration wait will hit different instances. Consider deriving the wait URL from devlakeClient.BaseURL (or validating they match) to avoid accidental mismatches.

This issue also appears on line 256 of the same file.

Copilot uses AI. Check for mistakes.
var lastErr error
for attempt := 1; attempt <= triggerAttempts; attempt++ {
err := devlakeClient.TriggerMigration()
if err == nil {
lastErr = nil
fmt.Println(" ✅ Migration triggered")
break
}
lastErr = err
fmt.Printf(" ⚠️ Trigger attempt %d/%d failed: %v\n", attempt, triggerAttempts, err)
if attempt < triggerAttempts {
fmt.Println(" DevLake may still be starting or migration may already be running — retrying...")
time.Sleep(triggerInterval)
}
}

fmt.Println("\n⏳ Waiting for migration to complete...")
if lastErr != nil {
fmt.Println(" Continuing to monitor migration status anyway...")
}
Comment on lines +256 to +275
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

In triggerAndWaitForMigrationWithClient, if an early trigger attempt fails and a later attempt succeeds, lastErr is never cleared. This leads to misleading output ("Continuing to monitor… anyway") and can produce an incorrect combined error claiming the trigger failed even when it eventually succeeded. Consider resetting lastErr to nil on success or tracking success with a separate boolean.

Copilot uses AI. Check for mistakes.
if err := waitForMigration(devlakeClient.BaseURL, waitAttempts, waitInterval); err != nil {
if lastErr != nil {
return fmt.Errorf("migration trigger failed earlier (%v) and waiting for migration completion also failed: %w", lastErr, err)
}
return err
Comment on lines +276 to +280
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

When both the trigger and wait phases fail, the returned error wraps only the wait error (%w) and interpolates the trigger error with %v, which loses the trigger error in the error chain. If callers/tests need to inspect both failures, consider returning a combined error (e.g., joining the two) while still preserving wrapping for each underlying error.

Copilot uses AI. Check for mistakes.
}
return nil
}

// ── Scope orchestration ─────────────────────────────────────────
Expand Down
131 changes: 131 additions & 0 deletions cmd/helpers_migration_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
package cmd

import (
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"

"github.com/DevExpGBB/gh-devlake/internal/devlake"
)

func TestTriggerAndWaitForMigrationWithClient_CompletesAfterTriggerTimeout(t *testing.T) {
triggerCalls := 0
pingCalls := 0

srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/proceed-db-migration":
triggerCalls++
time.Sleep(25 * time.Millisecond)
w.WriteHeader(http.StatusOK)
case "/ping":
pingCalls++
if pingCalls == 1 {
w.WriteHeader(http.StatusPreconditionRequired)
return
}
w.WriteHeader(http.StatusOK)
default:
http.NotFound(w, r)
}
}))
defer srv.Close()

client := &devlake.Client{
BaseURL: srv.URL,
HTTPClient: &http.Client{
Timeout: 5 * time.Millisecond,
},
}

err := triggerAndWaitForMigrationWithClient(client, 1, time.Millisecond, 3, time.Millisecond)
if err != nil {
Comment on lines +21 to +44
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

The migration helper tests use extremely small timeouts/intervals (1–5ms). These can be flaky on slower/loaded CI environments due to scheduler and timer granularity. Consider increasing the durations (while keeping the tests fast) to reduce nondeterminism, e.g., using tens of milliseconds and slightly larger retry intervals.

Copilot uses AI. Check for mistakes.
t.Fatalf("unexpected error: %v", err)
}
if triggerCalls != 1 {
t.Fatalf("trigger calls = %d, want 1", triggerCalls)
}
if pingCalls != 2 {
t.Fatalf("ping calls = %d, want 2", pingCalls)
}
}

func TestTriggerAndWaitForMigrationWithClient_RetriesBeforeWaiting(t *testing.T) {
triggerCalls := 0
pingCalls := 0

srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/proceed-db-migration":
triggerCalls++
if triggerCalls == 1 {
w.WriteHeader(http.StatusServiceUnavailable)
return
}
w.WriteHeader(http.StatusOK)
case "/ping":
pingCalls++
w.WriteHeader(http.StatusOK)
default:
http.NotFound(w, r)
}
}))
defer srv.Close()

client := devlake.NewClient(srv.URL)

err := triggerAndWaitForMigrationWithClient(client, 2, time.Millisecond, 2, time.Millisecond)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if triggerCalls != 2 {
t.Fatalf("trigger calls = %d, want 2", triggerCalls)
}
if pingCalls != 1 {
t.Fatalf("ping calls = %d, want 1", pingCalls)
}
}
Comment on lines +55 to +89
Copy link

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

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

The new migration helper has an edge case where an early trigger failure followed by a later success should not be treated as a trigger failure (and should not produce the combined "trigger failed earlier" error). Adding a focused test for "first trigger fails, later succeeds, then wait fails" would lock this behavior in and prevent regressions.

Copilot uses AI. Check for mistakes.

func TestTriggerAndWaitForMigrationWithClient_TriggerEventuallySucceedsBeforeWaitFails(t *testing.T) {
triggerCalls := 0
pingCalls := 0

srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/proceed-db-migration":
triggerCalls++
if triggerCalls == 1 {
w.WriteHeader(http.StatusServiceUnavailable)
return
}
w.WriteHeader(http.StatusOK)
case "/ping":
pingCalls++
w.WriteHeader(http.StatusPreconditionRequired)
default:
http.NotFound(w, r)
}
}))
defer srv.Close()

client := devlake.NewClient(srv.URL)

err := triggerAndWaitForMigrationWithClient(client, 2, 5*time.Millisecond, 2, 5*time.Millisecond)
if err == nil {
t.Fatal("expected error, got nil")
}
if strings.Contains(err.Error(), "migration trigger failed earlier") {
t.Fatalf("unexpected trigger failure in error: %v", err)
}
if !strings.Contains(err.Error(), "migration did not complete after 2 attempts") {
t.Fatalf("expected wait failure in error, got: %v", err)
}
if triggerCalls != 2 {
t.Fatalf("trigger calls = %d, want 2", triggerCalls)
}
if pingCalls != 2 {
t.Fatalf("ping calls = %d, want 2", pingCalls)
}
}
31 changes: 25 additions & 6 deletions internal/devlake/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"io"
"net/http"
"net/url"
"strings"
"time"
)

Expand Down Expand Up @@ -289,6 +290,29 @@ func doGet[T any](c *Client, path string) (*T, error) {
return &result, nil
}

// doGetNoBody is a helper for GET requests that only need a successful 2xx status.
func doGetNoBody(c *Client, path string) error {
resp, err := c.HTTPClient.Get(c.BaseURL + path)
if err != nil {
return fmt.Errorf("GET %s: %w", path, err)
}
defer resp.Body.Close()

body, err := io.ReadAll(io.LimitReader(resp.Body, 512))
if err != nil {
return fmt.Errorf("GET %s: reading response: %w", path, err)
}
if resp.StatusCode >= http.StatusOK && resp.StatusCode < http.StatusMultipleChoices {
return nil
}
Comment on lines +301 to +307
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

doGetNoBody reads only the first 512 bytes and then returns (including on 2xx). If the server returns a body larger than 512 bytes, closing without draining the remainder prevents HTTP connection reuse and can degrade performance/reliability for subsequent requests. Consider reading only when non-2xx for the error message and discarding/draining the rest of the body (or fully discarding on 2xx) before closing.

Suggested change
body, err := io.ReadAll(io.LimitReader(resp.Body, 512))
if err != nil {
return fmt.Errorf("GET %s: reading response: %w", path, err)
}
if resp.StatusCode >= http.StatusOK && resp.StatusCode < http.StatusMultipleChoices {
return nil
}
// For successful responses, fully drain the body so the HTTP connection
// can be reused, but ignore the content.
if resp.StatusCode >= http.StatusOK && resp.StatusCode < http.StatusMultipleChoices {
_, _ = io.Copy(io.Discard, resp.Body)
return nil
}
// For non-2xx responses, read a small snippet for the error message,
// then discard the remainder to allow connection reuse.
body, err := io.ReadAll(io.LimitReader(resp.Body, 512))
if err != nil {
return fmt.Errorf("GET %s: reading response: %w", path, err)
}
_, _ = io.Copy(io.Discard, resp.Body)

Copilot uses AI. Check for mistakes.

bodyText := strings.TrimSpace(string(body))
if bodyText != "" {
return fmt.Errorf("GET %s: DevLake returned %s: %s", path, resp.Status, bodyText)
}
return fmt.Errorf("GET %s: DevLake returned %s", path, resp.Status)
}

// doPut is a generic helper for PUT requests that return JSON.
func doPut[T any](c *Client, path string, payload any) (*T, error) {
jsonBody, err := json.Marshal(payload)
Expand Down Expand Up @@ -505,12 +529,7 @@ func (c *Client) SearchRemoteScopes(plugin string, connID int, search string, pa

// TriggerMigration triggers the DevLake database migration endpoint.
func (c *Client) TriggerMigration() error {
resp, err := c.HTTPClient.Get(c.BaseURL + "/proceed-db-migration")
if err != nil {
return err
}
resp.Body.Close()
return nil
return doGetNoBody(c, "/proceed-db-migration")
}

// PipelineListResponse is the response from GET /pipelines.
Expand Down
61 changes: 61 additions & 0 deletions internal/devlake/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,67 @@ func TestHealth(t *testing.T) {
}
}

func TestTriggerMigration(t *testing.T) {
tests := []struct {
name string
statusCode int
body string
wantErr bool
wantErrText string
}{
{
name: "success",
statusCode: http.StatusOK,
},
{
name: "no content",
statusCode: http.StatusNoContent,
},
{
name: "server error with body",
statusCode: http.StatusServiceUnavailable,
body: "warming up",
wantErr: true,
wantErrText: "GET /proceed-db-migration: DevLake returned 503 Service Unavailable: warming up",
},
{
name: "server error without body",
statusCode: http.StatusBadGateway,
wantErr: true,
wantErrText: "GET /proceed-db-migration: DevLake returned 502 Bad Gateway",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/proceed-db-migration" {
t.Errorf("path = %s, want /proceed-db-migration", r.URL.Path)
}
w.WriteHeader(tt.statusCode)
_, _ = w.Write([]byte(tt.body))
}))
defer srv.Close()

client := NewClient(srv.URL)
err := client.TriggerMigration()

if tt.wantErr {
if err == nil {
t.Fatal("expected error, got nil")
}
if err.Error() != tt.wantErrText {
t.Fatalf("error = %q, want %q", err.Error(), tt.wantErrText)
}
return
}
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
})
Comment on lines +912 to +969
Copy link

Copilot AI Mar 30, 2026

Choose a reason for hiding this comment

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

TestTriggerMigration currently only checks error vs. no error by status code. Since TriggerMigration was changed to include response details when available, it would be valuable to add a failing case that returns a body and assert the error string includes the status code and body content (and that body is omitted when empty).

Copilot uses AI. Check for mistakes.
}
}

// TestTestSavedConnection tests the TestSavedConnection method.
func TestTestSavedConnection(t *testing.T) {
tests := []struct {
Expand Down
Loading