Direct-engine reconcile classification fixes#5816
Merged
Merged
Conversation
… fields Several fields returned (or intentionally not returned) by UC/serving GET responses were not classified, so `bundle plan` reported a perpetual Update after deploy and could hard-fail with "Nothing to update" on redeploy. - catalogs: skip UC-injected `unity.catalog.managed.*` properties as backend defaults (mirrors the existing schemas handling). - registered_models: treat backend-computed `browse_only` as output-only. - model_serving_endpoints: ignore remote changes for `burst_scaling_enabled` and the write-only external-model `*_plaintext` secrets, none of which are echoed on GET. The testserver is extended to reproduce each drift (scoped catalog/model names, and stripping serving secrets on read) and new acceptance tests under each resource's `drift/` dir lock in the no-op plan and redeploy behavior.
radakam
force-pushed
the
fix/direct-reconcile-classification
branch
from
c01de9a to
f4ed131
Compare
Collaborator
Integration test reportCommit: 94729f7
8 interesting tests: 4 RECOVERED, 4 SKIP
Top 10 slowest tests (at least 2 minutes):
|
radakam
commented
Jul 3, 2026
| reason: output_only | ||
| - field: updated_by | ||
| reason: output_only | ||
| # Backend-computed; the user never sets it. Not annotated output_only in the spec. |
Contributor
Author
There was a problem hiding this comment.
We should probably annotate browse_only as output-only (or drop it from the create request) in the universe OpenAPI spec so the regenerated SDK stops surfacing it as a writable field.
| # Accepted on write but not returned by GET. | ||
| - field: config.served_entities[*].burst_scaling_enabled | ||
| reason: input_only | ||
| # Write-only secrets: the backend stores them and returns the reference field, not the plaintext. |
Contributor
There was a problem hiding this comment.
can we wild-card match these? there will be more as more models are added
Contributor
Author
There was a problem hiding this comment.
I'm not sure about that, wildcards can't match *_plaintext, and broader wildcards would also ignore legitimate GET fields. It might be better to mark these fields as write-only in the OpenAPI spec so they're generated automatically, like browse_only, what do you think?
Contributor
There was a problem hiding this comment.
on second look there's quite a few different patterns here (I was hoping it'd just be
(?<model>)_config.\1_api_key_plaintext
but that won't work and won't be fun to implement/maintain)
Contributor
There was a problem hiding this comment.
would be great if you can fix it upstream 🙌
denik
reviewed
Jul 3, 2026
| resources: | ||
| catalogs: | ||
| catalog1: | ||
| name: catalog_managed_defaults |
Contributor
There was a problem hiding this comment.
We have acceptance/bundle/invariant/configs/catalog.yml.tmpl why did not it catch this?
Is it because fuzz testing was against dogfood which adds these fields but our test env does not?
Contributor
Author
There was a problem hiding this comment.
Exactly -- the testserver wasn't populating these fields (dogfood does), so there was no drift to catch. I also added that to the testserver so it's now reproducible.
janniklasrose
approved these changes
Jul 6, 2026
| # Accepted on write but not returned by GET. | ||
| - field: config.served_entities[*].burst_scaling_enabled | ||
| reason: input_only | ||
| # Write-only secrets: the backend stores them and returns the reference field, not the plaintext. |
Contributor
There was a problem hiding this comment.
on second look there's quite a few different patterns here (I was hoping it'd just be
(?<model>)_config.\1_api_key_plaintext
but that won't work and won't be fun to implement/maintain)
| # Accepted on write but not returned by GET. | ||
| - field: config.served_entities[*].burst_scaling_enabled | ||
| reason: input_only | ||
| # Write-only secrets: the backend stores them and returns the reference field, not the plaintext. |
Contributor
There was a problem hiding this comment.
would be great if you can fix it upstream 🙌
Collaborator
Integration test reportCommit: f5640af
23 interesting tests: 12 flaky, 4 FAIL, 3 RECOVERED, 2 KNOWN, 2 SKIP
Top 50 slowest tests (at least 2 minutes):
|
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
4 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
Fuzzing the direct engine found fields that UC/serving GET responses either compute or never echo back. They weren't classified, so
bundle planshowed a permanentupdateafter deploy and redeploy could fail with400 ... Nothing to update.Changes
bundle/direct/dresources/resources.yml:properties['unity.catalog.managed.*']asbackend_defaults(mirrors existing schemas fix).browse_only→ignore_remote_changes(output-only, backend-computed, not in update surface).config.served_entities[*].burst_scaling_enabled+ all external-model*_plaintextsecrets →ignore_remote_changes(input-only, accepted on write but never returned on GET).These aren't annotated in the OpenAPI spec, so they're declared manually (absent from
resources.generated.yml).libs/testserver/:catalogs.go/registered_models.go: inject the computed values, scoped to dedicated names (catalog_managed_defaults,model_browse_only).serving_endpoints.go: strip*_plaintextsecrets on read (burst_scaling_enabledalready wasn't round-tripped).Tests
New direct-engine acceptance tests asserting no-op plan + no update call on redeploy:
acceptance/bundle/resources/catalogs/drift/managed_propertiesacceptance/bundle/resources/registered_models/drift/browse_onlyacceptance/bundle/resources/model_serving_endpoints/drift/write_only