Fix "Nothing to update" when a UC comment is set outside the bundle - #6343
Draft
denik wants to merge 5 commits into
Draft
Fix "Nothing to update" when a UC comment is set outside the bundle#6343denik wants to merge 5 commits into
denik wants to merge 5 commits into
Conversation
…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
Collaborator
Integration test reportCommit: a2dddae
Top 3 slowest tests (at least 2 minutes):
|
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
denik
changed the title
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
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 have the same shape and fail the same way — verified
against a real workspace,
{}returnsUpdateCatalog Nothing to updateandUpdateVolume Nothing to update.Force-send
commentin all five update paths (catalogs and volumes each have arename variant) so the payload always carries a field and the comment is
actually cleared, which is what the plan already said it wanted to do.
backend_defaults(the other option the issue suggests) is not right here:a comment 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.That treatment 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".
commentcanbe cleared, so it should be.
Tests
New acceptance tests for schemas, catalogs and volumes, each running locally and
on cloud. 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 fake's rejection is a real guard, not decoration: with
forceSendCommentstubbed out to a no-op, all three tests fail locally with the same
Nothing to updateerror the backend returns.Terraform does not plan an update for this drift at all, so it is unaffected
(it also never converges — the out-of-band comment just stays).
Fixes #6340
This pull request and its description were written by Isaac.