Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return diagnostics from config.Load #1324

Merged
merged 18 commits into from
Mar 28, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions bundle/config/loader/entry_point.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
12 changes: 7 additions & 5 deletions bundle/config/loader/process_include.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
30 changes: 9 additions & 21 deletions bundle/config/root.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (

type Root struct {
value dyn.Value
diags diag.Diagnostics
depth int

// Contains user defined variables
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}()

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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 {
Expand Down
18 changes: 9 additions & 9 deletions bundle/config/root_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)")
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/bundle/validate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
Loading