From d52fe57442429a13581d43737b2da88170645a3d Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 14 Oct 2026 14:44:38 +0000 Subject: [PATCH] direct: add Bind and BindAndUpdate plan actions CalculatePlan can now adopt an existing workspace resource: when DeploymentBundle.BindKey names a resource, it is read by BindID instead of its state entry, and planned as Bind (config matches) or BindAndUpdate (an in-place update applies the config). Changes that need a recreate or resize are rejected. Apply records a Bind without an API write and runs a normal update for BindAndUpdate, taking the id from the plan entry because the resource is not in state yet. Nothing sets BindKey yet; DMS bind uses it in a follow-up. Co-authored-by: Isaac --- bundle/deployplan/action.go | 8 ++++ bundle/deployplan/plan.go | 4 +- bundle/direct/apply.go | 12 +++++- bundle/direct/bind.go | 28 +++++++------ bundle/direct/bundle_plan.go | 66 +++++++++++++++++++++++-------- bundle/direct/bundle_plan_test.go | 17 ++++++++ bundle/direct/pkg.go | 4 ++ 7 files changed, 108 insertions(+), 31 deletions(-) diff --git a/bundle/deployplan/action.go b/bundle/deployplan/action.go index e53ef0d84bd..145eadd542c 100644 --- a/bundle/deployplan/action.go +++ b/bundle/deployplan/action.go @@ -53,6 +53,14 @@ const ( Delete ActionType = "delete" ) +// Bind and BindAndUpdate adopt an existing workspace resource: Bind records it unchanged and +// BindAndUpdate also applies the config. Only the resource being bound gets them, so they are +// never compared by severity. +const ( + Bind ActionType = "bind" + BindAndUpdate ActionType = "bind_and_update" +) + var actionOrder = map[ActionType]int{ Undefined: 0, Skip: 1, diff --git a/bundle/deployplan/plan.go b/bundle/deployplan/plan.go index 874479db43d..5575c0f97f9 100644 --- a/bundle/deployplan/plan.go +++ b/bundle/deployplan/plan.go @@ -54,7 +54,7 @@ func (p *Plan) CountActions() ActionCounts { switch entry.Action { case Create: c.Create++ - case Update, UpdateWithID, Resize: + case Update, UpdateWithID, Resize, BindAndUpdate: c.Change++ case Delete: // A state-only delete touches nothing in the backend and only drops the @@ -69,7 +69,7 @@ func (p *Plan) CountActions() ActionCounts { // A recreate counts as both a delete and a create. c.Delete++ c.Create++ - case Skip, Undefined: + case Skip, Undefined, Bind: c.Unchanged++ } } diff --git a/bundle/direct/apply.go b/bundle/direct/apply.go index 57367883c6c..486b044a124 100644 --- a/bundle/direct/apply.go +++ b/bundle/direct/apply.go @@ -36,8 +36,18 @@ func (d *DeploymentUnit) Destroy(ctx context.Context, db *dstate.DeploymentState func (d *DeploymentUnit) Deploy(ctx context.Context, db *dstate.DeploymentState, newState any, actionType deployplan.ActionType, planEntry *deployplan.PlanEntry) error { ctx = log.WithPrefix(ctx, "deploying "+d.ResourceKey) ctx = d.withResourceKey(ctx) - if actionType == deployplan.Create { + // A bound resource is not in state yet, so its id comes from the plan entry. + switch actionType { + case deployplan.Create: return d.Create(ctx, db, newState) + case deployplan.Bind: + copyRemoteEtag(d.ResourceKey, planEntry.RemoteState, newState) + log.Infof(ctx, "Bound %s id=%#v", d.ResourceKey, planEntry.ID) + return d.saveState(ctx, db, planEntry.ID, newState, d.DependsOn) + case deployplan.BindAndUpdate: + return d.Update(ctx, db, planEntry.ID, newState, planEntry) + default: + // The remaining actions act on a resource already in state. } oldID := db.GetResourceID(d.ResourceKey) diff --git a/bundle/direct/bind.go b/bundle/direct/bind.go index 517d641e763..693e7bb81bf 100644 --- a/bundle/direct/bind.go +++ b/bundle/direct/bind.go @@ -46,6 +46,21 @@ type BindResult struct { StatePath string } +// copyRemoteEtag copies the etag from remote state for resources that use etag-based drift +// detection (dashboards and genie spaces). The etag is not provided by the +// user; it comes from remote. If we don't store it in state, we won't +// detect remote drift correctly and the next plan shows a bogus update. +func copyRemoteEtag(resourceKey string, remoteState, newState any) { + if (strings.Contains(resourceKey, ".dashboards.") || strings.Contains(resourceKey, ".genie_spaces.")) && remoteState != nil { + etag, err := structaccess.Get(remoteState, structpath.NewStringKey(nil, "etag")) + if err == nil && etag != nil { + if etagStr, ok := etag.(string); ok && etagStr != "" { + _ = structaccess.Set(newState, structpath.NewStringKey(nil, "etag"), etagStr) + } + } + } +} + // Bind adds an existing workspace resource to a temporary state and calculates // if there will be any changes when deploying. // @@ -137,17 +152,8 @@ func (b *DeploymentBundle) Bind(ctx context.Context, client *databricks.Workspac dependsOn = entry.DependsOn } - // Copy etag from remote state for resources that use etag-based drift - // detection (dashboards and genie spaces). The etag is not provided by the - // user; it comes from remote. If we don't store it in state, we won't - // detect remote drift correctly and the next plan shows a bogus update. - if (strings.Contains(resourceKey, ".dashboards.") || strings.Contains(resourceKey, ".genie_spaces.")) && entry != nil && entry.RemoteState != nil { - etag, err := structaccess.Get(entry.RemoteState, structpath.NewStringKey(nil, "etag")) - if err == nil && etag != nil { - if etagStr, ok := etag.(string); ok && etagStr != "" { - _ = structaccess.Set(sv.Value, structpath.NewStringKey(nil, "etag"), etagStr) - } - } + if entry != nil { + copyRemoteEtag(resourceKey, entry.RemoteState, sv.Value) } // Compact hashed_fields fields so the persisted state stays small. Not needed for diff --git a/bundle/direct/bundle_plan.go b/bundle/direct/bundle_plan.go index 98da4f1376d..099d5c75614 100644 --- a/bundle/direct/bundle_plan.go +++ b/bundle/direct/bundle_plan.go @@ -269,21 +269,37 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks } dbentry, hasEntry := b.StateDB.GetResourceEntry(resourceKey) + // A bound resource is read by the id being bound; it has no state entry yet. + adopt := b.BindKey == resourceKey // Tolerate empty-ID entries from older partial-recreate failures // (apply.Recreate now deletes state on the way through, but pre-fix // state files may still carry a malformed entry). Treat as missing // and let the resource be re-created on this plan. - if !hasEntry || dbentry.ID == "" { + if !adopt && (!hasEntry || dbentry.ID == "") { entry.Action = deployplan.Create return true } - savedState, err := parseState(adapter.StateType(), dbentry.State) - if err != nil { - logdiag.LogError(ctx, fmt.Errorf("%s: interpreting state: %w", errorPrefix, err)) + sv, ok := b.StateCache.Load(resourceKey) + if !ok { + logdiag.LogError(ctx, fmt.Errorf("%s: internal error: no state cache entry found for %q", errorPrefix, resourceKey)) return false } + resourceID := dbentry.ID + var savedState any + if adopt { + resourceID = b.BindID + // No saved state, so diff the config against itself: only remote drift shows below. + savedState = sv.Value + } else { + savedState, err = parseState(adapter.StateType(), dbentry.State) + if err != nil { + logdiag.LogError(ctx, fmt.Errorf("%s: interpreting state: %w", errorPrefix, err)) + return false + } + } + // Compact the saved state so hashed_fields fields are hashes compactedSavedState, err := dresources.CompactState(adapter.ResourceConfig(), savedState) if err != nil { @@ -297,11 +313,6 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks // for integers: compare 0 with actual object ID. As long as real object IDs are never 0 we're good. // Once we add non-id fields or add per-field details to "bundle plan", we must read dynamic data and deal with references as first class citizen. // This means distinguishing between 0 that are actually object ids and 0 that are there because typed struct integer cannot contain ${...} string. - sv, ok := b.StateCache.Load(resourceKey) - if !ok { - logdiag.LogError(ctx, fmt.Errorf("%s: internal error: no state cache entry found for %q", errorPrefix, resourceKey)) - return false - } // Compact a copy for comparison only; sv.Value keeps the full contents, which // the deploy sends to the API. localState, err := dresources.CompactState(adapter.ResourceConfig(), sv.Value) @@ -316,13 +327,13 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks } remoteState, err := retry.With(ctx, retry.IsTransient, func() (any, error) { - return adapter.DoRead(ctx, dbentry.ID) + return adapter.DoRead(ctx, resourceID) }) if err != nil { if apierr.IsMissing(err) { remoteState = nil } else { - logdiag.LogError(ctx, fmt.Errorf("%s: reading id=%q: %w", errorPrefix, dbentry.ID, err)) + logdiag.LogError(ctx, fmt.Errorf("%s: reading id=%q: %w", errorPrefix, resourceID, err)) return false } } @@ -338,7 +349,7 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks if remoteState != nil { remoteStateComparable, err = adapter.RemapState(remoteState) if err != nil { - logdiag.LogError(ctx, fmt.Errorf("%s: interpreting remote state id=%q: %w", errorPrefix, dbentry.ID, err)) + logdiag.LogError(ctx, fmt.Errorf("%s: interpreting remote state id=%q: %w", errorPrefix, resourceID, err)) return false } @@ -346,7 +357,7 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks // is a hash on all three sides of the diff (saved, local, remote) remoteStateComparable, err = dresources.CompactState(adapter.ResourceConfig(), remoteStateComparable) if err != nil { - logdiag.LogError(ctx, fmt.Errorf("%s: compacting remote state id=%q: %w", errorPrefix, dbentry.ID, err)) + logdiag.LogError(ctx, fmt.Errorf("%s: compacting remote state id=%q: %w", errorPrefix, resourceID, err)) return false } @@ -369,11 +380,28 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks return false } - if remoteState == nil { + switch { + case remoteState == nil && adopt: + logdiag.LogError(ctx, fmt.Errorf("%s: cannot bind to id=%q: resource not found", errorPrefix, resourceID)) + return false + case remoteState == nil: // Even if local action is "recreate" which is higher than "create", we should still pick "create" here // because we know remote does not exist. action = deployplan.Create - } else { + case adopt: + // Bind adopts the resource as is or updates it in place; it does not recreate or resize. + switch maxAction := getMaxAction(entry.Changes); maxAction { + case deployplan.Skip: + action = deployplan.Bind + case deployplan.Update: + action = deployplan.BindAndUpdate + default: + logdiag.LogError(ctx, fmt.Errorf("%s: cannot bind id=%q: the config differs from the resource in a field that requires %s, which bind does not apply; align the config with the existing resource first", errorPrefix, resourceID, maxAction)) + return false + } + // The id is not in state yet, so the plan entry carries it to apply. + entry.ID = resourceID + default: action = getMaxAction(entry.Changes) } @@ -382,7 +410,7 @@ func (b *DeploymentBundle) CalculatePlan(ctx context.Context, client *databricks b.RemoteStateCache.Store(resourceKey, remoteState) // Validate that resources without DoUpdate don't have update actions - if action == deployplan.Update && !adapter.HasDoUpdate() { + if (action == deployplan.Update || action == deployplan.BindAndUpdate) && !adapter.HasDoUpdate() { logdiag.LogError(ctx, fmt.Errorf("%s: resource does not support update action but plan produced update", errorPrefix)) return false } @@ -984,7 +1012,11 @@ func (b *DeploymentBundle) LookupReferencePreDeploy(ctx context.Context, path *s if fieldPathS == "id" { if targetAction.KeepsID() { - id := b.StateDB.GetResourceID(targetResourceKey) + // A resource being bound is not in state yet; its plan entry carries the id. + id := targetEntry.ID + if targetAction != deployplan.Bind && targetAction != deployplan.BindAndUpdate { + id = b.StateDB.GetResourceID(targetResourceKey) + } if id == "" { return nil, errors.New("internal error: no db entry") } diff --git a/bundle/direct/bundle_plan_test.go b/bundle/direct/bundle_plan_test.go index 025ef91df6e..51344a43b56 100644 --- a/bundle/direct/bundle_plan_test.go +++ b/bundle/direct/bundle_plan_test.go @@ -502,6 +502,23 @@ func TestLookupReferencePreDeploy_FinishedJobRun(t *testing.T) { assert.Equal(t, jobs.RunResultStateSuccess, value) } +// A resource being bound is not in state yet, so a reference to its id (from its own +// permissions, say) resolves from the plan entry. +func TestLookupReferencePreDeploy_BoundResourceID(t *testing.T) { + for _, action := range []deployplan.ActionType{deployplan.Bind, deployplan.BindAndUpdate} { + t.Run(string(action), func(t *testing.T) { + plan := deployplan.NewPlanDirect() + plan.Plan["resources.jobs.foo"] = &deployplan.PlanEntry{ID: "123", Action: action} + b := &DeploymentBundle{Plan: plan} + + value, err := b.LookupReferencePreDeploy(t.Context(), structpath.MustParsePath("resources.jobs.foo.id")) + + require.NoError(t, err) + assert.Equal(t, "123", value) + }) + } +} + // jobRunResultStateAction classifies the result_state drift of a run in the given // state. Old == New because a deploy records the outcome it asked for, and only // the remote says whether the run reached it. diff --git a/bundle/direct/pkg.go b/bundle/direct/pkg.go index ea7ae1e51b6..1b9b4f16fd9 100644 --- a/bundle/direct/pkg.go +++ b/bundle/direct/pkg.go @@ -50,6 +50,10 @@ type DeploymentBundle struct { Plan *deployplan.Plan RemoteStateCache sync.Map StateCache structvar.Cache + + // BindKey and BindID name the resource CalculatePlan adopts for `bundle deployment bind`. + BindKey string + BindID string } // SetRemoteState updates the remote state with type validation and marks as fresh.