Make state commands check required version (#28025)

[gabriel376]

Description

This PR adds a CheckRequiredVersion for all state subcommands.
Fixes #28025, based on #26345

Problem

Currently, all state subcommands don't load the configuration, skipping the required_version constraint.

Fix

The fix is similar to #26345, where the configuration is loaded before the subcommand is executed.

Tests

Unit tests exists for all state subcommands.

Notes

I extracted the logic that checks the required version from

terraform/internal/command/taint.go

Lines 61 to 87 in 7a20c07

	// Load the config and check the core version requirements are satisfied
	loader, err := c.initConfigLoader()
	if err != nil {
	diags = diags.Append(err)
	c.showDiagnostics(diags)
	return 1
	}
	pwd, err := os.Getwd()
	if err != nil {
	c.Ui.Error(fmt.Sprintf("Error getting pwd: %s", err))
	return 1
	}
	config, configDiags := loader.LoadConfig(pwd)
	diags = diags.Append(configDiags)
	if diags.HasErrors() {
	c.showDiagnostics(diags)
	return 1
	}
	versionDiags := terraform.CheckCoreVersionRequirements(config)
	diags = diags.Append(versionDiags)
	if diags.HasErrors() {
	c.showDiagnostics(diags)
	return 1
	}

to its own function on command/CheckRequiredVersion() to avoid code duplication, although some duplication still exists in the unit tests. If there is a better approach, I will be glad to implement it.

[hashicorp-cla]

All committers have signed the CLA.

[radeksimko]

I understand the point made in #28025 but I just wanted to note that such a change (which applies to #26345 also) will likely make it more difficult to integrate these commands in the future into editors via language server.

We already ran into one case where Terraform CLI command imposes some assumptions making it very difficult to integrate with editors (see e.g. #24261).

Generally editors have to work with a virtual copy of a file which may not necessarily match the copy on disk.

That is not to say that the original problem described in #28025 is invalid but addressing it might require some extra care with the above in mind.

[alisdair]

@radeksimko Thanks for that note! If we were to make this change, I think we'd only want to do so for state-modifying commands. terraform state show and state list should not be affected. Would that still be a concern for editor integrations? Wouldn't editor users want the required version constraints to be checked for commands which would modify state?

[radeksimko]

Would that still be a concern for editor integrations? Wouldn't editor users want the required version constraints to be checked for commands which would modify state?

We do want to integrate with these commands which change state at some point, to enable refactoring in some way. I can't explain how much of a problem it ends up being in that context as we have not done any research into this feature yet.

I guess this all also comes down to what do we expect the sequence of refactoring steps to be. Right now AFAIK there doesn't seem to be a lot of opinions/guidance and users have to understand the consequence of doing state moves 1st or config changes 1st. I guess the challenge is generally that state and code are typically in two different places and attempts to do anything "atomic" across the two are likely to fail anyway. So I'm not even sure we can make safe assumptions about when/whether config should be saved, but we can probably provide some safety with state locking.

I'm guessing that what you're proposing may be a reasonable default behaviour (to read from disk). I do think that some commands, including the state subcommands will need to have some way of either reading from a virtual filesystem at some point or ignoring configuration, more likely the latter.

Another reason I'd prefer Terraform commands to not make assumptions about config in the context of editor is because much of its behaviour is based on the assumption that user works with a valid/correct config and if not then everything blows up. This is reasonable/wanted for testing and production, but is contrary to how many language servers work - we have to be able to deal with incomplete and invalid configs and still do something useful. #24261 is possibly the best example of that but

TL;DR feel free to go ahead as I unfortunately can't suggest any solution right now - I just wanted to note that we'll likely need to come back to this in the future.

[alisdair]

Thanks for opening this!

Just to clarify my first message in this thread with a real review: I think this makes sense for the state commands which can modify state, but not those which only read it. Could you update the changes to skip this behaviour for state list and state show?

I think the CheckRequiredVersion function could be an (unexported) method on the Meta struct, rather than taking it as an argument. Would that make sense?

[gabriel376]

Hi @alisdair,

Thank you for your suggestion, I just pushed new commits that implement the proposed changes. Let me know if you have any additional feedback.

[alisdair]

Thanks, this is great!

[github-actions]

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.