Fix "Nothing to update" when a UC comment is set outside the bundle - #6343
Draft
denik wants to merge 9 commits into
Draft
Fix "Nothing to update" when a UC comment is set outside the bundle#6343denik wants to merge 9 commits into
denik wants to merge 9 commits into
Conversation
Collaborator
Integration test reportCommit: 0596606
Top 16 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
from
August 24, 2026 13:39
02ee8cd to
207cd11
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 24, 2026 16:07
a070562 to
0596606
Compare
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.
Changes
A schema declared without
commentbecame undeployable on the direct engine assoon as someone set a description on it in UC. The engine reads the remote
comment, plans an update, and every field of
UpdateSchemaisomitempty, sothe PATCH body came out empty. UC answers that with
400 INVALID_PARAMETER_VALUE / UpdateSchema Nothing to updateinstead of ano-op, failing the whole deploy with no way out from the CLI. Catalogs and
volumes fail the same way — verified against a real workspace.
Registered models and external locations carry a quieter version of the same bug:
their payloads always include other fields, so there is no 400 — instead removing a
comment from the configuration reports "1 changed", leaves the comment in place, and
reports the same pending change on every later deploy. No out-of-band edit needed.
The fix force-sends the fields the plan reports as clearing, matched against the
request type's own JSON names, so each PATCH carries exactly what the plan says
is changing. That also converges the wider version of the bug: any
omitemptyfield the config stops setting was previously dropped from the payload, and a
partial-update API reads an absent field as "leave unchanged", so the clear was
silently discarded and the next plan reported the same drift forever. Roughly 40
fields across the UC resources are affected, not just
comment.forceSendClearedFieldsdocuments the shape, including why full-replacementAPIs (jobs, pipelines, model serving) need none of it.
backend_defaults(the other option the issue suggests) is not right here: acomment set in Catalog Explorer is real drift, not a value the backend filled in,
and suppressing it would silently stop the bundle from managing
comment. Thattreatment is correct for
properties['unity.catalog.managed.*.defaults.*'],which already carries such a rule, because UC cannot clear
propertiesat all —an empty or null map is itself rejected as "nothing to update". Skipped changes
are excluded here, so a field classified under
backend_defaultsorignore_remote_changesis still left alone.Tests
New acceptance tests for schemas, catalogs and volumes (locally and on cloud), plus
registered models and external locations for the non-converging variant — external
locations local-only, since they need real storage credentials. The first commit adds the schema one and makes the fake workspace reject an empty
UC update payload the way UC does, so the local run fails exactly like the cloud
one; the later commits turn them green. The schema test also covers a non-string
field (
custom_max_retention_hours), which the narrower fix could not converge.The guard is real, not decoration: with the force-send stubbed out, the three
drift tests fail locally with the same
Nothing to updatethe backend returns.Each fix commit is preceded by a commit that records the broken behaviour in the
goldens, so the diff shows exactly what changed.
The rename paths are handled in #6360, stacked on this branch.
The fake workspace now applies whatever an update payload names, including zero
values, instead of special-casing
commentper resource. That incidentally stopsmergofrom clobbering the storedForceSendFields, sobrowse_onlysurvives anupdate the way UC returns it.
Fixes #6340
This pull request and its description were written by Isaac.