From 811638f650f78c2055b5d8d678c16a7a7e621ace Mon Sep 17 00:00:00 2001 From: Jon Langevin Date: Wed, 22 Apr 2026 02:39:22 -0400 Subject: [PATCH] =?UTF-8?q?fix:=20DO=20plugin=20polish=20=E2=80=94=20log?= =?UTF-8?q?=20deploy=20ID,=20forward-compat=20phase,=20test=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - AppPlatformDriver.Update: log the triggered deployment ID after CreateDeployment succeeds for easier correlation with DO dashboard - appHealthResult: split known-failed phases (ERROR/CANCELED/SUPERSEDED) from the default case; unknown/future phases now report "unknown phase: " instead of "deployment failed" to avoid mislabeling godo additions in future releases - TestAppPlatformDriver_Update_Error: assert CreateDeployment is NOT called when the spec Update fails (short-circuit invariant) - WrapGodoError: expand doc comment to explain the two passthrough branches (non-godo error, nil Response) so intent is clear - Add unknown-phase HealthCheck tests for both AppPlatformDriver and APIGatewayDriver Co-Authored-By: Claude Sonnet 4.6 --- internal/drivers/api_gateway_test.go | 13 +++++++++++++ internal/drivers/app_platform.go | 16 +++++++++++++--- internal/drivers/app_platform_test.go | 16 ++++++++++++++++ internal/drivers/errors.go | 9 ++++++++- 4 files changed, 50 insertions(+), 4 deletions(-) diff --git a/internal/drivers/api_gateway_test.go b/internal/drivers/api_gateway_test.go index ebede1e..be735bb 100644 --- a/internal/drivers/api_gateway_test.go +++ b/internal/drivers/api_gateway_test.go @@ -295,3 +295,16 @@ func TestAPIGatewayDriver_HealthCheck_NoDeployment(t *testing.T) { t.Errorf("message should contain 'no deployment', got: %q", result.Message) } } + +func TestAPIGatewayDriver_HealthCheck_InProgress_UnknownPhase(t *testing.T) { + d := drivers.NewAPIGatewayDriverWithClient(&mockAPIGatewayClient{ + app: gwAppWithPhases(nil, gwPhasePtr(godo.DeploymentPhase_Unknown), nil), + }, "nyc3") + result, _ := d.HealthCheck(context.Background(), interfaces.ResourceRef{Name: "phased-gw", ProviderID: "app-gw-999"}) + if result.Healthy { + t.Error("expected Healthy=false for unknown phase") + } + if !strings.Contains(result.Message, "unknown phase") { + t.Errorf("message should contain 'unknown phase', got: %q", result.Message) + } +} diff --git a/internal/drivers/app_platform.go b/internal/drivers/app_platform.go index 89f3ff9..2cc8d93 100644 --- a/internal/drivers/app_platform.go +++ b/internal/drivers/app_platform.go @@ -142,9 +142,11 @@ func (d *AppPlatformDriver) Update(ctx context.Context, ref interfaces.ResourceR return nil, fmt.Errorf("app platform update %q: %w", ref.Name, WrapGodoError(err)) } // Trigger a new deployment — Update only changes the spec; DO does not auto-deploy. - if _, _, err := d.client.CreateDeployment(ctx, ref.ProviderID, &godo.DeploymentCreateRequest{ForceBuild: true}); err != nil { + dep, _, err := d.client.CreateDeployment(ctx, ref.ProviderID, &godo.DeploymentCreateRequest{ForceBuild: true}) + if err != nil { return nil, fmt.Errorf("app platform create deployment %q: %w", ref.Name, WrapGodoError(err)) } + fmt.Printf(" app platform deploy %q: triggered deployment %s\n", spec.Name, dep.ID) return appOutput(app), nil } @@ -203,12 +205,20 @@ func appHealthResult(app *godo.App) *interfaces.HealthResult { Healthy: false, Message: fmt.Sprintf("deployment in progress: %s", dep.Phase), } - default: - // ERROR, CANCELED, SUPERSEDED, etc. + case godo.DeploymentPhase_Error, + godo.DeploymentPhase_Canceled, + godo.DeploymentPhase_Superseded: return &interfaces.HealthResult{ Healthy: false, Message: fmt.Sprintf("deployment failed: %s", dep.Phase), } + default: + // Forward-compat: a future godo release may add new phases. + // Report "unknown" rather than "failed" to avoid mislabeling. + return &interfaces.HealthResult{ + Healthy: false, + Message: fmt.Sprintf("unknown phase: %s", dep.Phase), + } } } diff --git a/internal/drivers/app_platform_test.go b/internal/drivers/app_platform_test.go index 9ff0917..91c0616 100644 --- a/internal/drivers/app_platform_test.go +++ b/internal/drivers/app_platform_test.go @@ -185,6 +185,9 @@ func TestAppPlatformDriver_Update_Error(t *testing.T) { if err == nil { t.Fatal("expected error, got nil") } + if mock.createDeploymentCalled { + t.Error("CreateDeployment should not be called on Update failure") + } } func TestAppPlatformDriver_Update_TriggersCreateDeployment(t *testing.T) { @@ -514,6 +517,19 @@ func TestAppPlatformDriver_HealthCheck_NoDeployment(t *testing.T) { } } +func TestAppPlatformDriver_HealthCheck_InProgress_UnknownPhase(t *testing.T) { + d := drivers.NewAppPlatformDriverWithClient(&mockAppClient{ + app: appWithPhases(nil, phasePtr(godo.DeploymentPhase_Unknown), nil), + }, "nyc3") + result, _ := d.HealthCheck(context.Background(), interfaces.ResourceRef{Name: "phased-app", ProviderID: "app-999"}) + if result.Healthy { + t.Error("expected Healthy=false for unknown phase") + } + if !strings.Contains(result.Message, "unknown phase") { + t.Errorf("message should contain 'unknown phase', got: %q", result.Message) + } +} + // ── ParseImageRef unit tests ────────────────────────────────────────────────── func TestParseImageRef_DOCR(t *testing.T) { diff --git a/internal/drivers/errors.go b/internal/drivers/errors.go index bbb01ca..35bbfeb 100644 --- a/internal/drivers/errors.go +++ b/internal/drivers/errors.go @@ -13,13 +13,20 @@ import ( // sentinel so callers can use errors.Is for classification while still having // access to the original DO API message via err.Error(). // -// If err is nil or not a *godo.ErrorResponse, it is returned unchanged. +// Two passthrough branches return err unchanged: +// - err is nil → return nil (no-op; safe to call unconditionally) +// - err is not a *godo.ErrorResponse, or its Response field is nil → return err +// (e.g. network errors, context cancellation, or SDK bugs; not DO API errors) +// +// HTTP codes with no sentinel mapping (e.g. 301, 400 handled elsewhere) also +// pass through unchanged via sentinelForStatus returning nil. func WrapGodoError(err error) error { if err == nil { return nil } gErr, ok := err.(*godo.ErrorResponse) if !ok || gErr.Response == nil { + // Not a DO API error — pass through so the original error is preserved. return err } sentinel := sentinelForStatus(gErr.Response.StatusCode)