Repository: opentofu/opentofu
File: internal/tofu/node_resource_deposed.go (Go)

Code before:
	diags = diags.Append(destroyPlanDiags)
	if diags.HasErrors() {
		return diags
	}

	// Call pre-apply hook
	diags = diags.Append(n.preApplyHook(evalCtx, change))
	if diags.HasErrors() {
		return diags
	}

	// we pass a nil configuration to apply because we are destroying
	state, applyDiags := n.apply(ctx, evalCtx, state, change, nil, instances.RepetitionData{}, false)
	diags = diags.Append(applyDiags)
	// don't return immediately on errors, we need to handle the state

	// Always write the resource back to the state deposed. If it
	// was successfully destroyed it will be pruned. If it was not, it will
	// be caught on the next run.
	writeDiags := n.writeResourceInstanceState(ctx, evalCtx, state)
	diags.Append(writeDiags)
	if diags.HasErrors() {
		return diags
	}

	return diags.Append(n.postApplyHook(evalCtx, state, diags.Err()))
}

// GraphNodeDeposer is an optional interface implemented by graph nodes that
// might create a single new deposed object for a specific associated resource
// instance, allowing a caller to optionally pre-allocate a DeposedKey for
// it.
type GraphNodeDeposer interface {
	// SetPreallocatedDeposedKey will be called during graph construction
	// if a particular node must use a pre-allocated deposed key if/when it
	// "deposes" the current object of its associated resource instance.
	SetPreallocatedDeposedKey(key states.DeposedKey)
}

// graphNodeDeposer is an embeddable implementation of GraphNodeDeposer.
// Embed it in a node type to get automatic support for it, and then access
// the field PreallocatedDeposedKey to access any pre-allocated key.
type graphNodeDeposer struct {
	PreallocatedDeposedKey states.DeposedKey
}

func (n *graphNodeDeposer) SetPreallocatedDeposedKey(key states.DeposedKey) {
	n.PreallocatedDeposedKey = key
}

func (n *NodeDestroyDeposedResourceInstanceObject) writeResourceInstanceState(ctx context.Context, evalCtx EvalContext, obj *states.ResourceInstanceObject) error {

Review comment (bug):
This and [another similar change](https://github.com/opentofu/opentofu/pull/3011/files#diff-1ef543c63b8f3d30c4ab6826b558af9b23bd7ca475d2cefe408ab9ca25293b46L432-R441) have effectively swapped the order of hook calls so that `PostStateUpdate` now comes before `PostApply`. This means that `PostApply` can now potentially have a hook-reported error reported to it where it couldn't before.

So I looked for all of the implementations of `Hook.PostApply` to see if that seems likely to matter:

- [In `views.countHook`](https://github.com/opentofu/opentofu/blob/11d416edf9957b256f6ae7ddf91475bf53320fc2/internal/command/views/hook_count.go#L66) we update the counts of actions taken, but if we're going to return an error anyway then the counts are essentially irrelevant.
- [In `views.jsonHook`](https://github.com/opentofu/opentofu/blob/11d416edf9957b256f6ae7ddf91475bf53320fc2/internal/command/views/hook_json.go#L110) we just emit the JSON object describing the event, which could now describe an error from `PostStateUpdate` whereas before it wouldn't.
- [In `views.UiHook`](https://github.com/opentofu/opentofu/blob/11d416edf9957b256f6ae7ddf91475bf53320fc2/internal/command/views/hook_ui.go#L198) we would no longer print the "modifications complete" (or similar) message if `PostStateUpdate` fails, which is a little different than current behavior but seems relatively harmless.
- In `tofu.stopHook` the `PostStateUpdate` and `PostApply` behaviors are identical, so it doesn't matter which one is called first as long as nothing else happens between the two hook calls. (though https://github.com/opentofu/opentofu/pull/3011/files#r2274816098 highlights a situation where that doesn't seem to be true, which I'm intending to review more closely once I've looked over the other changes here.)

So it seems like swapping the order of these hook calls is technically end-user-visible but not in a way that is likely to cause any practical problems, but in https://github.com/opentofu/opentofu/pull/3011/files#r2275628449 @yottta found a situation where there were other side-effects between the two hook calls and so will need to look at that situation a little more closely.


Code after:
	diags = diags.Append(destroyPlanDiags)
	if diags.HasErrors() {
		return diags
	}

	// Call pre-apply hook
	diags = diags.Append(n.preApplyHook(evalCtx, change))
	if diags.HasErrors() {
		return diags
	}

	// we pass a nil configuration to apply because we are destroying
	state, applyDiags := n.apply(ctx, evalCtx, state, change, nil, instances.RepetitionData{}, false)
	diags = diags.Append(applyDiags)
	// don't return immediately on errors, we need to handle the state

	// Always write the resource back to the state deposed. If it
	// was successfully destroyed it will be pruned. If it was not, it will
	// be caught on the next run.
	writeDiags := n.writeResourceInstanceState(ctx, evalCtx, state)
	diags.Append(writeDiags)
	if diags.HasErrors() {
		return diags
	}

	diags = diags.Append(n.postApplyHook(evalCtx, state, diags.Err()))

	return diags.Append(updateStateHook(evalCtx, n.Addr))
}

// GraphNodeDeposer is an optional interface implemented by graph nodes that
// might create a single new deposed object for a specific associated resource
// instance, allowing a caller to optionally pre-allocate a DeposedKey for
// it.
type GraphNodeDeposer interface {
	// SetPreallocatedDeposedKey will be called during graph construction
	// if a particular node must use a pre-allocated deposed key if/when it
	// "deposes" the current object of its associated resource instance.
	SetPreallocatedDeposedKey(key states.DeposedKey)
}

// graphNodeDeposer is an embeddable implementation of GraphNodeDeposer.
// Embed it in a node type to get automatic support for it, and then access
// the field PreallocatedDeposedKey to access any pre-allocated key.
type graphNodeDeposer struct {
	PreallocatedDeposedKey states.DeposedKey
}

func (n *graphNodeDeposer) SetPreallocatedDeposedKey(key states.DeposedKey) {
	n.PreallocatedDeposedKey = key
}