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

Code before:

	steps := []GraphTransformer{
		// Creates all the resources represented in the config
		&ConfigTransformer{
			Concrete: b.ConcreteResource,
			Config:   b.Config,

			// Instead of just skipping the ConfigTransformer altogether during walkPlanDestroy,
			// we want to add only the ephemeral resources.
			// This is needed to be able later to use the changes generated to create
			// actual applyable instance nodes to have the ephemeral information fetched
			// for the nodes that depend on it (ie: configuring a "provider" block with ephemeral values)
			ModeFilter: func(mode addrs.ResourceMode) bool {
				if b.Operation != walkPlanDestroy {
					// Allow all the resource types on the operations that are not walkPlanDestroy
					return false
				}
				// For the walkPlanDestroy, allow only ephemeral resources
				return mode != addrs.EphemeralResourceMode
			},

			importTargets: b.ImportTargets,

			// We only want to generate config during a plan operation.
			generateConfigPathForImportTargets: b.GenerateConfigPath,
			operation:                          b.Operation,
		},

		// Add dynamic values
		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
		&ModuleVariableTransformer{Config: b.Config},
		&LocalTransformer{Config: b.Config},
		&OutputTransformer{
			Config:      b.Config,
			RefreshOnly: b.skipPlanChanges || b.preDestroyRefresh,
			Destroying:  b.Operation == walkPlanDestroy,

			// NOTE: We currently treat anything built with the plan graph
			// builder as "planning" for our purposes here, because we share
			// the same graph node implementation between all of the walk
			// types and so the pre-planning walks still think they are
			// producing a plan even though we immediately discard it.
			Planning: true,
		},

		// Add nodes and edges for the check block assertions. Check block data
		// sources were added earlier.
		&checkTransformer{
			Config:    b.Config,
			Operation: b.Operation,
		},

Review comment (suggestion):
I don't really like propagating the raw "operation" around like this, because it makes it easier for there to be hidden differences between the different walk types that we don't expect under maintenance.

Could we make this be a boolean field named something like `forceAddImportTargets`, and define it here as `forceAddImportTargets: b.Operation == walkValidate`? That way the graph transformer is just being directly told what to do, and we don't need to look so deep to find all of the potential differences by operation.

If you think this would make the code more complex or harder to read then I don't feel super strongly about it. This is just a preference I have based on previous experience being surprised by hidden uses of operation type in deep parts of the system.


Code after:

	steps := []GraphTransformer{
		// Creates all the resources represented in the config
		&ConfigTransformer{
			Concrete: b.ConcreteResource,
			Config:   b.Config,

			// Instead of just skipping the ConfigTransformer altogether during walkPlanDestroy,
			// we want to add only the ephemeral resources.
			// This is needed to be able later to use the changes generated to create
			// actual applyable instance nodes to have the ephemeral information fetched
			// for the nodes that depend on it (ie: configuring a "provider" block with ephemeral values)
			ModeFilter: func(mode addrs.ResourceMode) bool {
				if b.Operation != walkPlanDestroy {
					// Allow all the resource types on the operations that are not walkPlanDestroy
					return false
				}
				// For the walkPlanDestroy, allow only ephemeral resources
				return mode != addrs.EphemeralResourceMode
			},

			importTargets: b.ImportTargets,

			// We only want to generate config during a plan operation.
			generateConfigPathForImportTargets: b.GenerateConfigPath,
			forceAddImportTargets:              b.Operation == walkValidate,
		},

		// Add dynamic values
		&RootVariableTransformer{Config: b.Config, RawValues: b.RootVariableValues},
		&ModuleVariableTransformer{Config: b.Config},
		&LocalTransformer{Config: b.Config},
		&OutputTransformer{
			Config:      b.Config,
			RefreshOnly: b.skipPlanChanges || b.preDestroyRefresh,
			Destroying:  b.Operation == walkPlanDestroy,

			// NOTE: We currently treat anything built with the plan graph
			// builder as "planning" for our purposes here, because we share
			// the same graph node implementation between all of the walk
			// types and so the pre-planning walks still think they are
			// producing a plan even though we immediately discard it.
			Planning: true,
		},

		// Add nodes and edges for the check block assertions. Check block data
		// sources were added earlier.
		&checkTransformer{
			Config:    b.Config,
			Operation: b.Operation,
		},