direct: fix plan JSON round-trip for RemoteState typed fields#5671
Open
denik wants to merge 10 commits into
Open
direct: fix plan JSON round-trip for RemoteState typed fields#5671denik wants to merge 10 commits into
denik wants to merge 10 commits into
Conversation
Contributor
Approval status: pending
|
Collaborator
Integration test reportCommit: 9402fa7
|
denik
force-pushed
the
denik/serialized-plan-number
branch
from
345ff9c to
467acab
Compare
When loading a plan from JSON, PlanEntry.RemoteState (typed any) is decoded
as map[string]interface{} rather than *GrantsState. The type assertion in
removedGrantPrincipals then failed silently, so principals removed from config
were never revoked on deploy --plan.
Fix by re-serializing the map and unmarshaling into *GrantsState when the
direct type assertion fails.
Co-authored-by: Isaac
Co-authored-by: Isaac
…ipal Co-authored-by: Isaac
LoadPlanFromFile decoded any-typed fields (RemoteState, ChangeDesc.Old/New/Remote)
as map[string]interface{}, causing type assertions in resource adapters to fail
(e.g. grants.removedGrantPrincipals did entry.RemoteState.(*GrantsState)).
Fix at the deserialization layer instead of in each adapter:
- Add UseNumber() to LoadPlanFromFile so large integers stay exact through
the re-hydration step rather than losing precision as float64.
- In InitForApply, re-hydrate each entry's RemoteState from the generic map
back into the correct typed struct using the adapter's StateType(), mirroring
the existing NewState re-hydration that was already there.
Resource implementations are now unaware of whether a plan came from a file
or was computed in memory.
Co-authored-by: Isaac
Co-authored-by: Isaac
ml.ModelDatabricks embeds a custom MarshalJSON that shadows outer struct fields, causing model_id to be dropped when serializing MlflowModelRemote to the plan JSON. Add MarshalJSON/UnmarshalJSON (using the SDK marshal package, same as AppRemote) so model_id survives the plan file round-trip. Also fix the RemoteType() vs StateType() comment/code in InitForApply. Co-authored-by: Isaac
Tests cover the bug where RemoteState loaded from a plan JSON file loses
type information (deserialization to map[string]interface{}), which caused
incorrect lifecycle decisions for apps/clusters and missing model_id for
model permissions.
Also fix libs/testserver/clusters.go to preserve runtime-only fields (State,
ClusterId) on ClustersEdit, which the cluster readplan test depends on.
Co-authored-by: Isaac
…wModelRemote Rebase introduced duplicate method declarations; keep only the intended pair. Co-authored-by: Isaac
Also update cluster plan snapshot to include state/lifecycle fields now preserved by the testserver on cluster edit. Co-authored-by: Isaac
denik
force-pushed
the
denik/serialized-plan-number
branch
from
467acab to
af3adc7
Compare
yamlfmt requires single space before inline comments. 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.
Summary
bundle deploy --plan),PlanEntry.RemoteStatewas decoded asmap[string]interface{}instead of the typed struct, causing type assertions in resource adapters to silently fail.removedGrantPrincipalsgot an untyped map → principal revocation was skipped on the second deploy.MlflowModelRemoteembedsml.ModelDatabrickswhich has a customMarshalJSONthat shadowed the outermodel_idfield — it was dropped from the plan JSON entirely. Permissions were then applied to an empty resource ID.remoteIsStarted/remoteClusterIsRunninggotnillifecycle/state → incorrect lifecycle decisions when deploying from plan.Fixes:
InitForApplyre-hydrates eachRemoteStateentry by round-tripping throughjson.Marshal+json.Unmarshalintoreflect.New(adapter.RemoteType().Elem()).MlflowModelRemotegets customMarshalJSON/UnmarshalJSON(using the SDKmarshalpackage, same pattern asAppRemote) somodel_idis preserved.decoder.UseNumber()inLoadPlanFromFileprevents large int64 values from degrading to float64 through the re-hydration round-trip.Test plan
acceptance/bundle/deploy/readplan/grants-remove-principal: deploys with extra grant principal, saves plan, removes principal, deploys from plan — verifies revocation happens.acceptance/bundle/resources/apps/readplan-lifecycle: verifies no spurious app start when deploying description change from plan.acceptance/bundle/resources/clusters/readplan-lifecycle: verifies no spurious cluster start when deploying tag change from plan.acceptance/bundle/resources/models/readplan-permissions: verifies permission update (manager removed) uses correct numeric model ID when deploying from plan.All tests run with
READPLAN=["", "1"]to cover both inline and file-loaded plan paths.This pull request and its description were written by Isaac.