-
Notifications
You must be signed in to change notification settings - Fork 5
Harden deploy migration flow during local and Azure deploys #148
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.
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
base: main
Are you sure you want to change the base?
Changes from 7 commits
644a8e8
0735ac2
f289fcd
bbbc087
63a85e0
edb7da9
4b4825f
4cbb5e2
1efb3a3
359b749
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -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) | ||||||||
| 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) | ||||||||
|
||||||||
| 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
AI
Mar 25, 2026
There was a problem hiding this comment.
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
AI
Mar 30, 2026
There was a problem hiding this comment.
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
AI
Mar 30, 2026
There was a problem hiding this comment.
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.
| 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
|
||
| 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
|
||
| 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 { | ||
|
|
@@ -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) | ||
| } | ||
|
||
| return nil | ||
| } | ||
|
|
||
| // PipelineListResponse is the response from GET /pipelines. | ||
| type PipelineListResponse struct { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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
|
||
| } | ||
| } | ||
|
|
||
| // TestTestSavedConnection tests the TestSavedConnection method. | ||
| func TestTestSavedConnection(t *testing.T) { | ||
| tests := []struct { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
waitForMigrationkeepslastStatusfrom the last successful HTTP response; if a later attempt errors (timeout/DNS/etc.), the progress output can still show a stalestatus=...even though the current attempt didn’t get a response. Consider resettinglastStatusto 0 on request errors (or tracking/printing the last error separately) so the status suffix always reflects an actual response.