Use resource.yml in field classification in config-remote-sync#4677
Use resource.yml in field classification in config-remote-sync#4677ilyakuz-db wants to merge 9 commits intomainfrom
resource.yml in field classification in config-remote-sync#4677Conversation
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Commit: 4045284
20 interesting tests: 7 flaky, 7 SKIP, 6 RECOVERED
Top 20 slowest tests (at least 2 minutes):
|
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
| email_notifications.on_failure[0]: replace | ||
| max_concurrent_runs: replace | ||
| tags['env']: remove | ||
| timeout_seconds: remove |
There was a problem hiding this comment.
This is expected, logic was incorrect previously - this case was skipped completely
| // If a CLI-defaulted field is changed on remote and should be disabled | ||
| // (e.g. queueing disabled → remote field is nil), we can't define it | ||
| // in the config as "null" because the CLI default will be applied again. | ||
| var resetValues = map[string][]resetRule{ |
There was a problem hiding this comment.
Should we include this logic in resources.yml?
| reason: managed | ||
|
|
||
| backend_defaults: | ||
| # Backend returns PERFORMANCE_OPTIMIZED for serverless jobs when not set |
There was a problem hiding this comment.
This is set only for serverless jobs, probably, this should live in changes desc overrides
There was a problem hiding this comment.
Is this field ever set for non-serverless jobs?
There was a problem hiding this comment.
It is, actually! I thought that it only applies to Serverless jobs because in UI we show this checkbox as disabled for non-Serverless, but looking at the settings object in the resource, we do have this field set to PERFORMANCE_OPTIMIZED
|
|
||
| # Same as clusters.node_type_id — see clusters/resource_cluster.go#L333 | ||
| # s.SchemaPath("node_type_id").SetComputed() | ||
| - field: tasks[*].new_cluster.node_type_id |
There was a problem hiding this comment.
When I try to create new_cluster without node_type_id it fails with 400 error:
Endpoint: POST https://<host>/api/2.2/jobs/reset
HTTP Status: 400 Bad Request
API error_code: INVALID_PARAMETER_VALUE
API message: Cluster validation error: Unknown node type id
Considering that backend default is applied only when config value is nil, and for nil value we have 400 error - it doesn't make sense to keep it here
There was a problem hiding this comment.
Question, if I don't specify new_cluster, can it be added by the backend when resource is fetched? I guess not, or some test would fail, but good to say it explicitly.
There was a problem hiding this comment.
In which case? You can either use existing_cluster_id fields or new_cluster object. node_type_id exists in new_cluster and it cannot be set independently. When you specify compute for a task, you have 3 options:
- select existing cluster (existing_cluster_id field)
- add new (new_cluster field)
- use serverless (doesn't have node_type_id)
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
changed the title
resource.yml in field classification in config-remote-sync
ilyakuz-db
requested review from
andrewnester,
anton-107,
denik,
pietern,
shreyas-goenka and
simonfaltum
as code owners
| ignore_remote_changes: | ||
| # edit_mode is set by CLI, not user-configurable | ||
| - field: edit_mode | ||
| reason: managed |
There was a problem hiding this comment.
I'm using "managed" in other places to mean managed by backend, we should have another reason here.
|
|
||
| jobs: | ||
| ignore_remote_changes: | ||
| # edit_mode is set by CLI, not user-configurable |
There was a problem hiding this comment.
You mean DABs override user setting? Do you know if that is intentional?
There was a problem hiding this comment.
No, I meant that the user can't specify this field in databricks.yml, it's not in the json schema
It's only set by CLI
There was a problem hiding this comment.
Understood, although still not clear why ignore_remote_changes for it? What if CLI sets A and user updates to B, should we not act on that drift?
There was a problem hiding this comment.
In config-remote-sync, we don't need to act, as this field only controls "Job is locked for edits" state. And when yaml sync feature is enabled, we always expect EDITABLE. We also set it explicitly during deployment:
But for the bundle plan, perhaps it could make sense. Example use case - user clicks "Disconnect this job from DAB", which changes edit_mode: UI_LOCKED -> EDITABLE in the resource. When user deploys again - the change is edit_mode: EDITABLE -> UI_LOCKED
Maybe it makes sense to keep a separate option in resources.yml for such cases, for example: ignore_for_sync / ignore_in_sync
There was a problem hiding this comment.
Moved to configsync package from here
| reason: managed | ||
|
|
||
| backend_defaults: | ||
| # Backend returns PERFORMANCE_OPTIMIZED for serverless jobs when not set |
There was a problem hiding this comment.
Is this field ever set for non-serverless jobs?
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
…rate-to-resource-yml
ilyakuz-db
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
left a comment
simonfaltum
left a comment
There was a problem hiding this comment.
Review (automated, 2 agents)
Verdict: Not ready yet | 0 Critical | 1 Major | 1 Gap(Major) | 2 Nit | 1 Suggestion
[Major] Migration drops suppression behavior for ~16 fields
bundle/direct/dresources/resources.yml
The old serverSideDefaults handled timeout_seconds, email_notifications, webhook_notifications, task disabled, pipeline_task.full_refresh, single_user_name, data_security_mode, pipeline continuous, and others. These have no equivalent rules in resources.yml yet. After this PR, config-remote-sync will start surfacing these as drift.
Additionally, removing node_type_id from backend_defaults changes deploy plan behavior (not just config-sync), potentially causing spurious updates for clusters where the backend auto-fills node type. Note the asymmetry: driver_node_type_id is still in backend_defaults.
Suggestion: Add equivalent backend_defaults/ignore_remote_changes rules for the missing fields, or explain why removing them is safe.
[Gap (Major)] No regression tests for migrated configsync classification rules
bundle/configsync/diff_test.go (deleted), bundle/configsync/diff.go
The only unit test file was deleted (90 lines of matchPattern tests). No replacement tests for shouldClassifySkip, filterEntityDefaults, or convertChangeDesc. The new glue logic (synthetic ChangeDesc{Old: nil, New: nil, Remote: value}) has subtle interactions with the classification pipeline.
Suggestion: Add table-driven tests for shouldClassifySkip and convertChangeDesc covering backend defaults, zero/nil values, nested maps, and non-default values.
[Suggestion] Document the hasConfigValue behavioral change
bundle/configsync/diff.go:107
The change from hasConfigValue := cd.Old != nil || cd.New != nil to hasConfigValue := cd.New != nil is a correctness fix (confirmed by the acceptance test update), but there's no comment explaining why only cd.New is checked, making it easy for a future contributor to "fix" it back.
[Nit] Silent error swallowing in shouldClassifySkip
bundle/configsync/diff.go:60-63 - Classification errors silently return false. A debug log would help diagnose issues.
[Nit] Array handling in filterEntityDefaults preserves nil entries
bundle/configsync/diff.go:72-77 - Filtered arrays produce [nil, nil, nil] instead of nil, inconsistent with the map case that returns nil for empty maps. Low priority.
4 participants
Changes
resource.ymlclassification in config-remote-sync instead of custom rulesresource.ymlwith new fieldsWhy
This PR eliminates tech debt, config-remote-sync will be in sync with resource updates
Tests
Existing tests (one snapshot updated because underlying bug was fixed)