diff --git a/apps/dashboard/pkg/migration/conversion/conversion_test.go b/apps/dashboard/pkg/migration/conversion/conversion_test.go index d5615b92c20..bc915fd906c 100644 --- a/apps/dashboard/pkg/migration/conversion/conversion_test.go +++ b/apps/dashboard/pkg/migration/conversion/conversion_test.go @@ -249,7 +249,8 @@ func TestConversionMetrics(t *testing.T) { name string source metav1.Object target metav1.Object - expectSuccess bool + expectAPISuccess bool + expectMetricsSuccess bool expectedSourceAPI string expectedTargetAPI string expectedSourceSchema string @@ -266,7 +267,8 @@ func TestConversionMetrics(t *testing.T) { }}, }, target: &dashv1.Dashboard{}, - expectSuccess: true, + expectAPISuccess: true, + expectMetricsSuccess: true, expectedSourceAPI: dashv0.APIVERSION, expectedTargetAPI: dashv1.APIVERSION, expectedSourceSchema: "14", @@ -282,7 +284,8 @@ func TestConversionMetrics(t *testing.T) { }}, }, target: &dashv0.Dashboard{}, - expectSuccess: true, + expectAPISuccess: true, + expectMetricsSuccess: true, expectedSourceAPI: dashv1.APIVERSION, expectedTargetAPI: dashv0.APIVERSION, expectedSourceSchema: "42", @@ -295,7 +298,8 @@ func TestConversionMetrics(t *testing.T) { Spec: dashv2alpha1.DashboardSpec{Title: "test dashboard"}, }, target: &dashv2beta1.Dashboard{}, - expectSuccess: true, + expectAPISuccess: true, + expectMetricsSuccess: true, expectedSourceAPI: dashv2alpha1.APIVERSION, expectedTargetAPI: dashv2beta1.APIVERSION, expectedSourceSchema: "v2alpha1", @@ -311,7 +315,8 @@ func TestConversionMetrics(t *testing.T) { }}, }, target: &dashv1.Dashboard{}, - expectSuccess: true, // Conversion succeeds but status indicates failure + expectAPISuccess: true, // Conversion succeeds but status indicates failure + expectMetricsSuccess: false, expectedSourceAPI: dashv0.APIVERSION, expectedTargetAPI: dashv1.APIVERSION, expectedSourceSchema: "5", @@ -329,7 +334,7 @@ func TestConversionMetrics(t *testing.T) { err := scheme.Convert(tt.source, tt.target, nil) // Check error expectation - if tt.expectSuccess { + if tt.expectAPISuccess { require.NoError(t, err, "expected successful conversion") } else { require.Error(t, err, "expected conversion to fail") @@ -352,7 +357,7 @@ func TestConversionMetrics(t *testing.T) { } } - if tt.expectSuccess { + if tt.expectAPISuccess && tt.expectMetricsSuccess { require.Equal(t, float64(1), successTotal, "success metric should be incremented") require.Equal(t, float64(0), failureTotal, "failure metric should not be incremented") } else { @@ -372,14 +377,15 @@ func TestConversionMetricsWrapper(t *testing.T) { migration.RegisterMetrics(registry) tests := []struct { - name string - source interface{} - target interface{} - conversionFunction func(a, b interface{}, scope conversion.Scope) error - expectSuccess bool - expectedSourceUID string - expectedSourceAPI string - expectedTargetAPI string + name string + source interface{} + target interface{} + conversionFunction func(a, b interface{}, scope conversion.Scope) error + expectAPISuccess bool + expectMetricsSuccess bool + expectedSourceUID string + expectedSourceAPI string + expectedTargetAPI string }{ { name: "successful conversion wrapper", @@ -395,10 +401,11 @@ func TestConversionMetricsWrapper(t *testing.T) { // Simulate successful conversion return nil }, - expectSuccess: true, - expectedSourceUID: "test-wrapper-1", - expectedSourceAPI: dashv0.APIVERSION, - expectedTargetAPI: dashv1.APIVERSION, + expectAPISuccess: true, + expectMetricsSuccess: true, + expectedSourceUID: "test-wrapper-1", + expectedSourceAPI: dashv0.APIVERSION, + expectedTargetAPI: dashv1.APIVERSION, }, { name: "failed conversion wrapper", @@ -414,10 +421,11 @@ func TestConversionMetricsWrapper(t *testing.T) { // Simulate conversion failure return fmt.Errorf("conversion failed") }, - expectSuccess: false, - expectedSourceUID: "test-wrapper-2", - expectedSourceAPI: dashv1.APIVERSION, - expectedTargetAPI: dashv0.APIVERSION, + expectAPISuccess: true, + expectMetricsSuccess: false, + expectedSourceUID: "test-wrapper-2", + expectedSourceAPI: dashv1.APIVERSION, + expectedTargetAPI: dashv0.APIVERSION, }, } @@ -434,7 +442,7 @@ func TestConversionMetricsWrapper(t *testing.T) { err := wrappedFunc(tt.source, tt.target, nil) // Check error expectation - if tt.expectSuccess { + if tt.expectAPISuccess { require.NoError(t, err, "expected successful conversion") } else { require.Error(t, err, "expected conversion to fail") @@ -457,7 +465,7 @@ func TestConversionMetricsWrapper(t *testing.T) { } } - if tt.expectSuccess { + if tt.expectAPISuccess && tt.expectMetricsSuccess { require.Equal(t, float64(1), successTotal, "success metric should be incremented") require.Equal(t, float64(0), failureTotal, "failure metric should not be incremented") } else { @@ -704,7 +712,7 @@ func TestConversionLogLevels(t *testing.T) { target2 := &dashv0.Dashboard{} err = failureWrapper(source2, target2, nil) - require.Error(t, err, "failed conversion should error") + require.NoError(t, err, "conversion wrapper should not error after recording logs") // The logging code paths are executed in both cases above // Success case logs at Debug level with fields: diff --git a/apps/dashboard/pkg/migration/conversion/metrics.go b/apps/dashboard/pkg/migration/conversion/metrics.go index ceda3e4648e..5bb7c895d81 100644 --- a/apps/dashboard/pkg/migration/conversion/metrics.go +++ b/apps/dashboard/pkg/migration/conversion/metrics.go @@ -209,6 +209,6 @@ func withConversionMetrics(sourceVersionAPI, targetVersionAPI string, conversion logger.Debug("Dashboard conversion succeeded", successLogFields...) } - return err + return nil } } diff --git a/apps/dashboard/pkg/migration/conversion/v0.go b/apps/dashboard/pkg/migration/conversion/v0.go index d70db2e060f..a1ccc0e5a6c 100644 --- a/apps/dashboard/pkg/migration/conversion/v0.go +++ b/apps/dashboard/pkg/migration/conversion/v0.go @@ -36,7 +36,7 @@ func Convert_V0_to_V1(in *dashv0.Dashboard, out *dashv1.Dashboard, scope convers if err != nil { out.Status.Conversion.Failed = true out.Status.Conversion.Error = ptr.To(err.Error()) - return nil + return schemaversion.NewMigrationError(err.Error(), schemaversion.GetSchemaVersion(in.Spec.Object), schemaversion.LATEST_VERSION, "Convert_V0_to_V1") } // a background service identity is used here because the user who is reading the specific dashboard @@ -47,7 +47,7 @@ func Convert_V0_to_V1(in *dashv0.Dashboard, out *dashv1.Dashboard, scope convers if err := migration.Migrate(ctx, out.Spec.Object, schemaversion.LATEST_VERSION); err != nil { out.Status.Conversion.Failed = true out.Status.Conversion.Error = ptr.To(err.Error()) - return nil + return schemaversion.NewMigrationError(err.Error(), schemaversion.GetSchemaVersion(in.Spec.Object), schemaversion.LATEST_VERSION, "Convert_V0_to_V1") } return nil