diff --git a/pkg/registry/apis/provisioning/files.go b/pkg/registry/apis/provisioning/files.go index 3176d5eec7d..89a00bd2781 100644 --- a/pkg/registry/apis/provisioning/files.go +++ b/pkg/registry/apis/provisioning/files.go @@ -126,12 +126,7 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime. responder.Error(err) return } - obj = resource.AsResourceWrapper() - code = http.StatusOK - if len(resource.Errors) > 0 { - code = http.StatusExpectationFailed - } case http.MethodPost: if isDir { obj, err = dualReadWriter.CreateFolder(ctx, filePath, ref, message) @@ -173,7 +168,6 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime. responder.Error(err) return } - obj = resource.AsResourceWrapper() default: err = apierrors.NewMethodNotSupported(provisioning.RepositoryResourceInfo.GroupResource(), r.Method) @@ -185,9 +179,8 @@ func (s *filesConnector) Connect(ctx context.Context, name string, opts runtime. return } - // something failed - if len(obj.Errors) > 0 && code < 400 { - code = http.StatusInternalServerError + if len(obj.Errors) > 0 { + code = http.StatusPartialContent } logger.Debug("request resulted in valid object", "object", obj) diff --git a/pkg/registry/apis/provisioning/jobs/migrate/resources.go b/pkg/registry/apis/provisioning/jobs/migrate/resources.go index 4c43ae06485..e752d785279 100644 --- a/pkg/registry/apis/provisioning/jobs/migrate/resources.go +++ b/pkg/registry/apis/provisioning/jobs/migrate/resources.go @@ -57,7 +57,7 @@ func (r *legacyResourceResourceMigrator) Write(ctx context.Context, key *resourc parsed, err := r.parser.Parse(ctx, &repository.FileInfo{ Path: "", // empty path to ignore file system Data: value, - }, false) + }) if err != nil { return fmt.Errorf("failed to unmarshal unstructured: %w", err) } diff --git a/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go b/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go index 7c7a22bf2b5..25a42cb8dc9 100644 --- a/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go +++ b/pkg/registry/apis/provisioning/jobs/pullrequest/worker.go @@ -109,7 +109,7 @@ func (c *PullRequestWorker) Process(ctx context.Context, return fmt.Errorf("read file: %w", err) } - _, err = parser.Parse(ctx, fileInfo, true) + _, err = parser.Parse(ctx, fileInfo) if err != nil { if errors.Is(err, resources.ErrUnableToReadResourceBytes) { progress.SetFinalMessage(ctx, "file changes is not valid resource") diff --git a/pkg/registry/apis/provisioning/jobs/sync/worker.go b/pkg/registry/apis/provisioning/jobs/sync/worker.go index 6d334ac61ba..60c345a8a89 100644 --- a/pkg/registry/apis/provisioning/jobs/sync/worker.go +++ b/pkg/registry/apis/provisioning/jobs/sync/worker.go @@ -309,14 +309,12 @@ func (r *syncJob) applyChanges(ctx context.Context, changes []ResourceFileChange name, gvk, err := r.resourceManager.WriteResourceFromFile(ctx, change.Path, "") result := jobs.JobResourceResult{ - Path: change.Path, - Action: change.Action, - Name: name, - Error: err, - } - if gvk != nil { - result.Resource = gvk.Kind - result.Group = gvk.Group + Path: change.Path, + Action: change.Action, + Name: name, + Error: err, + Resource: gvk.Kind, + Group: gvk.Group, } r.progress.Record(ctx, result) } @@ -392,30 +390,24 @@ func (r *syncJob) applyVersionedChanges(ctx context.Context, repo repository.Ver result.Error = fmt.Errorf("write resource: %w", err) } result.Name = name - if gvk != nil { - result.Resource = gvk.Kind - result.Group = gvk.Group - } + result.Resource = gvk.Kind + result.Group = gvk.Group case repository.FileActionDeleted: name, gvk, err := r.resourceManager.RemoveResourceFromFile(ctx, change.Path, change.PreviousRef) if err != nil { result.Error = fmt.Errorf("delete resource: %w", err) } result.Name = name - if gvk != nil { - result.Resource = gvk.Kind - result.Group = gvk.Group - } + result.Resource = gvk.Kind + result.Group = gvk.Group case repository.FileActionRenamed: name, gvk, err := r.resourceManager.RenameResourceFile(ctx, change.Path, change.PreviousRef, change.Path, change.Ref) if err != nil { result.Error = fmt.Errorf("rename resource: %w", err) } result.Name = name - if gvk != nil { - result.Resource = gvk.Kind - result.Group = gvk.Group - } + result.Resource = gvk.Kind + result.Group = gvk.Group case repository.FileActionIgnored: // do nothing } diff --git a/pkg/registry/apis/provisioning/resources/dualwriter.go b/pkg/registry/apis/provisioning/resources/dualwriter.go index a1719d16ece..5c25094c1cc 100644 --- a/pkg/registry/apis/provisioning/resources/dualwriter.go +++ b/pkg/registry/apis/provisioning/resources/dualwriter.go @@ -2,12 +2,12 @@ package resources import ( "context" - "errors" "fmt" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/grafana/grafana-app-sdk/logging" "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" provisioning "github.com/grafana/grafana/pkg/apis/provisioning/v0alpha1" "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository" @@ -34,23 +34,17 @@ func (r *DualReadWriter) Read(ctx context.Context, path string, ref string) (*Pa info, err := r.repo.Read(ctx, path, ref) if err != nil { - return nil, err + return nil, fmt.Errorf("read file: %w", err) } - parsed, err := r.parser.Parse(ctx, info, true) + parsed, err := r.parser.Parse(ctx, info) if err != nil { - return nil, err + return nil, fmt.Errorf("parse file: %w", err) } - // GVR will exist for anything we can actually save - // TODO: Add known error in parser for unsupported resource - if parsed.GVR == nil { - if parsed.GVK != nil { - //nolint:govet - parsed.Errors = append(parsed.Errors, fmt.Errorf("unknown resource for Kind: %s", parsed.GVK.Kind)) - } else { - parsed.Errors = append(parsed.Errors, fmt.Errorf("unknown resource")) - } + // Fail as we use the dry run for this response and it's not about updating the resource + if err := parsed.DryRun(ctx); err != nil { + return nil, fmt.Errorf("run dry run: %w", err) } return parsed, nil @@ -68,14 +62,14 @@ func (r *DualReadWriter) Delete(ctx context.Context, path string, ref string, me file, err := r.repo.Read(ctx, path, ref) if err != nil { - return nil, err // unable to read value + return nil, fmt.Errorf("read file: %w", err) } // TODO: document in API specification // We can only delete parsable things - parsed, err := r.parser.Parse(ctx, file, false) + parsed, err := r.parser.Parse(ctx, file) if err != nil { - return nil, err // unable to read value + return nil, fmt.Errorf("parse file: %w", err) } parsed.Action = provisioning.ResourceActionDelete @@ -162,25 +156,9 @@ func (r *DualReadWriter) CreateResource(ctx context.Context, path string, ref st Ref: ref, } - // TODO: improve parser to parse out of reader - parsed, err := r.parser.Parse(ctx, info, true) + parsed, err := r.parser.Parse(ctx, info) if err != nil { - if errors.Is(err, ErrUnableToReadResourceBytes) { - return nil, apierrors.NewBadRequest("unable to read the request as a resource") - } - - return nil, err - } - - // GVR will exist for anything we can actually save - // TODO: Add known error in parser for unsupported resource - if parsed.GVR == nil { - return nil, apierrors.NewBadRequest("The payload does not map to a known resource") - } - - // Do not write if any errors exist - if len(parsed.Errors) > 0 { - return parsed, err + return nil, fmt.Errorf("parse file: %w", err) } data, err = parsed.ToSaveBytes() @@ -188,8 +166,7 @@ func (r *DualReadWriter) CreateResource(ctx context.Context, path string, ref st return nil, err } - err = r.repo.Create(ctx, path, ref, data, message) - if err != nil { + if err := r.repo.Create(ctx, path, ref, data, message); err != nil { return nil, fmt.Errorf("create resource in repository: %w", err) } @@ -198,12 +175,23 @@ func (r *DualReadWriter) CreateResource(ctx context.Context, path string, ref st // FIXME: to make sure if behaves in the same way as in sync, we should // we should refactor the code to use the same function. if ref == "" { - if err := r.writeParsed(ctx, path, parsed); err != nil { - parsed.Errors = append(parsed.Errors, err) + if _, err := r.folders.EnsureFolderPathExist(ctx, path); err != nil { + return nil, fmt.Errorf("ensure folder path exists: %w", err) + } + + if err := parsed.Run(ctx); err != nil { + return nil, fmt.Errorf("run resource: %w", err) + } + } else { + if err := parsed.DryRun(ctx); err != nil { + logger := logging.FromContext(ctx).With("path", path, "name", parsed.Obj.GetName(), "ref", ref) + logger.Warn("failed to dry run resource on create", "error", err) + // Do not fail here as it's purely informational + parsed.Errors = append(parsed.Errors, err.Error()) } } - return parsed, err + return parsed, nil } // UpdateResource updates a resource in the repository @@ -219,24 +207,9 @@ func (r *DualReadWriter) UpdateResource(ctx context.Context, path string, ref st } // TODO: improve parser to parse out of reader - parsed, err := r.parser.Parse(ctx, info, true) + parsed, err := r.parser.Parse(ctx, info) if err != nil { - if errors.Is(err, ErrUnableToReadResourceBytes) { - return nil, apierrors.NewBadRequest("unable to read the request as a resource") - } - - return nil, err - } - - // GVR will exist for anything we can actually save - // TODO: Add known error in parser for unsupported resource - if parsed.GVR == nil { - return nil, apierrors.NewBadRequest("The payload does not map to a known resource") - } - - // Do not write if any errors exist - if len(parsed.Errors) > 0 { - return parsed, err + return nil, fmt.Errorf("parse file: %w", err) } data, err = parsed.ToSaveBytes() @@ -244,8 +217,7 @@ func (r *DualReadWriter) UpdateResource(ctx context.Context, path string, ref st return nil, err } - err = r.repo.Update(ctx, path, ref, data, message) - if err != nil { + if err = r.repo.Update(ctx, path, ref, data, message); err != nil { return nil, fmt.Errorf("update resource in repository: %w", err) } @@ -254,33 +226,21 @@ func (r *DualReadWriter) UpdateResource(ctx context.Context, path string, ref st // FIXME: to make sure if behaves in the same way as in sync, we should // we should refactor the code to use the same function. if ref == "" { - if err := r.writeParsed(ctx, path, parsed); err != nil { - parsed.Errors = append(parsed.Errors, err) + if _, err := r.folders.EnsureFolderPathExist(ctx, path); err != nil { + return nil, fmt.Errorf("ensure folder path exists: %w", err) } - } - return parsed, err -} - -// writeParsed write parsed resource to the repository and grafana database -func (r *DualReadWriter) writeParsed(ctx context.Context, path string, parsed *ParsedResource) error { - if _, err := r.folders.EnsureFolderPathExist(ctx, path); err != nil { - return fmt.Errorf("ensure folder path exists: %w", err) - } - - // FIXME: I don't like this parsed strategy here - var err error - if parsed.Existing == nil { - parsed.Upsert, err = parsed.Client.Create(ctx, parsed.Obj, metav1.CreateOptions{}) - if err != nil { - return fmt.Errorf("create resource: %w", err) + if err := parsed.Run(ctx); err != nil { + return nil, fmt.Errorf("run resource: %w", err) } } else { - parsed.Upsert, err = parsed.Client.Update(ctx, parsed.Obj, metav1.UpdateOptions{}) - if err != nil { - return fmt.Errorf("update resource: %w", err) + if err := parsed.DryRun(ctx); err != nil { + // Do not fail here as it's purely informational + logger := logging.FromContext(ctx).With("path", path, "name", parsed.Obj.GetName(), "ref", ref) + logger.Warn("failed to dry run resource on update", "error", err) + parsed.Errors = append(parsed.Errors, err.Error()) } } - return nil + return parsed, nil } diff --git a/pkg/registry/apis/provisioning/resources/fileformat.go b/pkg/registry/apis/provisioning/resources/fileformat.go index b11795d3ca2..dfff81efa62 100644 --- a/pkg/registry/apis/provisioning/resources/fileformat.go +++ b/pkg/registry/apis/provisioning/resources/fileformat.go @@ -18,8 +18,10 @@ import ( "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository" ) -var ErrUnableToReadResourceBytes = errors.New("unable to read bytes as a resource") -var ErrClassicResourceIsAlreadyK8sForm = errors.New("classic resource is already structured with apiVersion and kind") +var ( + ErrUnableToReadResourceBytes = errors.New("unable to read bytes as a resource") + ErrClassicResourceIsAlreadyK8sForm = errors.New("classic resource is already structured with apiVersion and kind") +) // This reads a "classic" file format and will convert it to an unstructured k8s resource // The file path may determine how the resource is parsed diff --git a/pkg/registry/apis/provisioning/resources/fileformat_test.go b/pkg/registry/apis/provisioning/resources/fileformat_test.go index 925272dfa59..7bb0de5295d 100644 --- a/pkg/registry/apis/provisioning/resources/fileformat_test.go +++ b/pkg/registry/apis/provisioning/resources/fileformat_test.go @@ -101,15 +101,14 @@ spec: }, } - // try to validate (and lint) - validate := true - // Support dashboard conversion - parsed, err := parser.Parse(context.Background(), info, validate) - require.Error(t, err) // no clients configured! + parsed, err := parser.Parse(context.Background(), info) + require.EqualError(t, err, "no clients configured") + err = parsed.DryRun(context.Background()) + require.EqualError(t, err, "no client configured") require.Equal(t, provisioning.ClassicDashboard, parsed.Classic) - require.Equal(t, &schema.GroupVersionKind{ + require.Equal(t, schema.GroupVersionKind{ Group: "dashboard.grafana.app", Version: "v0alpha1", Kind: "Dashboard", diff --git a/pkg/registry/apis/provisioning/resources/id.go b/pkg/registry/apis/provisioning/resources/id.go index 66ebc82329b..b32355b4b5f 100644 --- a/pkg/registry/apis/provisioning/resources/id.go +++ b/pkg/registry/apis/provisioning/resources/id.go @@ -91,15 +91,6 @@ func appendHashSuffix(hashKey, repositoryName string) func(string) string { } } -// Will pick a name based on the hashed repository and path -func FileNameFromHashedRepoPath(repo string, fpath string) string { - // Remove the extension: we don't want the extension to impact the ID. This lets the user change between all supported formats. - fpath = safepath.RemoveExt(fpath) - hasher := appendHashSuffix(fpath, repo) - - return hasher(safepath.Base(fpath)) -} - // Folder contains the data for a folder we use in provisioning. type Folder struct { // Title is the human-readable name created by a human who wrote it. diff --git a/pkg/registry/apis/provisioning/resources/id_test.go b/pkg/registry/apis/provisioning/resources/id_test.go index 8aa681b876f..ec92cb29d94 100644 --- a/pkg/registry/apis/provisioning/resources/id_test.go +++ b/pkg/registry/apis/provisioning/resources/id_test.go @@ -67,13 +67,6 @@ func TestAppendHashSuffix(t *testing.T) { } } -func TestFileNameFromHashedRepoPath(t *testing.T) { - dashName := FileNameFromHashedRepoPath("xyz", "path/to/folder/dashboard.json") - assert.Equal(t, "dashboard-fy2kflbmskvt6u-9uecoahd1ekwbb7", dashName, "dashboard name of dashboard.json") - // We only want 40 characters because UIDs support no more. When we get rid of legacy storage, we can extend the support to 253 character long strings. - assert.LessOrEqual(t, len(dashName), 40, "dashName after hashing needs to be <=40 chars long") -} - func TestParseFolderID(t *testing.T) { const repoName = "unit-test" // we have other tests verifying the repo name changes the id diff --git a/pkg/registry/apis/provisioning/resources/parser.go b/pkg/registry/apis/provisioning/resources/parser.go index 613f3fd9df5..1e39694fc03 100644 --- a/pkg/registry/apis/provisioning/resources/parser.go +++ b/pkg/registry/apis/provisioning/resources/parser.go @@ -4,15 +4,14 @@ import ( "bytes" "context" "encoding/json" - "errors" "fmt" "path" "gopkg.in/yaml.v3" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/client-go/dynamic" "github.com/grafana/grafana-app-sdk/logging" @@ -24,10 +23,6 @@ import ( "github.com/grafana/grafana/pkg/registry/apis/provisioning/safepath" ) -var ( - ErrNamespaceMismatch = errors.New("the file namespace does not match target namespace") -) - type ParserFactory struct { ClientFactory *ClientFactory } @@ -83,9 +78,9 @@ type ParsedResource struct { Meta utils.GrafanaMetaAccessor // The Kind is defined in the file - GVK *schema.GroupVersionKind + GVK schema.GroupVersionKind // The Resource is found by mapping Kind to the right apiserver - GVR *schema.GroupVersionResource + GVR schema.GroupVersionResource // Client that can talk to this resource Client dynamic.ResourceInterface @@ -103,7 +98,7 @@ type ParsedResource struct { Upsert *unstructured.Unstructured // If we got some Errors - Errors []error + Errors []string } // FIXME: eliminate clients from parser (but be careful that we can use the same cache/resolved GVK+GVR) @@ -111,32 +106,33 @@ func (r *Parser) Clients() ResourceClients { return r.clients } -func (r *Parser) Parse(ctx context.Context, info *repository.FileInfo, validate bool) (parsed *ParsedResource, err error) { - logger := logging.FromContext(ctx).With("path", info.Path, "validate", validate) +func (r *Parser) Parse(ctx context.Context, info *repository.FileInfo) (parsed *ParsedResource, err error) { + logger := logging.FromContext(ctx).With("path", info.Path) parsed = &ParsedResource{ Info: info, Repo: r.repo, } if err := IsPathSupported(info.Path); err != nil { - return parsed, err + return nil, err } - parsed.Obj, parsed.GVK, err = DecodeYAMLObject(bytes.NewBuffer(info.Data)) - if err != nil { - logger.Debug("failed to find GVK of the input data", "error", err) - parsed.Obj, parsed.GVK, parsed.Classic, err = ReadClassicResource(ctx, info) - if err != nil { - logger.Debug("also failed to get GVK from fallback loader?", "error", err) - return parsed, err + var gvk *schema.GroupVersionKind + parsed.Obj, gvk, err = DecodeYAMLObject(bytes.NewBuffer(info.Data)) + if err != nil || gvk == nil { + 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, err } } + parsed.GVK = *gvk + if r.urls != nil { parsed.URLs, err = r.urls.ResourceURLs(ctx, info) if err != nil { - logger.Debug("failed to load resource URLs", "error", err) - return parsed, err + return nil, fmt.Errorf("load resource URLs: %w", err) } } @@ -149,13 +145,13 @@ func (r *Parser) Parse(ctx context.Context, info *repository.FileInfo, validate parsed.Meta, err = utils.MetaAccessor(parsed.Obj) if err != nil { - return nil, err + return nil, fmt.Errorf("get meta accessor: %w", err) } obj := parsed.Obj // Validate the namespace if obj.GetNamespace() != "" && obj.GetNamespace() != r.repo.Namespace { - parsed.Errors = append(parsed.Errors, ErrNamespaceMismatch) + return nil, apierrors.NewBadRequest("the file namespace does not match target namespace") } obj.SetNamespace(r.repo.Namespace) @@ -169,9 +165,7 @@ func (r *Parser) Parse(ctx context.Context, info *repository.FileInfo, validate }) if obj.GetName() == "" && obj.GetGenerateName() == "" { - parsed.Errors = append(parsed.Errors, - field.Required(field.NewPath("name", "metadata", "name"), - "An explicit name must be saved in the resource (or generateName)")) + return nil, ErrMissingName } // Calculate folder identifier from the file path @@ -184,54 +178,69 @@ func (r *Parser) Parse(ctx context.Context, info *repository.FileInfo, validate obj.SetUID("") // clear identifiers obj.SetResourceVersion("") // clear identifiers - // We can not do anything more if no kind is defined - if parsed.GVK == nil { - return parsed, nil - } - + // FIXME: remove this check once we have better unit tests if r.clients == nil { - return parsed, fmt.Errorf("no client configured") + return parsed, fmt.Errorf("no clients configured") } - client, gvr, err := r.clients.ForKind(*parsed.GVK) + // TODO: catch the not found gvk error to return bad request + parsed.Client, parsed.GVR, err = r.clients.ForKind(parsed.GVK) if err != nil { - return nil, err // does not map to a resour e + return nil, fmt.Errorf("get client for kind: %w", err) } - parsed.GVR = &gvr - parsed.Client = client - if !validate { - return parsed, nil - } + return parsed, nil +} - if parsed.Client == nil { - parsed.Errors = append(parsed.Errors, fmt.Errorf("unable to find client")) - return parsed, nil +func (f *ParsedResource) DryRun(ctx context.Context) error { + // FIXME: remove this check once we have better unit tests + if f.Client == nil { + return fmt.Errorf("no client configured") } + var err error + // FIXME: shouldn't we check for the specific error? // Dry run CREATE or UPDATE - parsed.Existing, _ = parsed.Client.Get(ctx, obj.GetName(), metav1.GetOptions{}) - if parsed.Existing == nil { - parsed.Action = provisioning.ResourceActionCreate - parsed.DryRunResponse, err = parsed.Client.Create(ctx, obj, metav1.CreateOptions{ + f.Existing, _ = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{}) + if f.Existing == nil { + f.Action = provisioning.ResourceActionCreate + f.DryRunResponse, err = f.Client.Create(ctx, f.Obj, metav1.CreateOptions{ DryRun: []string{"All"}, }) } else { - parsed.Action = provisioning.ResourceActionUpdate - parsed.DryRunResponse, err = parsed.Client.Update(ctx, obj, metav1.UpdateOptions{ + f.Action = provisioning.ResourceActionUpdate + f.DryRunResponse, err = f.Client.Update(ctx, f.Obj, metav1.UpdateOptions{ DryRun: []string{"All"}, }) } // When the name is missing (and generateName is configured) use the value from DryRun - if obj.GetName() == "" && parsed.DryRunResponse != nil { - obj.SetName(parsed.DryRunResponse.GetName()) + if f.Obj.GetName() == "" && f.DryRunResponse != nil { + f.Obj.SetName(f.DryRunResponse.GetName()) } - if err != nil { - parsed.Errors = append(parsed.Errors, err) + return err +} + +func (f *ParsedResource) Run(ctx context.Context) error { + // FIXME: remove this check once we have better unit tests + if f.Client == nil { + return fmt.Errorf("unable to find client") } - return parsed, nil + + var err error + // FIXME: shouldn't we check for the specific error? + // Run update or create + f.Existing, _ = f.Client.Get(ctx, f.Obj.GetName(), metav1.GetOptions{}) + if f.Existing == nil { + f.Action = provisioning.ResourceActionCreate + f.Upsert, err = f.Client.Create(ctx, f.Obj, metav1.CreateOptions{}) + } else { + f.Action = provisioning.ResourceActionUpdate + f.Upsert, err = f.Client.Update(ctx, f.Obj, metav1.UpdateOptions{}) + } + + return err } func (f *ParsedResource) ToSaveBytes() ([]byte, error) { @@ -267,16 +276,10 @@ func (f *ParsedResource) AsResourceWrapper() *provisioning.ResourceWrapper { Action: f.Action, } - if f.GVK != nil { - res.Type.Group = f.GVK.Group - res.Type.Version = f.GVK.Version - res.Type.Kind = f.GVK.Kind - } - - // The resource (GVR) is derived from the kind (GVK) - if f.GVR != nil { - res.Type.Resource = f.GVR.Resource - } + res.Type.Group = f.GVK.Group + res.Type.Version = f.GVK.Version + res.Type.Kind = f.GVK.Kind + res.Type.Resource = f.GVR.Resource if f.Obj != nil { res.File = v0alpha1.Unstructured{Object: f.Obj.Object} @@ -297,9 +300,8 @@ func (f *ParsedResource) AsResourceWrapper() *provisioning.ResourceWrapper { URLs: f.URLs, Timestamp: info.Modified, Resource: res, + Errors: f.Errors, } - for _, err := range f.Errors { - wrap.Errors = append(wrap.Errors, err.Error()) - } + return wrap } diff --git a/pkg/registry/apis/provisioning/resources/parser_test.go b/pkg/registry/apis/provisioning/resources/parser_test.go index 9a03321293b..391e8abbd4a 100644 --- a/pkg/registry/apis/provisioning/resources/parser_test.go +++ b/pkg/registry/apis/provisioning/resources/parser_test.go @@ -13,7 +13,7 @@ import ( "github.com/grafana/grafana/pkg/registry/apis/provisioning/repository" ) -func TestParer(t *testing.T) { +func TestParser(t *testing.T) { clients := NewMockResourceClients(t) clients.On("ForKind", dashboardV0.DashboardResourceInfo.GroupVersionKind()). Return(nil, dashboardV0.DashboardResourceInfo.GroupVersionResource(), nil).Maybe() @@ -32,7 +32,7 @@ func TestParer(t *testing.T) { t.Run("invalid input", func(t *testing.T) { _, err := parser.Parse(context.Background(), &repository.FileInfo{ Data: []byte("hello"), // not a real resource - }, false) + }) require.Error(t, err) require.Equal(t, "classic resource must be JSON", err.Error()) }) @@ -46,7 +46,7 @@ metadata: spec: title: Test dashboard `), - }, false) + }) require.NoError(t, err) require.Equal(t, "test-v0", dash.Obj.GetName()) require.Equal(t, "dashboard.grafana.app", dash.GVK.Group) @@ -55,20 +55,19 @@ spec: require.Equal(t, "v0alpha1", dash.GVR.Version) // Now try again without a name - dash, err = parser.Parse(context.Background(), &repository.FileInfo{ + _, err = parser.Parse(context.Background(), &repository.FileInfo{ Data: []byte(`apiVersion: dashboard.grafana.app/v1alpha1 kind: Dashboard spec: title: Test dashboard `), - }, false) - require.NoError(t, err) // parsed, but has internal error - require.NotEmpty(t, dash.Errors) + }) + require.EqualError(t, err, "name.metadata.name: Required value: missing name in resource") // Read the name from classic grafana format dash, err = parser.Parse(context.Background(), &repository.FileInfo{ Data: []byte(`{ "uid": "test", "schemaVersion": 30, "panels": [], "tags": [] }`), - }, false) + }) require.NoError(t, err) require.Equal(t, v0alpha1.ClassicDashboard, dash.Classic) require.Equal(t, "test", dash.Obj.GetName()) diff --git a/pkg/registry/apis/provisioning/resources/resources.go b/pkg/registry/apis/provisioning/resources/resources.go index b6a14f9f8fb..22731d63594 100644 --- a/pkg/registry/apis/provisioning/resources/resources.go +++ b/pkg/registry/apis/provisioning/resources/resources.go @@ -18,7 +18,10 @@ import ( "github.com/grafana/grafana/pkg/registry/apis/provisioning/safepath" ) -var ErrAlreadyInRepository = errors.New("already in repository") +var ( + ErrAlreadyInRepository = errors.New("already in repository") + ErrMissingName = field.Required(field.NewPath("name", "metadata", "name"), "missing name in resource") +) type WriteOptions struct { Path string @@ -77,8 +80,7 @@ func (r *ResourcesManager) CreateResourceFileFromObject(ctx context.Context, obj name := meta.GetName() if name == "" { - return "", field.Required(field.NewPath("name", "metadata", "name"), - "An explicit name must be saved in the resource") + return "", ErrMissingName } manager, _ := meta.GetManagerProperties() @@ -131,16 +133,20 @@ func (r *ResourcesManager) CreateResourceFileFromObject(ctx context.Context, obj return fileName, nil } -func (r *ResourcesManager) WriteResourceFromFile(ctx context.Context, path string, ref string) (string, *schema.GroupVersionKind, error) { +func (r *ResourcesManager) WriteResourceFromFile(ctx context.Context, path string, ref string) (string, schema.GroupVersionKind, error) { // Read the referenced file fileInfo, err := r.repo.Read(ctx, path, ref) if err != nil { - return "", nil, fmt.Errorf("failed to read file: %w", err) + return "", schema.GroupVersionKind{}, fmt.Errorf("failed to read file: %w", err) } - parsed, err := r.parser.Parse(ctx, fileInfo, false) // no validation + parsed, err := r.parser.Parse(ctx, fileInfo) if err != nil { - return "", nil, fmt.Errorf("failed to parse file: %w", err) + return "", schema.GroupVersionKind{}, fmt.Errorf("failed to parse file: %w", err) + } + + if parsed.Obj.GetName() == "" { + return "", schema.GroupVersionKind{}, ErrMissingName } // Check if the resource already exists @@ -171,7 +177,7 @@ func (r *ResourcesManager) WriteResourceFromFile(ctx context.Context, path strin return parsed.Obj.GetName(), parsed.GVK, err } -func (r *ResourcesManager) RenameResourceFile(ctx context.Context, previousPath, previousRef, newPath, newRef string) (string, *schema.GroupVersionKind, error) { +func (r *ResourcesManager) RenameResourceFile(ctx context.Context, previousPath, previousRef, newPath, newRef string) (string, schema.GroupVersionKind, error) { name, gvk, err := r.RemoveResourceFromFile(ctx, previousPath, previousRef) if err != nil { return name, gvk, fmt.Errorf("failed to remove resource: %w", err) @@ -180,34 +186,33 @@ func (r *ResourcesManager) RenameResourceFile(ctx context.Context, previousPath, return r.WriteResourceFromFile(ctx, newPath, newRef) } -func (r *ResourcesManager) RemoveResourceFromFile(ctx context.Context, path string, ref string) (string, *schema.GroupVersionKind, error) { +func (r *ResourcesManager) RemoveResourceFromFile(ctx context.Context, path string, ref string) (string, schema.GroupVersionKind, error) { info, err := r.repo.Read(ctx, path, ref) if err != nil { - return "", nil, fmt.Errorf("failed to read file: %w", err) + return "", schema.GroupVersionKind{}, fmt.Errorf("failed to read file: %w", err) } obj, gvk, _ := DecodeYAMLObject(bytes.NewBuffer(info.Data)) if obj == nil { - return "", nil, fmt.Errorf("no object found") + return "", schema.GroupVersionKind{}, fmt.Errorf("no object found") } objName := obj.GetName() if objName == "" { - // Find the referenced file - objName = FileNameFromHashedRepoPath(r.repo.Config().Name, path) + return "", schema.GroupVersionKind{}, ErrMissingName } client, _, err := r.clients.ForKind(*gvk) if err != nil { - return "", nil, fmt.Errorf("unable to get client for deleted object: %w", err) + return "", schema.GroupVersionKind{}, fmt.Errorf("unable to get client for deleted object: %w", err) } err = client.Delete(ctx, objName, metav1.DeleteOptions{}) if err != nil { - return "", nil, fmt.Errorf("failed to delete: %w", err) + return "", schema.GroupVersionKind{}, fmt.Errorf("failed to delete: %w", err) } - return objName, gvk, nil + return objName, schema.GroupVersionKind{}, nil } func (r *ResourcesManager) withAuthorSignature(ctx context.Context, item utils.GrafanaMetaAccessor) context.Context {