diff --git a/pkg/registry/apis/provisioning/jobs/job_resource_result.go b/pkg/registry/apis/provisioning/jobs/job_resource_result.go index 5122f4c50a9..d0f4ea1e25c 100644 --- a/pkg/registry/apis/provisioning/jobs/job_resource_result.go +++ b/pkg/registry/apis/provisioning/jobs/job_resource_result.go @@ -1,9 +1,19 @@ package jobs import ( + "errors" + "github.com/grafana/grafana/apps/provisioning/pkg/repository" ) +// WarningError is an interface for errors that should be classified as warnings +// in job progress tracking rather than errors. Any error type can implement this +// interface to be automatically classified as a warning. +type WarningError interface { + error + IsWarning() bool +} + // JobResourceResult represents the result of a resource operation in a job. type JobResourceResult struct { name string @@ -35,7 +45,8 @@ func NewSkippedJobResourceResult(name, group, kind, path string, err error) JobR } func isWarningError(err error) bool { - return false + var warningErr WarningError + return errors.As(err, &warningErr) && warningErr.IsWarning() } // newJobResourceResult creates a new JobResourceResult. diff --git a/pkg/registry/apis/provisioning/jobs/progress_test.go b/pkg/registry/apis/provisioning/jobs/progress_test.go index adca2122eec..f7761e4b4cd 100644 --- a/pkg/registry/apis/provisioning/jobs/progress_test.go +++ b/pkg/registry/apis/provisioning/jobs/progress_test.go @@ -7,6 +7,7 @@ import ( provisioning "github.com/grafana/grafana/apps/provisioning/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/apps/provisioning/pkg/repository" + "github.com/grafana/grafana/pkg/registry/apis/provisioning/resources" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -252,3 +253,69 @@ func TestJobProgressRecorderWarningOnlyNoErrors(t *testing.T) { require.NotNil(t, finalStatus.Warnings) assert.Len(t, finalStatus.Warnings, 1) } + +func TestJobProgressRecorderWarningClassification(t *testing.T) { + ctx := context.Background() + + // Create a progress recorder + mockProgressFn := func(ctx context.Context, status provisioning.JobStatus) error { + return nil + } + recorder := newJobProgressRecorder(mockProgressFn).(*jobProgressRecorder) + + // Test that ParseError (which implements WarningError) is automatically classified as a warning + parseErr := resources.NewParseError("unable to read file as a resource") + result := NewJobResourceResult( + "test-resource", + "test.grafana.app", + "Dashboard", + "dashboards/test.json", + repository.FileActionCreated, + parseErr, + ) + + // Verify that ParseError was classified as a warning, not an error + assert.Nil(t, result.Error(), "ParseError should be classified as warning, not error") + assert.NotNil(t, result.Warning(), "ParseError should be classified as warning") + assert.Equal(t, parseErr, result.Warning()) + + // Record the result + recorder.Record(ctx, result) + + // Verify it's stored as a warning in summaries + recorder.mu.RLock() + require.Len(t, recorder.summaries, 1) + dashboardSummary := recorder.summaries["test.grafana.app:Dashboard"] + require.NotNil(t, dashboardSummary) + assert.Equal(t, int64(1), dashboardSummary.Warning) + assert.Len(t, dashboardSummary.Warnings, 1) + assert.Contains(t, dashboardSummary.Warnings[0], "unable to read file as a resource") + assert.Equal(t, int64(0), dashboardSummary.Error) + assert.Len(t, dashboardSummary.Errors, 0) + recorder.mu.RUnlock() + + // Complete the job and verify final status + finalStatus := recorder.Complete(ctx, nil) + assert.Equal(t, provisioning.JobStateWarning, finalStatus.State) + assert.Equal(t, "completed with warnings", finalStatus.Message) + assert.Empty(t, finalStatus.Errors) + require.NotNil(t, finalStatus.Warnings) + assert.Len(t, finalStatus.Warnings, 1) + assert.Contains(t, finalStatus.Warnings[0], "unable to read file as a resource") + + // Test that regular errors are still classified as errors + regularErr := errors.New("regular error") + result2 := NewJobResourceResult( + "test-resource-2", + "test.grafana.app", + "Dashboard", + "dashboards/test2.json", + repository.FileActionUpdated, + regularErr, + ) + + // Verify that regular error was classified as an error, not a warning + assert.NotNil(t, result2.Error(), "Regular error should be classified as error") + assert.Nil(t, result2.Warning(), "Regular error should not be classified as warning") + assert.Equal(t, regularErr, result2.Error()) +} diff --git a/pkg/registry/apis/provisioning/resources/dualwriter.go b/pkg/registry/apis/provisioning/resources/dualwriter.go index e8a7ee83dd0..19f32910553 100644 --- a/pkg/registry/apis/provisioning/resources/dualwriter.go +++ b/pkg/registry/apis/provisioning/resources/dualwriter.go @@ -225,7 +225,8 @@ func (r *DualReadWriter) createOrUpdate(ctx context.Context, create bool, opts D parsed, err := r.parser.Parse(ctx, info) if err != nil { - return nil, err + // Convert parsing errors to API errors for the HTTP response + return nil, apierrors.NewBadRequest(fmt.Sprintf("Parse file failed: %v", err)) } // Make sure the value is valid diff --git a/pkg/registry/apis/provisioning/resources/parser.go b/pkg/registry/apis/provisioning/resources/parser.go index a684e4cffc5..43e43c65fe1 100644 --- a/pkg/registry/apis/provisioning/resources/parser.go +++ b/pkg/registry/apis/provisioning/resources/parser.go @@ -72,6 +72,27 @@ func (f *parserFactory) GetParser(ctx context.Context, repo repository.Reader) ( }, nil } +// ParseError represents an error that occurred during resource parsing. +// It implements the WarningError interface (defined in the jobs package) to be +// classified as a warning in job progress. +type ParseError struct { + Message string +} + +func (e *ParseError) Error() string { + return e.Message +} + +// IsWarning implements the WarningError interface to mark this as a warning. +func (e *ParseError) IsWarning() bool { + return true +} + +// NewParseError creates a new ParseError with the given message. +func NewParseError(message string) *ParseError { + return &ParseError{Message: message} +} + type parser struct { // The target repository repo provisioning.ResourceRepositoryInfo @@ -144,7 +165,7 @@ func (r *parser) Parse(ctx context.Context, info *repository.FileInfo) (parsed * logger.Debug("failed to find GVK of the input data, trying fallback loader", "error", err) parsed.Obj, gvk, parsed.Classic, err = ReadClassicResource(ctx, info) if err != nil || gvk == nil { - return nil, apierrors.NewBadRequest("unable to read file as a resource") + return nil, NewParseError("unable to read file as a resource") } } @@ -172,7 +193,7 @@ func (r *parser) Parse(ctx context.Context, info *repository.FileInfo) (parsed * // Validate the namespace if obj.GetNamespace() != "" && obj.GetNamespace() != r.repo.Namespace { - return nil, apierrors.NewBadRequest("the file namespace does not match target namespace") + return nil, NewParseError("the file namespace does not match target namespace") } obj.SetNamespace(r.repo.Namespace)