Skip to content

Conversation

@SarahFrench
Copy link
Member

@SarahFrench SarahFrench commented Sep 8, 2025

Context & how code was structured before this PR

Terraform command implementations obtain an operations backends to use from the (Meta).Backend method. This operations backend may be used for accessing state and/or for running operations.

When using the (Meta).Backend method, calling code may include parsed backend configuration in the BackendOpts argument passed into the Backend method:

// Get the backend config from the root module
backendConfig, configDiags := c.loadBackendConfig(".")
// handle configDiags errors etc

// Pass the config into the Backend method
be, beDiags = c.Backend(&BackendOpts{
	BackendConfig: backendConfig,
	ViewType:      viewType,
})

Or, the calling code could pass a nil value into the Backend method and instead rely on downstream logic to parse any backend blocks in the configuration (this happens if the BackendOpts.BackendConfig value is missing).

// Load the backend
b, backendDiags := c.Backend(nil)

Changes in this PR

This PR adds a new method: *(m Meta) prepareBackend

This method makes it easier to access an instance of an operations backend. This method parses any backend or state_store configuration present in the root module, provides that input to the (Meta).Backend method, and returns the operations backend from there. When pluggable state storage is in use the method takes care of obtaining locks and provider factories necessary for using PSS.

Overall, this PR makes Terraform commands compatible with state stored via state_store. The exceptions are commands like terraform cloud * which expect a specific type of state storage to be present in the configuration when the user executes the command.

Target Release

N/A

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@SarahFrench SarahFrench added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Sep 8, 2025
Comment on lines 284 to 294
func (c *ShowCommand) getDataFromCloudPlan(plan *cloudplan.SavedPlanBookmark, redacted bool) (*cloudplan.RemotePlanJSON, error) {
mod, diags := c.Meta.loadSingleModule(".")
if diags.HasErrors() {
return nil, diags.Err()
}

// Set up the backend
b, backendDiags := c.Backend(nil)
if backendDiags.HasErrors() {
return nil, errUnusable(backendDiags.Err(), "cloud plan")
b, diags := c.Meta.prepareBackend(mod)
if diags.HasErrors() {
return nil, errUnusable(diags.Err(), "cloud plan")
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This currently doesn't have any test coverage

@SarahFrench SarahFrench force-pushed the pss/update-how-commands-access-backend branch 2 times, most recently from f8d4dcc to 57f1d85 Compare October 20, 2025 11:10
@SarahFrench SarahFrench changed the base branch from main to pss/init-base-case-no-config-changes October 20, 2025 11:10
@SarahFrench SarahFrench force-pushed the pss/update-how-commands-access-backend branch from 57f1d85 to 3df8563 Compare October 20, 2025 12:20
Comment on lines 221 to 223
}
// TODO: Update BackendForLocalPlan to use state storage, and plan to be able to contain State Store config details
be, beDiags = c.BackendForLocalPlan(plan.Backend)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is scoped to this ticket: https://hashicorp.atlassian.net/browse/TF-28374
I've linked here from that ticket.

Base automatically changed from pss/init-base-case-no-config-changes to main October 20, 2025 15:51
@SarahFrench SarahFrench force-pushed the pss/update-how-commands-access-backend branch from 3df8563 to 823c5b4 Compare October 20, 2025 15:57
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some early thoughts after briefly glancing over the diffs

  1. (mostly stylistic) Can we drop all that .Meta stuff from the calling code? Meta is embedded struct inside of every command and it seems unnecessarily verbose to use that notation?
  2. If we now call loadSingleModule() to load the module configuration from various places, doesn't it mean that any diagnostics which aren't directly related to the backend or state store would prevent it from being initialised? 🤔

I can dig deeper and think more the effects and side effects, especially in relation to (2) later.

UPDATE:

Re (2) Upon closer inspection I can now see that the pre-existing logic already calls loadSingleModule and so there is no change in terms of what diagnostics would be raised and when, I suppose. The change just makes this effect more obvious.

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments in-line.

Regarding the note about missing test - I think the error case of cloud backend decoding could be covered by a new test in internal/command/meta_backend_test.go?

One more meta-question (no pun intended):

Could we make similar change to the earlier one and drop the .Meta from all of the c.Meta.loadSingleModule?

// This method should be used in NON-init operations only; it's incapable of processing new init command CLI flags used
// for partial configuration, however it will use the backend state file to use partial configuration from a previous
// init command.
func (m *Meta) prepareBackend(root *configs.Module) (backendrun.OperationsBackend, tfdiags.Diagnostics) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly readability related nitpick: I'd consider making this method's arguments a bit less "greedy", e.g. backend, cloudConfig, stateStore.

Then it makes it more obvious to the reader from all the calling places that this method cannot possibly do any business with the rest of the module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could resolve this by making this method load the root module, instead of using data from the calling code.

That would address this concern about duplicated warnings and resembles the original backendConfig method used.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah but now I take a proper look I realise that one of the things I was trying to do was avoid the root module being parsed multiple times (e.g. in internal/command/providers.go the diff uses some already-parsed config as the argument to this method).

I'll think on it more.

@radeksimko
Copy link
Member

One more note - sorry for the chaos!

I'd say this note in the existing code is relatively important:

// Only return error diagnostics at this point. Any warnings will be caught
// again later and duplicated in the output.
if diags.HasErrors() {
return nil, diags
}

I'm just unsure how to best preserve it in the wide-scale refactoring here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants