-
Notifications
You must be signed in to change notification settings - Fork 49
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
Use dynamic walk for validating unique resource identifiers #1414
Conversation
Note: #1425 will add a test to assert resources are maps of string -> pointers to a struct. |
bundle/config/root.go
Outdated
|
||
// Error, encountered a duplicate resource identifier. | ||
return v, fmt.Errorf("multiple resources named %s (%s at %v, %s at %v)", k, ot, ol, nt, nl) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same as above. Any chance we can perform this check after loading everything? Then we could surface every conflict in the same diag. It can also be a read-only mutator at that point instead of functions on the root struct that are invoked every time we merge.
bundle/config/root_test.go
Outdated
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 TestDuplicateIdOnLoadReturnsErrorForJobAndPipeline(t *testing.T) { | ||
_, diags := Load("./testdata/duplicate_resource_names_in_root_job_and_pipeline/databricks.yml") | ||
assert.ErrorContains(t, diags.Error(), "multiple resources named foo (job at ./testdata/duplicate_resource_names_in_root_job_and_pipeline/databricks.yml:10:7, pipeline at ./testdata/duplicate_resource_names_in_root_job_and_pipeline/databricks.yml:13:7)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this error OK with slashes on Windows and below you need to run filepath.FromSlash
?
resources: | ||
jobs: | ||
foo: | ||
name: job foo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the test focuses on just the duplicate jobs and hardcodes the loads, these can be resources1
and resources2
, for example. I was looking at this and thinking this won't work because there is no include
section in this file, while it does because the loads are explicit.
bundle/config/root.go
Outdated
k := p[1].Key() | ||
if _, ok := paths[k]; ok { | ||
// Type and location of the existing resource in the map. | ||
ot := strings.TrimSuffix(paths[k][0].Key(), "s") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do this suffix trim in many places but shall we just get rid of it and show the message like?
multiple resources named <name> (jobs.<name> at ... and pipelines.<name> at ...)
If not, then better move it to a separate function and reuse
Closing in favour of #1614 |
Changes
Using dynamic walk here removes the need to add validation when a new resource type is added. All resources will be automatically validated to have unique keys.
Tests
Modified existing unit tests and added new ones.