direct: Fix "Nothing to update" when a UC comment is cleared or set outside the bundle - #6343
Draft
denik wants to merge 11 commits into
Draft
direct: Fix "Nothing to update" when a UC comment is cleared or set outside the bundle#6343denik wants to merge 11 commits into
denik wants to merge 11 commits into
Conversation
Collaborator
Integration test reportCommit: 1457ca6
Top 3 slowest tests (at least 2 minutes):
|
denik
changed the title
denik
force-pushed
the
denik/issue-6340
branch
from
August 24, 2026 12:19
e9bd2fc to
02ee8cd
Compare
denik
added a commit
that referenced
this pull request
Aug 24, 2026
## Why We already pass it to DoUpdate, so it was just an omission. Planned to be used in #6343
denik
force-pushed
the
denik/issue-6340
branch
2 times, most recently
from
August 24, 2026 16:07
a070562 to
0596606
Compare
…he bundle A schema without `comment` in the config becomes undeployable once someone sets a description on it in UC: the engine reads the remote comment, plans an update, and every field of the PATCH serializes away under omitempty. UC rejects the empty body with `400 INVALID_PARAMETER_VALUE / UpdateSchema Nothing to update`, which aborts the whole deploy. Add an acceptance test for it and make the fake workspace reject an empty UpdateSchema payload the way UC does, so the local run fails the same way the cloud one does. Co-authored-by: Isaac
…n empty PATCH Every UpdateSchema field is omitempty, so a schema whose config declares no comment produced an empty PATCH body once the comment was set out of band. UC answers that with `400 / UpdateSchema Nothing to update` rather than a no-op, which failed the whole deploy with no way out from the CLI. Force-send comment so the payload always carries a field and clearing a comment set outside the bundle actually happens. Co-authored-by: Isaac
Co-authored-by: Isaac
Catalogs and volumes fail exactly like schemas did: their update payloads carry only fields the config may leave unset, so clearing a comment that was set out of band produced an empty PATCH and `400 / Nothing to update`. Verified against a real workspace for both. Force-send comment in all four update paths (catalogs and volumes each have a rename variant), moving the shared reason into forceSendComment, and teach the fake workspace to reject an empty payload and honour an explicit empty comment the way UC does. Co-authored-by: Isaac
The bundle name is the workspace state path, and cloud tests share one real workspace, so the hardcoded "test-bundle" made these three fight over the same deploy.lock as every other test using that name. They passed run alone and failed under parallelism: the integration run reported success while retrying them on nearly every environment. Co-authored-by: Isaac
…itionally The previous commits always force-sent comment, so a deploy whose only drift was another field still put `"comment": ""` on the wire, and a field the plan had classified as skip would have been cleared behind the plan's back. Any other omitempty field the config stops setting was still dropped, so it never converged. Derive the force-send list from the plan instead, matched against the request type's own JSON names. Each PATCH now carries exactly what the plan says is changing, and roughly 40 omitempty fields across the UC resources converge instead of only comment. forceSendClearedFields documents the general shape, including why full-replacement APIs (jobs, pipelines, model serving) need none of it. Also drop the special-casing in the fake workspace: applyUpdatedFields applies whatever the payload names, including zero values, which is what a partial-update API does. That incidentally stops mergo from clobbering the stored ForceSendFields, so browse_only survives an update the way UC returns it. Co-authored-by: Isaac
… locations Removing a comment from the configuration reports a successful update and then never converges: the field is omitempty, so it leaves the payload entirely, and a partial-update API reads its absence as "leave unchanged". The comment stays, and every later plan reports the same pending change. Unlike the schema case this needs no out-of-band edit, and there is no error to notice -- the deploy says "1 changed" while changing nothing. The external locations golden also gains four synced files, since the new test lives inside that test's bundle root. Co-authored-by: Isaac
… too Apply forceSendClearedFields to the two remaining resources whose update drops a cleared field, so removing a comment from the configuration converges instead of reporting a change forever. External locations get it on both the update and the rename path. The fake workspace applies whatever an update payload names, replacing guards that swallowed an explicit empty comment. Registered models accept a payload with no field at all -- unlike schemas, catalogs and volumes, verified against a real workspace -- so parseUCUpdate is split and they use the parse-only half rather than gaining a rejection the backend does not have. Co-authored-by: Isaac
main removed the setting in #6359 while this branch was open, so the merge failed with "Undecoded key ... RequiresUnityCatalog" on the direct-engine runners. Co-authored-by: Isaac
denik
force-pushed
the
denik/issue-6340
branch
from
August 25, 2026 07:29
0596606 to
1457ca6
Compare
denik
changed the title
resources/<resource>/<test-name> matches the equivalent tests that already live flat (cluster_policies/out_of_band_change, apps/config-drift, permissions/jobs/added_remotely), and the drift prefix says nothing the names do not. Pure rename, no content change. Co-authored-by: Isaac
grep -v silently leaves the config unchanged if the pattern stops matching, and these tests would then assert convergence against a config that still declares the comment. update_file.py fails instead, and the intermediate copies of databricks.yml are no longer needed. Co-authored-by: Isaac
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
A schema that does not declare
commentbecomes undeployable as soon as someonesets a description on it in UC. The plan clears the comment, every
UpdateSchemafield is omitempty, and the empty PATCH is rejected:
Catalogs and volumes fail the same way. Registered models and external locations
always send other fields, so there is nothing to reject — instead removing a
comment from the configuration reports "1 changed" and never converges.
Changes
Force-send the fields the plan reports as cleared, matched by the request type's
own JSON name.
backend_defaultswould be wrong here: a description typed inCatalog Explorer is real drift, not a backend default, and suppressing it would
stop the bundle managing
commentat all.Fixes #6340
This pull request and its description were written by Isaac.