Provisioning: return field paths in test error messages (#103850)

* Provisioning: Do not block connect step on error

* Display field errors

* Cleanup

* return field errors

* fix test

* convert errros to an array

* Fix history display

* Add getFormErrors

* metav1 issues

* lint

* Proper field names

* Fix notification

* Remove unused component

---------

Co-authored-by: Clarity-89 <homes89@ukr.net>

pkg/registry/apis/provisioning/controller/repository.go

@@ -271,18 +271,22 @@ func (rc *RepositoryController) runHealthCheck(ctx context.Context, repo reposit
 	if err != nil {
 		res = &provisioning.TestResults{
 			Success: false,
-			Errors: []string{
-				"error running test repository",
-				err.Error(),
-			},
+			Errors: []provisioning.ErrorDetails{{
+				Detail: fmt.Sprintf("error running test repository: %s", err.Error()),
+			}},
 		}
 	}
 
 	healthStatus := provisioning.HealthStatus{
 		Healthy: res.Success,
 		Checked: time.Now().UnixMilli(),
-		Message: res.Errors,
 	}
+	for _, err := range res.Errors {
+		if err.Detail != "" {
+			healthStatus.Message = append(healthStatus.Message, err.Detail)
+		}
+	}
+
 	logger.Info("health check completed", "status", healthStatus)
 
 	return healthStatus

pkg/registry/apis/provisioning/register.go

@@ -1114,7 +1114,7 @@ func (b *APIBuilder) AsRepository(ctx context.Context, r *provisioning.Repositor
 			return gogit.Clone(ctx, b.clonedir, r, opts, b.secrets)
 		}
 
-		return repository.NewGitHub(ctx, r, b.ghFactory, b.secrets, webhookURL, cloneFn)
+		return repository.NewGitHub(ctx, r, b.ghFactory, b.secrets, webhookURL, cloneFn), nil
 	default:
 		return nil, fmt.Errorf("unknown repository type (%s)", r.Spec.Type)
 	}

pkg/registry/apis/provisioning/repository/github.go

@@ -12,12 +12,11 @@ import (
 	"strings"
 
 	"github.com/google/go-github/v70/github"
+	"github.com/google/uuid"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
-	"github.com/google/uuid"
-
 	"github.com/grafana/grafana-app-sdk/logging"
 	provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
 	pgh "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository/github"
@@ -57,18 +56,14 @@ func NewGitHub(
 	secrets secrets.Service,
 	webhookURL string,
 	cloneFn CloneFn,
-) (*githubRepository, error) {
-	owner, repo, err := parseOwnerRepo(config.Spec.GitHub.URL)
-	if err != nil {
-		return nil, err
-	}
+) *githubRepository {
+	owner, repo, _ := parseOwnerRepo(config.Spec.GitHub.URL)
 	token := config.Spec.GitHub.Token
 	if token == "" {
 		decrypted, err := secrets.Decrypt(ctx, config.Spec.GitHub.EncryptedToken)
-		if err != nil {
-			return nil, err
+		if err == nil {
+			token = string(decrypted)
 		}
-		token = string(decrypted)
 	}
 	return &githubRepository{
 		config:     config,
@@ -78,7 +73,7 @@ func NewGitHub(
 		owner:      owner,
 		repo:       repo,
 		cloneFn:    cloneFn,
-	}, nil
+	}
 }
 
 func (r *githubRepository) Config() *provisioning.Repository {
@@ -138,59 +133,48 @@ func parseOwnerRepo(giturl string) (owner string, repo string, err error) {
 	return parts[1], parts[2], nil
 }
 
-func fromError(err error, code int) *provisioning.TestResults {
-	statusErr, ok := err.(apierrors.APIStatus)
-	if ok {
-		s := statusErr.Status()
-		return &provisioning.TestResults{
-			Code:    int(s.Code),
-			Success: false,
-			Errors:  []string{s.Message},
-		}
-	}
-	return &provisioning.TestResults{
-		Code:    code,
-		Success: false,
-		Errors:  []string{err.Error()},
-	}
-}
-
 // Test implements provisioning.Repository.
 func (r *githubRepository) Test(ctx context.Context) (*provisioning.TestResults, error) {
 	if err := r.gh.IsAuthenticated(ctx); err != nil {
-		return fromError(err, http.StatusUnauthorized), nil
+		return &provisioning.TestResults{
+			Code:    http.StatusBadRequest,
+			Success: false,
+			Errors: []provisioning.ErrorDetails{{
+				Type:   metav1.CauseTypeFieldValueInvalid,
+				Field:  field.NewPath("spec", "github", "token").String(),
+				Detail: err.Error(),
+			}}}, nil
 	}
 
-	owner, repo, err := parseOwnerRepo(r.config.Spec.GitHub.URL)
+	url := r.config.Spec.GitHub.URL
+	owner, repo, err := parseOwnerRepo(url)
 	if err != nil {
-		return fromError(err, http.StatusBadRequest), nil
+		return fromFieldError(field.Invalid(
+			field.NewPath("spec", "github", "url"), url, err.Error())), nil
 	}
 
 	// FIXME: check token permissions
 	ok, err := r.gh.RepoExists(ctx, owner, repo)
 	if err != nil {
-		return fromError(err, http.StatusBadRequest), nil
+		return fromFieldError(field.Invalid(
+			field.NewPath("spec", "github", "url"), url, err.Error())), nil
 	}
 
 	if !ok {
-		return &provisioning.TestResults{
-			Code:    http.StatusBadRequest,
-			Success: false,
-			Errors:  []string{"repository does not exist"},
-		}, nil
+		return fromFieldError(field.NotFound(
+			field.NewPath("spec", "github", "url"), url)), nil
 	}
 
-	ok, err = r.gh.BranchExists(ctx, r.owner, r.repo, r.config.Spec.GitHub.Branch)
+	branch := r.config.Spec.GitHub.Branch
+	ok, err = r.gh.BranchExists(ctx, r.owner, r.repo, branch)
 	if err != nil {
-		return fromError(err, http.StatusBadRequest), nil
+		return fromFieldError(field.Invalid(
+			field.NewPath("spec", "github", "branch"), branch, err.Error())), nil
 	}
 
 	if !ok {
-		return &provisioning.TestResults{
-			Code:    http.StatusBadRequest,
-			Success: false,
-			Errors:  []string{"branch does not exist"},
-		}, nil
+		return fromFieldError(field.NotFound(
+			field.NewPath("spec", "github", "branch"), branch)), nil
 	}
 
 	return &provisioning.TestResults{

pkg/registry/apis/provisioning/repository/local.go

@@ -146,36 +146,19 @@ func (r *localRepository) Validate() (fields field.ErrorList) {
 // Test implements provisioning.Repository.
 // NOTE: Validate has been called (and passed) before this function should be called
 func (r *localRepository) Test(ctx context.Context) (*provisioning.TestResults, error) {
+	path := field.NewPath("spec", "localhost", "path")
 	if r.config.Spec.Local.Path == "" {
-		return &provisioning.TestResults{
-			Code:    http.StatusBadRequest,
-			Success: false,
-			Errors: []string{
-				"no path is configured",
-			},
-		}, nil
+		return fromFieldError(field.Required(path, "no path is configured")), nil
 	}
 
 	_, err := r.resolver.LocalPath(r.config.Spec.Local.Path)
 	if err != nil {
-		return &provisioning.TestResults{
-			Code:    http.StatusBadRequest,
-			Success: false,
-			Errors: []string{
-				err.Error(),
-			},
-		}, nil
+		return fromFieldError(field.Invalid(path, r.config.Spec.Local.Path, err.Error())), nil
 	}
 
 	_, err = os.Stat(r.path)
 	if errors.Is(err, os.ErrNotExist) {
-		return &provisioning.TestResults{
-			Code:    http.StatusBadRequest,
-			Success: false,
-			Errors: []string{
-				fmt.Sprintf("directory not found: %s", r.config.Spec.Local.Path),
-			},
-		}, nil
+		return fromFieldError(field.NotFound(path, r.config.Spec.Local.Path)), nil
 	}
 
 	return &provisioning.TestResults{

pkg/registry/apis/provisioning/repository/test.go

@@ -6,6 +6,7 @@ import (
 	"net/http"
 	"slices"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 
 	provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
@@ -26,10 +27,14 @@ func TestRepository(ctx context.Context, repo Repository) (*provisioning.TestRes
 		rsp := &provisioning.TestResults{
 			Code:    http.StatusUnprocessableEntity, // Invalid
 			Success: false,
-			Errors:  make([]string, len(errors)),
+			Errors:  make([]provisioning.ErrorDetails, len(errors)),
 		}
-		for i, v := range errors {
-			rsp.Errors[i] = v.Error()
+		for i, err := range errors {
+			rsp.Errors[i] = provisioning.ErrorDetails{
+				Type:   metav1.CauseType(err.Type),
+				Field:  err.Field,
+				Detail: err.Detail,
+			}
 		}
 		return rsp, nil
 	}
@@ -85,3 +90,15 @@ func ValidateRepository(repo Repository) field.ErrorList {
 
 	return list
 }
+
+func fromFieldError(err *field.Error) *provisioning.TestResults {
+	return &provisioning.TestResults{
+		Code:    http.StatusBadRequest,
+		Success: false,
+		Errors: []provisioning.ErrorDetails{{
+			Type:   metav1.CauseType(err.Type),
+			Field:  err.Field,
+			Detail: err.Detail,
+		}},
+	}
+}

pkg/registry/apis/provisioning/repository/test_test.go

@@ -6,11 +6,12 @@ import (
 	"net/http"
 	"testing"
 
-	provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1"
 )
 
 func TestValidateRepository(t *testing.T) {
@@ -192,7 +193,7 @@ func TestTestRepository(t *testing.T) {
 		name          string
 		repository    *MockRepository
 		expectedCode  int
-		expectedErrs  []string
+		expectedErrs  []provisioning.ErrorDetails
 		expectedError error
 	}{
 		{
@@ -208,7 +209,11 @@ func TestTestRepository(t *testing.T) {
 				return m
 			}(),
 			expectedCode: http.StatusUnprocessableEntity,
-			expectedErrs: []string{"spec.title: Required value: a repository title must be given"},
+			expectedErrs: []provisioning.ErrorDetails{{
+				Type:   metav1.CauseTypeFieldValueRequired,
+				Field:  "spec.title",
+				Detail: "a repository title must be given",
+			}},
 		},
 		{
 			name: "test passes",
@@ -257,12 +262,18 @@ func TestTestRepository(t *testing.T) {
 				m.On("Test", mock.Anything).Return(&provisioning.TestResults{
 					Code:    http.StatusBadRequest,
 					Success: false,
-					Errors:  []string{"test failed"},
+					Errors: []provisioning.ErrorDetails{{
+						Type:  metav1.CauseTypeFieldValueInvalid,
+						Field: "spec.property",
+					}},
 				}, nil)
 				return m
 			}(),
 			expectedCode: http.StatusBadRequest,
-			expectedErrs: []string{"test failed"},
+			expectedErrs: []provisioning.ErrorDetails{{
+				Type:  metav1.CauseTypeFieldValueInvalid,
+				Field: "spec.property",
+			}},
 		},
 	}
 

pkg/registry/apis/provisioning/test.go

@@ -99,9 +99,9 @@ func (t *RepositoryTester) UpdateHealthStatus(ctx context.Context, cfg *provisio
 	if res == nil {
 		res = &provisioning.TestResults{
 			Success: false,
-			Errors: []string{
-				"missing health status",
-			},
+			Errors: []provisioning.ErrorDetails{{
+				Detail: "missing health status",
+			}},
 		}
 	}
 
@@ -109,7 +109,11 @@ func (t *RepositoryTester) UpdateHealthStatus(ctx context.Context, cfg *provisio
 	repo.Status.Health = provisioning.HealthStatus{
 		Healthy: res.Success,
 		Checked: time.Now().UnixMilli(),
-		Message: res.Errors,
+	}
+	for _, err := range res.Errors {
+		if err.Detail != "" {
+			repo.Status.Health.Message = append(repo.Status.Health.Message, err.Detail)
+		}
 	}
 
 	_, err := t.client.Repositories(repo.GetNamespace()).