Config remote sync command #4289
Conversation
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Commit: cf60183
14 interesting tests: 7 KNOWN, 6 SKIP, 1 RECOVERED
Top 21 slowest tests (at least 2 minutes):
|
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
There was a problem hiding this comment.
Could you add some acceptance tests?
You can see examples here that modify remote resource:
% git ls-files | find . -type d | grep remote
./bundle/resources/permissions/jobs/deleted_remotely
./bundle/resources/permissions/jobs/added_remotely
./bundle/resources/permissions/jobs/reorder_remotely
./bundle/resources/volumes/remote-delete
./bundle/resources/volumes/remote-change-name
./bundle/resources/jobs/remote_add_tag
./bundle/resources/jobs/remote_matches_config
./bundle/resources/jobs/remote_delete
./bundle/resources/jobs/remote_delete/destroy
./bundle/resources/jobs/remote_delete/deploy
./bundle/resource_deps/remote_field_storage_location
./bundle/resource_deps/remote_app_url
./bundle/resource_deps/remote_app_url/app
./bundle/resource_deps/jobs_update_remote
./bundle/resource_deps/remote_pipeline
| schema Generate JSON Schema for bundle configuration | ||
| summary Summarize resources deployed by this bundle | ||
| sync Synchronize bundle tree to the workspace | ||
| validate Validate configuration |
There was a problem hiding this comment.
The output seems to be the same except for whitespace?
There was a problem hiding this comment.
Yes, probably a bug in help/Cobra? New command is hidden, but help reserves space for it.
bundle/configsync/diff.go
Outdated
| isDefault: isEmptyStruct, | ||
| }, | ||
| { | ||
| pattern: regexp.MustCompile(`^webhook_notifications$`), |
There was a problem hiding this comment.
You can probably drop these, they now using different reason: #4338
There was a problem hiding this comment.
Yep that works good now, thanks!
| currentValue := b.Config.Value() | ||
|
|
||
| for i, n := range nodes { | ||
| if key, ok := n.StringKey(); ok { |
There was a problem hiding this comment.
StringKey might be rendered with a dot or with [""] or with [''] depending on string contents.
I suggest to output PathNode and then convert it to string rather than recreate the string directly.
There was a problem hiding this comment.
Good catch, updated to use path nodes
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
@denik I added acceptance tests and addressed all comments |
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
denik
left a comment
denik
left a comment
There was a problem hiding this comment.
suggestions: add permissions to some of the test configs.
| } | ||
|
|
||
| if save { | ||
| if err := configsync.SaveFiles(ctx, b, files); err != nil { |
There was a problem hiding this comment.
It looks like --save will write the updated files and will also print a diff. Is that intentional? I'd expect that you either need a diff or in-place updates but not both.
There was a problem hiding this comment.
It is pretty useful, I can avoid reading the affected files in the UI when I need to pass this diff to the accept/reject UI
Also quite useful for debugging
| cmd.Flags().BoolVar(&save, "save", false, "Write updated config files to disk") | ||
|
|
||
| cmd.RunE = func(cmd *cobra.Command, args []string) error { | ||
| b, _, err := utils.ProcessBundleRet(cmd, utils.ProcessOptions{}) |
There was a problem hiding this comment.
I see that plan command does this:
cmd.RunE = func(cmd *cobra.Command, args []string) error {
opts := utils.ProcessOptions{
AlwaysPull: true,
FastValidate: true,
Build: true,
PreDeployChecks: true,
}
You need Build because it updates the configuration and hence the plan. You probably also want AlwaysPull as well.
|
|
||
| title "Updated configuration" | ||
| echo | ||
| cat databricks.yml |
There was a problem hiding this comment.
can we record diff old_databricks.yml new_databricks.yml here instead of full config? It's hard to see if there are any subtle changes there.
| my_job: | ||
| email_notifications: | ||
| on_success: | ||
| - success@example.com |
There was a problem hiding this comment.
IMO a few of these tests can be combined into one, e.g. formatting_preserved, job_email_notifications
| @@ -0,0 +1,8 @@ | |||
| Local = true | |||
| Cloud = false | |||
There was a problem hiding this comment.
Running some of these tests on Cloud would actually be useful here since extra fields backends adds can play a role.
| for task in r["tasks"]: | ||
| if task["task_key"] == "process": | ||
| task["new_cluster"]["num_workers"] = 5 | ||
| task["timeout_seconds"] = 3600 |
There was a problem hiding this comment.
suggestion: also delete a task in the middle to see how it copes.
| jsonPointers = append(jsonPointers, targetPrefix+jsonPointer) | ||
| } | ||
|
|
||
| hasConfigValue := changeDesc.Old != nil || changeDesc.New != nil |
There was a problem hiding this comment.
Do you have a test where a field is removed in remote and in config (New)?
Changes
New experimental command
databricks bundle config-remote-sync. The command fetches the latest remote changes of resources and compares them to the deployed state. With the--saveflag, it also writes changes back to YAML files.The command leverages the diffing algorithm of the
bundle plancommand. Also note that new dependency is added to do yaml patching that preserves commentsThis is a first PR, I will continue fixing all limitations in the next PRs
Current limitations:
bundle plantasks[task_key='my_task']need to be tested for cases when the object or parent doesn't existNo changelog entries are needed as the command is intended to be private for now, and we don't want to encourage users to use it due to the unstable API
Why
This command serves as the backend for the new visual authoring feature in DABs in the Workspace.
User edits job or pipeline in the Workspace UI, then clicks the "Sync" button, and then the new command applies the diff to the source configuration files. Then the user may accept or reject these changes in the editor
Tests
Currently, only unit tests to speed up devloop, but once I have full functionality, I'll add proper acceptance test coverage
Acceptance tests are failing because of the change in formatting (even though the command is hidden)