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
49 changes: 47 additions & 2 deletions cmd/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -221,19 +221,64 @@ 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(baseURL, devlake.NewClient(baseURL), 3, 10*time.Second, 60, 5*time.Second)
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.

triggerAndWaitForMigration creates a devlake.NewClient(baseURL), whose default HTTP timeout is 90s. With multiple trigger attempts, a single hung request could significantly extend deploy time (e.g., 3×90s + intervals) before even starting the migration wait loop. Consider using a shorter timeout specifically for TriggerMigration (or passing in an HTTP client/timeout) so retries remain responsive.

Suggested change
return triggerAndWaitForMigrationWithClient(baseURL, devlake.NewClient(baseURL), 3, 10*time.Second, 60, 5*time.Second)
client := devlake.NewClientWithTimeout(baseURL, 10*time.Second)
return triggerAndWaitForMigrationWithClient(baseURL, client, 3, 10*time.Second, 60, 5*time.Second)

Copilot uses AI. Check for mistakes.
}

func triggerAndWaitForMigrationWithClient(baseURL string, 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 {
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(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
88 changes: 88 additions & 0 deletions cmd/helpers_migration_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
package cmd

import (
"net/http"
"net/http/httptest"
"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(srv.URL, 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(srv.URL, 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.
39 changes: 24 additions & 15 deletions internal/devlake/client.go
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
// Package devlake provides an HTTP client for the DevLake REST API.
package devlake

import (
"bytes"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
"time"
)
import (
"bytes"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
"strings"
"time"
)

// Client wraps HTTP calls to the DevLake backend API.
type Client struct {
Expand Down Expand Up @@ -506,12 +507,20 @@ 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
}
if err != nil {
return fmt.Errorf("triggering migration: %w", err)
}
defer resp.Body.Close()
if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
body, _ := io.ReadAll(io.LimitReader(resp.Body, 512))
bodyText := strings.TrimSpace(string(body))
if bodyText != "" {
return fmt.Errorf("DevLake returned status %d: %s", resp.StatusCode, bodyText)
}
return fmt.Errorf("DevLake returned status %d", resp.StatusCode)
}
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.

TriggerMigration error messages drop the request context (endpoint/path), which makes logs harder to interpret compared to other client helpers that include the HTTP method and path. Consider including /proceed-db-migration (and ideally resp.Status) in the returned error so callers can quickly identify the failing call.

See below for a potential fix:

	path := "/proceed-db-migration"
	resp, err := c.HTTPClient.Get(c.BaseURL + path)
	if err != nil {
		return fmt.Errorf("GET %s: triggering migration: %w", path, err)
	}
	defer resp.Body.Close()
	if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
		body, _ := io.ReadAll(io.LimitReader(resp.Body, 512))
		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)

Copilot uses AI. Check for mistakes.
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.

TriggerMigration’s non-2xx error message omits which endpoint/method failed (it only reports the status code and optional body). Including the request context (e.g., GET /proceed-db-migration or full URL) would make failures easier to diagnose, especially when multiple DevLake instances are involved.

Copilot uses AI. Check for mistakes.
return nil
}

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

func TestTriggerMigration(t *testing.T) {
tests := []struct {
name string
statusCode int
wantErr bool
}{
{
name: "success",
statusCode: http.StatusOK,
},
{
name: "no content",
statusCode: http.StatusNoContent,
},
{
name: "server error",
statusCode: http.StatusServiceUnavailable,
wantErr: true,
},
}

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)
}))
defer srv.Close()

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

if tt.wantErr {
if err == nil {
t.Fatal("expected error, got nil")
}
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