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

@@ -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
 }

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)
+	this, diags := config.Load(m.fullPath)
+	if diags.HasError() {
+		return diags
+	}
+	err := b.Config.Merge(this)
 	if err != nil {
-		return diag.FromErr(err)
+		diags = diags.Extend(diag.FromErr(err))
 	}
-	// TODO: Return actual warnings.
-	err = b.Config.Merge(this)
-	return diag.FromErr(err)
+	return diags
 }

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,42 +68,40 @@ 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{}
 
 	// 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 {

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)")
 }
 

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)
 		}