Uh oh!
There was an error while loading. Please reload this page.
Fix postgres_role role_id serialization; add no_drift readplan invariant - #5672
Merged
Conversation
…readplan invariant The embedded postgres.RoleRoleSpec has its own MarshalJSON/UnmarshalJSON, which were promoted to PostgresRoleState and silently dropped role_id and parent when the plan was serialized to JSON. Deploying from such a plan (`deploy --plan`) created the role with an empty role_id, the server generated its own, and the next plan reported phantom "recreate" drift on the immutable role_id. Add explicit (Un)MarshalJSON on PostgresRoleState so all fields round-trip. Extend the no_drift invariant test with READPLAN variants so it exercises `deploy --plan` for every resource type, and add a focused readplan/postgres_role acceptance test that pins the serialized plan and create request. Co-authored-by: Isaac
Co-authored-by: Isaac
Collaborator
Integration test reportCommit: 4eafd92
22 interesting tests: 13 SKIP, 7 KNOWN, 2 flaky
Top 24 slowest tests (at least 2 minutes):
|
…tion The serialized plan state now carries role_id and parent, so the create plan records them under new_state.value and the no_change/update/restore plans no longer emit phantom remote_already_set change entries with old="". Co-authored-by: Isaac
pietern
approved these changes
Jun 22, 2026
pietern
left a comment
Contributor
There was a problem hiding this comment.
Great find!
Any ideas how we can avoid this class of bugs entirely?
The full three-resource plan is fed to `deploy --plan` via an ignored plan.json; the committed golden keeps just resources.postgres_roles.role, which is the entry whose new_state must carry role_id and parent. Co-authored-by: Isaac
denik
enabled auto-merge
June 22, 2026 10:51
Uh oh!
There was an error while loading. Please reload this page.
denik
commented
Jun 22, 2026
ContributorAuthor
deco-sdk-taggingBot
added a commit
that referenced
this pull request
Jun 24, 2026
## Release v1.5.0 ### CLI * `workspace export-dir` no longer aborts when a workspace object's name is not a legal local filename (e.g. a notebook named `New Notebook 2026-05-04 13:54:24` whose `:` is illegal on Windows). Such files are now exported under a sanitized name with a warning and the export completes ([#5171](#5171)). * `ssh connect` now opens an interactive `bash` login shell by default instead of the compute image's default `/bin/sh`, falling back gracefully when `bash` is unavailable. Passing an explicit remote command (`-- <cmd>`) is unaffected ([#5687](#5687)). * `ssh connect` interactive sessions now start in the user's workspace home folder (`/Workspace/Users/<email>`) instead of the OS home directory, falling back to the OS home when that folder is unavailable ([#5688](#5688)). ### Bundles * Add documentation for the common bundle resource fields `permissions`, `lifecycle`, and `grants` in the JSON schema, so they surface in editor completions and the docs. * `bundle run` now prints the modern job run URL (`/jobs/<id>/runs/<id>`) so that non-admin users permitted to view the run are taken to the run instead of the workspace homepage. * References to a registered model's `registered_model_id` now resolve under the direct engine, matching Terraform behavior ([#5621](#5621)). * Fix missing field descriptions in the bundle JSON schema for fields whose upstream API docs arrived after the field was first annotated (e.g. `vector_search_endpoints.*.target_qps`); stale placeholder markers no longer hide them ([#5588](#5588)). * Fix `bundle deploy --plan` dropping a `postgres_role`'s `role_id`, which caused the role to be recreated on the next deploy ([#5672](#5672)). * direct: Fix spurious cluster recreate when `apply_policy_default_values: true` is set ([#5693](#5693)). * direct: New 'deployment migrate' implementation that parses terraform state instead of fetching resources state from the backend ([#5399](#5399)). ### Dependency updates * Bump `github.com/databricks/databricks-sdk-go` from v0.141.0 to v0.147.0 ([#5636](#5636)). * Bump Terraform provider from v1.117.0 to v1.118.0 ([#5637](#5637)).
Chambras pushed a commit
to Chambras/newDatabrickscli
that referenced
this pull request
Jul 6, 2026
…atabricks#5681) Direct-engine resources persist their StateType to the state file and emit RemoteType in the plan's `remote_state` field, both via `encoding/json`. A wrapper that embeds an SDK type with its own `MarshalJSON` but omits its own silently drops the wrapper's extra fields. Nothing tested this. The same bug class was fixed manually in databricks#5672 (`role_id`); these tests turn it into a guard across every resource. Adds round-trip tests and fixes two resources that had this bug: - `VectorSearchEndpointRemote` dropped `endpoint_uuid` - `MlflowModelRemote` dropped `model_id` Both fields carry the numeric ID the permissions API needs. Tests: - `TestRoundtripFixtureStateType` — StateType round-trip on a realistic per-resource fixture - `TestRoundtripAllFieldsStateType` / `TestRoundtripAllFieldsRemoteType` — synthetic all-fields round-trip, so a dropped field is always observable regardless of fixture coverage - plus a concrete-value RemoteType check in the existing `testCRUD` InputType is intentionally not covered in this PR - many issues found but it is never serialized via `encoding/json` (the bundle config layer uses dyn). This pull request and its description were written 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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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
postgres_rolestoresrole_idandparentas top-level fields onPostgresRoleState, alongside the embeddedpostgres.RoleRoleSpec. That spec defines its ownMarshalJSON/UnmarshalJSON, which were promoted ontoPostgresRoleStateand (de)serialize only the spec fields — so serializing the state silently dropsrole_idandparent.This only bites
bundle deploy --plan.bundle plan -o jsonserializes each resource's state into the plan file'snew_statewith the lossy marshaler, so the file already lacksrole_id. On deploy, thatnew_stateis deserialized back into aPostgresRoleStateand used as the create payload, soCreateRoleis called with an emptyrole_id; the server generates its own, and the next plan sees the immutablerole_iddiverge from config and schedules arecreate.A plain
bundle deploynever uses serialized state to build the create request — it applies the typedPostgresRoleStatefromPrepareStatedirectly, sorole_idreaches the server. It does write the same lossy state file, but that copy isn't used for the create, and a later plan reconciles the immutablerole_idagainst remote (which holds the configured value), so the drift is masked.What
MarshalJSON/UnmarshalJSONtoPostgresRoleStateso every field survives serialization.no_driftinvariant test withREADPLANvariants, exercisingdeploy --planacross every resource type.readplan/postgres_roleacceptance test that commits the serialized plan and the create request, guarding the exact regression.This pull request and its description were written by Isaac.