From 8afba87a39e91e99a74976ff1e9dbad1528cfa03 Mon Sep 17 00:00:00 2001 From: Shreyas Goenka Date: Wed, 14 Oct 2026 16:07:47 +0000 Subject: [PATCH] direct: add a Bind plan action 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. Apply records it in state without an API write, taking the id from the plan entry because the resource is not in state yet; the next deploy applies any config differences. Nothing sets BindKey yet; DMS bind uses it in a follow-up. Co-authored-by: Isaac --- bundle/deployplan/action.go | 5 +++ bundle/deployplan/plan.go | 2 +- bundle/direct/apply.go | 10 +++++- bundle/direct/bind.go | 28 ++++++++++------ bundle/direct/bundle_plan.go | 56 ++++++++++++++++++++++--------- bundle/direct/bundle_plan_test.go | 13 +++++++ bundle/direct/pkg.go | 4 +++ 7 files changed, 89 insertions(+), 29 deletions(-) diff --git a/bundle/deployplan/action.go b/bundle/deployplan/action.go index e53ef0d84bd..07219fe8160 100644 --- a/bundle/deployplan/action.go +++ b/bundle/deployplan/action.go @@ -53,6 +53,11 @@ const ( Delete ActionType = "delete" ) +// Bind adopts an existing workspace resource: it records the resource in state without changing +// it, so the next deploy applies any config differences. Only the resource being bound gets it, +// so it is never compared by severity. +const Bind ActionType = "bind" + var actionOrder = map[ActionType]int{ Undefined: 0, Skip: 1, diff --git a/bundle/deployplan/plan.go b/bundle/deployplan/plan.go index 874479db43d..3e27ce59042 100644 --- a/bundle/deployplan/plan.go +++ b/bundle/deployplan/plan.go @@ -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..d507f253fe6 100644 --- a/bundle/direct/apply.go +++ b/bundle/direct/apply.go @@ -36,8 +36,16 @@ 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) + 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 9ac921b9c3c..ba48b28daf3 100644 --- a/bundle/direct/bind.go +++ b/bundle/direct/bind.go @@ -50,6 +50,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. // @@ -141,17 +156,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..65835c7d1ab 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,20 @@ 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 records the resource as is; entry.Changes shows what the next deploy will apply. + action = deployplan.Bind + // The id is not in state yet, so the plan entry carries it to apply. + entry.ID = resourceID + default: action = getMaxAction(entry.Changes) } @@ -984,7 +1004,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 { + 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 2d051dc21b9..3e7293d67f8 100644 --- a/bundle/direct/bundle_plan_test.go +++ b/bundle/direct/bundle_plan_test.go @@ -502,6 +502,19 @@ 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) { + plan := deployplan.NewPlanDirect() + plan.Plan["resources.jobs.foo"] = &deployplan.PlanEntry{ID: "123", Action: deployplan.Bind} + 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.