From eea34b25040751862d6b720fa43f9e962970978d Mon Sep 17 00:00:00 2001 From: Pieter Noordhuis Date: Thu, 28 Mar 2024 11:59:03 +0100 Subject: [PATCH] Return diagnostics from `config.Load` (#1324) ## Changes We no longer need to store load diagnostics on the `config.Root` type itself and instead can return them from the `config.Load` call directly. It is up to the caller of this function to append them to previous diagnostics, if any. Background: previous commits moved configuration loading of the entry point into a mutator, so now all diagnostics naturally flow from applying mutators. This PR depends on #1319. ## Tests Unit and manual validation of the debug statements in the validate command. --- bundle/config/loader/entry_point.go | 12 +++++----- bundle/config/loader/process_include.go | 14 +++++++----- bundle/config/root.go | 30 ++++++++----------------- bundle/config/root_test.go | 18 +++++++-------- cmd/bundle/validate.go | 2 +- 5 files changed, 34 insertions(+), 42 deletions(-) diff --git a/bundle/config/loader/entry_point.go b/bundle/config/loader/entry_point.go index 24ba2f06..2c73a582 100644 --- a/bundle/config/loader/entry_point.go +++ b/bundle/config/loader/entry_point.go @@ -24,11 +24,13 @@ func (m *entryPoint) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics if err != nil { return diag.FromErr(err) } - this, err := config.Load(path) - if err != nil { - return diag.FromErr(err) + this, diags := config.Load(path) + if diags.HasError() { + return diags } - // TODO: Return actual warnings. err = b.Config.Merge(this) - return diag.FromErr(err) + if err != nil { + diags = diags.Extend(diag.FromErr(err)) + } + return diags } diff --git a/bundle/config/loader/process_include.go b/bundle/config/loader/process_include.go index 328f4eac..7cf9a17d 100644 --- a/bundle/config/loader/process_include.go +++ b/bundle/config/loader/process_include.go @@ -27,11 +27,13 @@ func (m *processInclude) Name() string { } func (m *processInclude) Apply(_ context.Context, b *bundle.Bundle) diag.Diagnostics { - this, err := config.Load(m.fullPath) - if err != nil { - return diag.FromErr(err) + this, diags := config.Load(m.fullPath) + if diags.HasError() { + return diags } - // TODO: Return actual warnings. - err = b.Config.Merge(this) - return diag.FromErr(err) + err := b.Config.Merge(this) + if err != nil { + diags = diags.Extend(diag.FromErr(err)) + } + return diags } diff --git a/bundle/config/root.go b/bundle/config/root.go index 0e54c04c..18b548d6 100644 --- a/bundle/config/root.go +++ b/bundle/config/root.go @@ -20,7 +20,6 @@ import ( type Root struct { value dyn.Value - diags diag.Diagnostics depth int // Contains user defined variables @@ -69,10 +68,10 @@ type Root struct { } // Load loads the bundle configuration file at the specified path. -func Load(path string) (*Root, error) { +func Load(path string) (*Root, diag.Diagnostics) { raw, err := os.ReadFile(path) if err != nil { - return nil, err + return nil, diag.FromErr(err) } r := Root{} @@ -80,31 +79,29 @@ func Load(path string) (*Root, error) { // Load configuration tree from YAML. v, err := yamlloader.LoadYAML(path, bytes.NewBuffer(raw)) if err != nil { - return nil, fmt.Errorf("failed to load %s: %w", path, err) + return nil, diag.Errorf("failed to load %s: %v", path, err) } // Rewrite configuration tree where necessary. v, err = rewriteShorthands(v) if err != nil { - return nil, fmt.Errorf("failed to rewrite %s: %w", path, err) + return nil, diag.Errorf("failed to rewrite %s: %v", path, err) } // Normalize dynamic configuration tree according to configuration type. v, diags := convert.Normalize(r, v) - // Keep track of diagnostics (warnings and errors in the schema). - // We delay acting on diagnostics until we have loaded all - // configuration files and merged them together. - r.diags = diags - // Convert normalized configuration tree to typed configuration. err = r.updateWithDynamicValue(v) if err != nil { - return nil, fmt.Errorf("failed to load %s: %w", path, err) + return nil, diag.Errorf("failed to load %s: %v", path, err) } _, err = r.Resources.VerifyUniqueResourceIdentifiers() - return &r, err + if err != nil { + diags = diags.Extend(diag.FromErr(err)) + } + return &r, diags } func (r *Root) initializeDynamicValue() error { @@ -126,11 +123,9 @@ func (r *Root) initializeDynamicValue() error { func (r *Root) updateWithDynamicValue(nv dyn.Value) error { // Hack: restore state; it may be cleared by [ToTyped] if // the configuration equals nil (happens in tests). - diags := r.diags depth := r.depth defer func() { - r.diags = diags r.depth = depth }() @@ -224,10 +219,6 @@ func (r *Root) MarkMutatorExit(ctx context.Context) error { return nil } -func (r *Root) Diagnostics() diag.Diagnostics { - return r.diags -} - // SetConfigFilePath configures the path that its configuration // was loaded from in configuration leafs that require it. func (r *Root) ConfigureConfigFilePath() { @@ -261,9 +252,6 @@ func (r *Root) InitializeVariables(vars []string) error { } func (r *Root) Merge(other *Root) error { - // Merge diagnostics. - r.diags = append(r.diags, other.diags...) - // Check for safe merge, protecting against duplicate resource identifiers err := r.Resources.VerifySafeMerge(&other.Resources) if err != nil { diff --git a/bundle/config/root_test.go b/bundle/config/root_test.go index 3b25fb1f..b5676884 100644 --- a/bundle/config/root_test.go +++ b/bundle/config/root_test.go @@ -25,24 +25,24 @@ func TestRootMarshalUnmarshal(t *testing.T) { } func TestRootLoad(t *testing.T) { - root, err := Load("../tests/basic/databricks.yml") - require.NoError(t, err) + root, diags := Load("../tests/basic/databricks.yml") + require.NoError(t, diags.Error()) assert.Equal(t, "basic", root.Bundle.Name) } func TestDuplicateIdOnLoadReturnsError(t *testing.T) { - _, err := Load("./testdata/duplicate_resource_names_in_root/databricks.yml") - assert.ErrorContains(t, err, "multiple resources named foo (job at ./testdata/duplicate_resource_names_in_root/databricks.yml, pipeline at ./testdata/duplicate_resource_names_in_root/databricks.yml)") + _, diags := Load("./testdata/duplicate_resource_names_in_root/databricks.yml") + assert.ErrorContains(t, diags.Error(), "multiple resources named foo (job at ./testdata/duplicate_resource_names_in_root/databricks.yml, pipeline at ./testdata/duplicate_resource_names_in_root/databricks.yml)") } func TestDuplicateIdOnMergeReturnsError(t *testing.T) { - root, err := Load("./testdata/duplicate_resource_name_in_subconfiguration/databricks.yml") - require.NoError(t, err) + root, diags := Load("./testdata/duplicate_resource_name_in_subconfiguration/databricks.yml") + require.NoError(t, diags.Error()) - other, err := Load("./testdata/duplicate_resource_name_in_subconfiguration/resources.yml") - require.NoError(t, err) + other, diags := Load("./testdata/duplicate_resource_name_in_subconfiguration/resources.yml") + require.NoError(t, diags.Error()) - err = root.Merge(other) + err := root.Merge(other) assert.ErrorContains(t, err, "multiple resources named foo (job at ./testdata/duplicate_resource_name_in_subconfiguration/databricks.yml, pipeline at ./testdata/duplicate_resource_name_in_subconfiguration/resources.yml)") } diff --git a/cmd/bundle/validate.go b/cmd/bundle/validate.go index 57bf6f7b..e625539b 100644 --- a/cmd/bundle/validate.go +++ b/cmd/bundle/validate.go @@ -32,7 +32,7 @@ func newValidateCommand() *cobra.Command { // Until we change up the output of this command to be a text representation, // we'll just output all diagnostics as debug logs. - for _, diag := range b.Config.Diagnostics() { + for _, diag := range diags { log.Debugf(cmd.Context(), "[%s]: %s", diag.Location, diag.Summary) }