direct: refactor deploy/plan interaction (#3350)

## Changes
"bundle deploy" no longer calculates and classifies the diff, it relies
on action types calculated during "bundle plan". Note, this will need to
be further developed once we handle ${resources} references - we might
want to recalculate the diff in some cases.

We no longer have UpdateUpdatesID setting on the resource. Instead,
resources can implement optional method DoUpdateWithID that can return
an updated id as part of the update. Currently, only volumes uses that.

The original DoUpdate method no longer allows to return ID. By
convention, if possible, it also checks that ID is not changed by the
backend.

There is new ActionType: "update_with_id". This is what triggers
DoUpdateWithID() call. However, when stringified, this is presented as
"update" so that it's compatible with terraform output in "bundle plan".

## Why
The main motivation is to have information at plan-time on whether we're
going to update ID or not. If not, we can resolve ${resources.*.*.id}
right away and have more parallelism at deploy time.

Additionally, this simplifies usual case DoUpdate() that resources need
to implement.

## Tests
Existing tests. The new action type is tested by
#3349

Author: denik

bundle/deployplan/plan.go

@@ -36,21 +36,34 @@ func (a Action) IsInplaceSupported() bool {
 	return false
 }
 
-// These enum values correspond to action types defined in the tfjson library.
+// These enum values are superset to action types defined in the tfjson library.
 // "recreate" maps to the tfjson.Actions.Replace() function.
-// "update" maps to tfjson.Actions.Update() and so on. source:
+// "update" and "update_with_id" maps to tfjson.Actions.Update() and so on. source:
 // https://github.com/hashicorp/terraform-json/blob/0104004301ca8e7046d089cdc2e2db2179d225be/action.go#L14
 type ActionType string
 
 const (
-	ActionTypeUnset    ActionType = ""
-	ActionTypeNoop     ActionType = "noop"
-	ActionTypeCreate   ActionType = "create"
-	ActionTypeDelete   ActionType = "delete"
-	ActionTypeUpdate   ActionType = "update"
-	ActionTypeRecreate ActionType = "recreate"
+	ActionTypeUnset        ActionType = ""
+	ActionTypeNoop         ActionType = "noop"
+	ActionTypeCreate       ActionType = "create"
+	ActionTypeDelete       ActionType = "delete"
+	ActionTypeUpdate       ActionType = "update"
+	ActionTypeUpdateWithID ActionType = "update_with_id"
+	ActionTypeRecreate     ActionType = "recreate"
 )
 
+var ShortName = map[ActionType]ActionType{
+	ActionTypeUpdateWithID: ActionTypeUpdate,
+}
+
+func (a ActionType) String() string {
+	shortAction := ShortName[a]
+	if shortAction != "" {
+		return string(shortAction)
+	}
+	return string(a)
+}
+
 // Filter returns actions that match the specified action type
 func Filter(changes []Action, actionType ActionType) []Action {
 	var result []Action

bundle/terranova/apply.go

@@ -15,7 +15,6 @@ import (
 	"github.com/databricks/cli/libs/diag"
 	"github.com/databricks/cli/libs/log"
 	"github.com/databricks/cli/libs/logdiag"
-	"github.com/databricks/cli/libs/structdiff"
 	"github.com/databricks/databricks-sdk-go"
 )
 
@@ -143,46 +142,41 @@ func (d *Deployer) destroy(ctx context.Context, inputConfig any) error {
 }
 
 func (d *Deployer) deploy(ctx context.Context, inputConfig any, actionType deployplan.ActionType) error {
-	entry, hasEntry := d.db.GetResourceEntry(d.group, d.resourceName)
-
-	resource, cfgType, err := New(d.client, d.group, d.resourceName, inputConfig)
+	resource, _, err := New(d.client, d.group, d.resourceName, inputConfig)
 	if err != nil {
 		return err
 	}
 
 	config := resource.Config()
 
-	if !hasEntry {
+	if actionType == deployplan.ActionTypeCreate {
 		return d.Create(ctx, resource, config)
 	}
 
+	entry, hasEntry := d.db.GetResourceEntry(d.group, d.resourceName)
+	if !hasEntry {
+		return errors.New("state entry not found")
+	}
+
 	oldID := entry.ID
 	if oldID == "" {
 		return errors.New("invalid state: empty id")
 	}
 
-	savedState, err := typeConvert(cfgType, entry.State)
-	if err != nil {
-		return fmt.Errorf("interpreting state: %w", err)
-	}
-
-	localDiffType, err := calcDiff(d.settings, resource, savedState, config)
-	if err != nil {
-		return fmt.Errorf("comparing state and config: %w", err)
-	}
-
-	if localDiffType == deployplan.ActionTypeRecreate {
+	switch actionType {
+	case deployplan.ActionTypeRecreate:
 		return d.Recreate(ctx, resource, oldID, config)
-	}
-
-	if localDiffType == deployplan.ActionTypeUpdate {
+	case deployplan.ActionTypeUpdate:
 		return d.Update(ctx, resource, oldID, config)
+	case deployplan.ActionTypeUpdateWithID:
+		updater, hasUpdater := resource.(IResourceUpdatesID)
+		if !hasUpdater {
+			return errors.New("internal error: plan is update_with_id but resource does not implement UpdateWithID")
+		}
+		return d.UpdateWithID(ctx, resource, updater, oldID, config)
+	default:
+		return fmt.Errorf("internal error: unexpected plan: %#v", actionType)
 	}
-
-	// localDiffType is either None or Partial: we should proceed to fetching remote state and calculate local+remote diff
-
-	log.Debugf(ctx, "Unchanged %s.%s id=%#v", d.group, d.resourceName, oldID)
-	return nil
 }
 
 func (d *Deployer) Create(ctx context.Context, resource IResource, config any) error {
@@ -238,8 +232,27 @@ func (d *Deployer) Recreate(ctx context.Context, resource IResource, oldID strin
 	return nil
 }
 
-func (d *Deployer) Update(ctx context.Context, resource IResource, oldID string, config any) error {
-	newID, err := resource.DoUpdate(ctx, oldID)
+func (d *Deployer) Update(ctx context.Context, resource IResource, id string, config any) error {
+	err := resource.DoUpdate(ctx, id)
+	if err != nil {
+		return fmt.Errorf("updating id=%s: %w", id, err)
+	}
+
+	err = d.db.SaveState(d.group, d.resourceName, id, config)
+	if err != nil {
+		return fmt.Errorf("saving state id=%s: %w", id, err)
+	}
+
+	err = resource.WaitAfterUpdate(ctx)
+	if err != nil {
+		return fmt.Errorf("waiting after updating id=%s: %w", id, err)
+	}
+
+	return nil
+}
+
+func (d *Deployer) UpdateWithID(ctx context.Context, resource IResource, updater IResourceUpdatesID, oldID string, config any) error {
+	newID, err := updater.DoUpdateWithID(ctx, oldID)
 	if err != nil {
 		return fmt.Errorf("updating id=%s: %w", oldID, err)
 	}
@@ -255,14 +268,11 @@ func (d *Deployer) Update(ctx context.Context, resource IResource, oldID string,
 		return fmt.Errorf("saving state id=%s: %w", oldID, err)
 	}
 
-	if oldID != newID && !d.settings.UpdateUpdatesID {
-		return fmt.Errorf("internal error, unexpected change of ID from %#v to %#v", oldID, newID)
-	}
-
 	err = resource.WaitAfterUpdate(ctx)
 	if err != nil {
 		return fmt.Errorf("waiting after updating id=%s: %w", newID, err)
 	}
+
 	return nil
 }
 
@@ -296,22 +306,3 @@ func typeConvert(destType reflect.Type, src any) (any, error) {
 
 	return reflect.ValueOf(destPtr).Elem().Interface(), nil
 }
-
-func calcDiff(settings ResourceSettings, resource IResource, savedState, config any) (deployplan.ActionType, error) {
-	localDiff, err := structdiff.GetStructDiff(savedState, config)
-	if err != nil {
-		return "", err
-	}
-
-	if settings.MustRecreate(localDiff) {
-		return deployplan.ActionTypeRecreate, nil
-	} else if len(localDiff) > 0 {
-		result := resource.ClassifyChanges(localDiff)
-		if result == deployplan.ActionTypeRecreate && !settings.RecreateAllowed {
-			return "", errors.New("internal error: ClassifyChanges returned 'recreate' unexpectedly")
-		}
-		return result, nil
-	}
-
-	return deployplan.ActionTypeNoop, nil
-}

bundle/terranova/plan.go

@@ -9,6 +9,7 @@ import (
 	"github.com/databricks/cli/bundle/deployplan"
 	"github.com/databricks/cli/bundle/terranova/tnstate"
 	"github.com/databricks/cli/libs/dyn"
+	"github.com/databricks/cli/libs/structdiff"
 	"github.com/databricks/cli/libs/utils"
 	"github.com/databricks/databricks-sdk-go"
 )
@@ -22,6 +23,7 @@ type Planner struct {
 }
 
 func (d *Planner) Plan(ctx context.Context, inputConfig any) (deployplan.ActionType, error) {
+	// TODO: wrap errors with prefix "planning <group>.<name>:"
 	entry, hasEntry := d.db.GetResourceEntry(d.group, d.resourceName)
 
 	resource, cfgType, err := New(d.client, d.group, d.resourceName, inputConfig)
@@ -158,3 +160,37 @@ func CalculateDestroyActions(ctx context.Context, b *bundle.Bundle) ([]deploypla
 
 	return actions, nil
 }
+
+func calcDiff(settings ResourceSettings, resource IResource, savedState, config any) (deployplan.ActionType, error) {
+	localDiff, err := structdiff.GetStructDiff(savedState, config)
+	if err != nil {
+		return "", err
+	}
+
+	if len(localDiff) == 0 {
+		return deployplan.ActionTypeNoop, nil
+	}
+
+	if settings.MustRecreate(localDiff) {
+		return deployplan.ActionTypeRecreate, nil
+	}
+
+	customClassify, hasCustomClassify := resource.(IResourceCustomClassify)
+
+	if hasCustomClassify {
+		_, hasUpdateWithID := resource.(IResourceUpdatesID)
+
+		result := customClassify.ClassifyChanges(localDiff)
+		if result == deployplan.ActionTypeRecreate && !settings.RecreateAllowed {
+			return "", errors.New("internal error: unexpected plan='recreate'")
+		}
+
+		if result == deployplan.ActionTypeUpdateWithID && !hasUpdateWithID {
+			return "", errors.New("internal error: unexpected plan='update_with_id'")
+		}
+
+		return result, nil
+	}
+
+	return deployplan.ActionTypeUpdate, nil
+}

bundle/terranova/resource.go

@@ -26,12 +26,6 @@ type ResourceSettings struct {
 	// Function to delete a resource of this type
 	DeleteFN DeleteResourceFN
 
-	// true if Update() method can return a different ID than that was passed in
-	// If ID changes during Update and UpdateUpdatesID is false, deployment of that resource will fail with internal error.
-	// This allows to make assumptions about references stability (${resources.jobs.foo.id}) when we see that
-	// operation is going to be "update" & ID is guarantee not to change.
-	UpdateUpdatesID bool
-
 	// true if ClassifyChanges() method can return a different ActionTypeRecreate
 	// If RecreateAllowed is false and RecreateFields is empty, the resource id is stable.
 	RecreateAllowed bool
@@ -41,9 +35,6 @@ type ResourceSettings struct {
 	// Fields are in structdiff.Change.String() format.
 	// Limitation: patterns like hello.*.world and hello[*].world are not supported
 	RecreateFields map[string]struct{}
-
-	// If resource does not set RecreateFields, RecreateAllowed, UpdateUpdatesID then
-	// it's ${resources.<group>.<name>.id} will considered stable for the purposes of concurrent deployment.
 }
 
 func (s *ResourceSettings) MustRecreate(changes []structdiff.Change) bool {
@@ -94,10 +85,9 @@ var SupportedResources = map[string]ResourceSettings{
 		),
 	},
 	"volumes": {
-		New:             reflect.ValueOf(tnresources.NewResourceVolume),
-		ConfigType:      TypeOfConfig(&tnresources.ResourceVolume{}),
-		DeleteFN:        tnresources.DeleteVolume,
-		UpdateUpdatesID: true,
+		New:        reflect.ValueOf(tnresources.NewResourceVolume),
+		ConfigType: TypeOfConfig(&tnresources.ResourceVolume{}),
+		DeleteFN:   tnresources.DeleteVolume,
 		// TF: https://github.com/databricks/terraform-provider-databricks/blob/f5fce0f/catalog/resource_volume.go#L19
 		RecreateFields: mkMap(
 			".catalog_name",
@@ -124,17 +114,26 @@ type IResource interface {
 	// Create the resource. Returns id of the resource.
 	DoCreate(ctx context.Context) (string, error)
 
-	// Update the resource. Returns id of the resource.
-	// Usually returns the same id as oldId but can also return a different one (e.g. schemas and volumes when certain fields are changed)
-	// Note, SupportedResources[group].UpdateUpdatesID must be true for this group if ID can be changed. Otherwise function must return the same ID.
-	DoUpdate(ctx context.Context, oldID string) (string, error)
+	// Update the resource. ID must not change.
+	DoUpdate(ctx context.Context, id string) error
 
 	WaitAfterCreate(ctx context.Context) error
 	WaitAfterUpdate(ctx context.Context) error
+}
 
+// Optional method for non-default change classification.
+// Default is to consider any change "an update" (RecreateFields handled separately).
+type IResourceCustomClassify interface {
 	ClassifyChanges(changes []structdiff.Change) deployplan.ActionType
 }
 
+// Optional method for resources that may update ID as part of update operation.
+type IResourceUpdatesID interface {
+	// Update the resource. Returns new id of the resource, which may be different from the old one.
+	// This will only be called if actiontype is ActionTypeUpdateWithID, so ClassifyChanges must be implemented as well.
+	DoUpdateWithID(ctx context.Context, oldID string) (string, error)
+}
+
 // invokeConstructor converts cfg to the parameter type expected by ctor and
 // executes the call, returning the IResource instance or error.
 func invokeConstructor(ctor reflect.Value, client *databricks.WorkspaceClient, cfg any) (IResource, error) {

bundle/terranova/tnresources/app.go

@@ -4,8 +4,7 @@ import (
 	"context"
 
 	"github.com/databricks/cli/bundle/config/resources"
-	"github.com/databricks/cli/bundle/deployplan"
-	"github.com/databricks/cli/libs/structdiff"
+	"github.com/databricks/cli/libs/log"
 	"github.com/databricks/databricks-sdk-go"
 	"github.com/databricks/databricks-sdk-go/service/apps"
 )
@@ -41,17 +40,21 @@ func (r *ResourceApp) DoCreate(ctx context.Context) (string, error) {
 	return waiter.Response.Name, nil
 }
 
-func (r *ResourceApp) DoUpdate(ctx context.Context, id string) (string, error) {
+func (r *ResourceApp) DoUpdate(ctx context.Context, id string) error {
 	request := apps.UpdateAppRequest{
 		App:  r.config,
 		Name: id,
 	}
 	response, err := r.client.Apps.Update(ctx, request)
 	if err != nil {
-		return "", SDKError{Method: "Apps.Update", Err: err}
+		return SDKError{Method: "Apps.Update", Err: err}
 	}
 
-	return response.Name, nil
+	if response.Name != id {
+		log.Warnf(ctx, "apps: response contains unexpected name=%#v (expected %#v)", response.Name, id)
+	}
+
+	return nil
 }
 
 func DeleteApp(ctx context.Context, client *databricks.WorkspaceClient, id string) error {
@@ -68,8 +71,3 @@ func (r *ResourceApp) WaitAfterUpdate(ctx context.Context) error {
 	// Intentional no-op
 	return nil
 }
-
-func (r *ResourceApp) ClassifyChanges(changes []structdiff.Change) deployplan.ActionType {
-	// TODO: changing name is recreation
-	return deployplan.ActionTypeUpdate
-}

bundle/terranova/tnresources/job.go

@@ -5,8 +5,6 @@ import (
 	"strconv"
 
 	"github.com/databricks/cli/bundle/config/resources"
-	"github.com/databricks/cli/bundle/deployplan"
-	"github.com/databricks/cli/libs/structdiff"
 	"github.com/databricks/databricks-sdk-go"
 	"github.com/databricks/databricks-sdk-go/service/jobs"
 )
@@ -40,16 +38,16 @@ func (r *ResourceJob) DoCreate(ctx context.Context) (string, error) {
 	return strconv.FormatInt(response.JobId, 10), nil
 }
 
-func (r *ResourceJob) DoUpdate(ctx context.Context, id string) (string, error) {
+func (r *ResourceJob) DoUpdate(ctx context.Context, id string) error {
 	request, err := makeResetJob(r.config, id)
 	if err != nil {
-		return "", err
+		return err
 	}
 	err = r.client.Jobs.Reset(ctx, request)
 	if err != nil {
-		return "", SDKError{Method: "Jobs.Reset", Err: err}
+		return SDKError{Method: "Jobs.Reset", Err: err}
 	}
-	return id, nil
+	return nil
 }
 
 func DeleteJob(ctx context.Context, client *databricks.WorkspaceClient, id string) error {
@@ -74,10 +72,6 @@ func (r *ResourceJob) WaitAfterUpdate(ctx context.Context) error {
 	return nil
 }
 
-func (r *ResourceJob) ClassifyChanges(changes []structdiff.Change) deployplan.ActionType {
-	return deployplan.ActionTypeUpdate
-}
-
 func makeCreateJob(config jobs.JobSettings) (jobs.CreateJob, error) {
 	result := jobs.CreateJob{}
 	// TODO: Validate copy - all fields must be initialized or explicitly allowed to be empty