-
Notifications
You must be signed in to change notification settings - Fork 167
acc: add BundleConfig setting #2809
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
Changes from all commits
6b23bd9
e74b58a
8e46c19
8a4c0a1
4626264
c072c8f
86a4bae
b3beab9
17f103c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package internal | ||
|
|
||
| import ( | ||
| "bytes" | ||
|
|
||
| "dario.cat/mergo" | ||
| "gopkg.in/yaml.v3" | ||
| ) | ||
|
|
||
| func MergeBundleConfig(source string, bundleConfig map[string]any) (string, error) { | ||
| config := make(map[string]any) | ||
|
|
||
| err := yaml.Unmarshal([]byte(source), &config) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| err = mergo.Merge( | ||
| &config, | ||
| bundleConfig, | ||
| mergo.WithoutDereference, | ||
| ) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| var buf bytes.Buffer | ||
| enc := yaml.NewEncoder(&buf) | ||
| enc.SetIndent(2) | ||
|
|
||
| err = enc.Encode(config) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| updated := buf.String() | ||
| return updated, nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,162 @@ | ||
| package internal | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "gopkg.in/yaml.v3" | ||
| ) | ||
|
|
||
| func TestMergeBundleConfig(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| initialYaml string | ||
| bundleConfig map[string]any | ||
| expected map[string]any | ||
| }{ | ||
| { | ||
| name: "new_file", | ||
| initialYaml: "", | ||
| bundleConfig: map[string]any{}, | ||
| expected: map[string]any{}, | ||
| }, | ||
| { | ||
| name: "empty_config", | ||
| initialYaml: "{}", | ||
| bundleConfig: map[string]any{}, | ||
| expected: map[string]any{}, | ||
| }, | ||
| { | ||
| name: "simple_top_level", | ||
| initialYaml: "{}", | ||
| bundleConfig: map[string]any{ | ||
| "name": "test-bundle", | ||
| }, | ||
| expected: map[string]any{ | ||
| "name": "test-bundle", | ||
| }, | ||
| }, | ||
| { | ||
| name: "simple_top_level_new_file", | ||
| initialYaml: "", | ||
| bundleConfig: map[string]any{ | ||
| "name": "test-bundle", | ||
| }, | ||
| expected: map[string]any{ | ||
| "name": "test-bundle", | ||
| }, | ||
| }, | ||
| { | ||
| name: "nested_config", | ||
| initialYaml: "{}", | ||
| bundleConfig: map[string]any{ | ||
| "bundle": map[string]any{ | ||
| "name": "test-bundle", | ||
| }, | ||
| }, | ||
| expected: map[string]any{ | ||
| "bundle": map[string]any{ | ||
| "name": "test-bundle", | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "nested_config_new_file", | ||
| initialYaml: "", | ||
| bundleConfig: map[string]any{ | ||
| "bundle": map[string]any{ | ||
| "name": "test-bundle", | ||
| }, | ||
| }, | ||
| expected: map[string]any{ | ||
| "bundle": map[string]any{ | ||
| "name": "test-bundle", | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "merge_with_existing_config", | ||
| initialYaml: ` | ||
| bundle: | ||
| name: original-name | ||
| target: dev | ||
| `, | ||
| bundleConfig: map[string]any{ | ||
| "bundle": map[string]any{ | ||
| "name": "default-name", | ||
| }, | ||
| }, | ||
| expected: map[string]any{ | ||
| "bundle": map[string]any{ | ||
| "name": "original-name", | ||
| "target": "dev", | ||
| }, | ||
| }, | ||
| }, | ||
| { | ||
| name: "merge_with_existing_config_2", | ||
| initialYaml: `resources: {}`, | ||
| bundleConfig: map[string]any{ | ||
| "bundle": map[string]any{ | ||
| "name": "new-name", | ||
| }, | ||
| }, | ||
| expected: map[string]any{ | ||
| "bundle": map[string]any{ | ||
| "name": "new-name", | ||
| }, | ||
| "resources": map[string]any{}, | ||
| }, | ||
| }, | ||
|
|
||
| { | ||
| name: "multiple_nested_levels", | ||
| initialYaml: ` | ||
| resources: | ||
| jobs: | ||
| myjob: | ||
| hello: world | ||
| pipelines: | ||
| mypipeline: | ||
| name: 123 | ||
| `, | ||
| bundleConfig: map[string]any{ | ||
| "resources": map[string]any{ | ||
| "jobs": map[string]any{ | ||
| "myjob": map[string]any{ | ||
| "name": "My Job", | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| expected: map[string]any{ | ||
| "resources": map[string]any{ | ||
| "jobs": map[string]any{ | ||
| "myjob": map[string]any{ | ||
| "name": "My Job", | ||
| "hello": "world", | ||
| }, | ||
| }, | ||
| "pipelines": map[string]any{ | ||
| "mypipeline": map[string]any{ | ||
| "name": 123, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| out, err := MergeBundleConfig(tt.initialYaml, tt.bundleConfig) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this function take a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it already does?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean for "source". The mix of unmarshalling and receiving an object is confusing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you mean, but there is some logic to it: the function receives config as a string and returns new updated config as a string. This allows it to encapsulate (and potentially have test cases for) details related to yaml parsing, such as whether it is strict or not, whether it preserves comments. The test runner on the other hand knows which file we're reading and writing and handles all I/O. The fact that it also receives unmarshalled bundleConfig is just because we do all of unmarshalling of config in one place and there is no other way to receive bundleConfig.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a lot of unnecessary marshalling / unmarshalling going on with this implementation though. It works for now and does not cause noticeable overhead, but worth rewriting a bit (for later). |
||
| assert.NoError(t, err) | ||
|
|
||
| var result map[string]any | ||
| require.NoError(t, yaml.Unmarshal([]byte(out), &result)) | ||
|
|
||
| assert.Equal(t, tt.expected, result) | ||
| }) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -106,6 +106,27 @@ type TestConfig struct { | |
| // For cloud tests, max(Timeout, TimeoutCloud) is used for timeout | ||
| // For cloud+windows tests, max(Timeout, TimeoutWindows, TimeoutCloud) is used for timeout | ||
| TimeoutCloud time.Duration | ||
|
|
||
| // Maps a name (arbitrary string, can be used to override/disable setting in a child config) to a mapping that specifies how | ||
| // to update databricks.yml (or other file designated by BundleConfigTarget). | ||
| // Example: | ||
| // BundleConfig.header.bundle.name = "test-bundle" | ||
| // This overwrite bundle.name in the databricks.yml, so you can omit adding """ | ||
| // bundle: | ||
| // name: somename | ||
| // """ to every test. | ||
| // If child config wants to disable or override this, they can simply do | ||
| // BundleConfig.header = "" | ||
| BundleConfig map[string]any | ||
|
|
||
| // Target config for BundleConfig updates. Empty string disables BundleConfig updates. | ||
| // Null means "databricks.yml" | ||
| BundleConfigTarget *string | ||
|
|
||
| // To be added: | ||
| // BundleConfigMatrix is to BundleConfig what EnvMatrix is to Env | ||
| // It creates different tests for each possible configuration update. | ||
| // BundleConfigMatrix map[string][]any | ||
| } | ||
|
|
||
| type ServerStub struct { | ||
|
|
@@ -192,8 +213,11 @@ func DoLoadConfig(t *testing.T, path string) TestConfig { | |
| require.NoError(t, err, "Failed to parse config %s", path) | ||
|
|
||
| keys := meta.Undecoded() | ||
| if len(keys) > 0 { | ||
| t.Fatalf("Undecoded keys in %s: %#v", path, keys) | ||
| for ind, key := range keys { | ||
| if len(key) > 0 && key[0] == "BundleConfig" { | ||
| continue | ||
| } | ||
| t.Errorf("Undecoded key in %s[%d]: %#v", path, ind, key) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does "undecoded" mean? How do these keys now end up in the config struct?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's means a key in TOML file was not mapped to any struct field.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But how does the "BundleConfig" map get populated if it is not decoded? I would expect either:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good question! This seems to be an issue with toml parser's handling of map[string]any type. Somehow it decodes it but also complains about it, This is what happens if I remove that check: |
||
| } | ||
|
|
||
| return config | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| bundle: | ||
| name: my-bundle | ||
| resources: | ||
| jobs: | ||
| example_job: | ||
| list2: 123 | ||
| name: Example Job | ||
| new_list: | ||
| - abc | ||
| - def | ||
| new_map: | ||
| key: value | ||
| new_string: hello | ||
| string2: | ||
| - item1 | ||
| - item2 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| mv my.yml out.my.yml |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| BundleConfigTarget = "my.yml" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # should not be changed |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # should not be changed |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| cat databricks.yml |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| BundleConfigTarget = "" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # should not be changed |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # should not be changed |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| cat databricks.yml |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| BundleConfig.config1 = "" | ||
| BundleConfig.config2 = "" |
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.
The keys are the "path" in YAML to apply the override to (e.g.
config2.resources.jobs.example_job)?If so, please include comments or examples of the shape of the input to this function to clarify.
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.
config2 is not part of yaml, it is the name of the config (for override purposes). There is selftest that shows how it works.