Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make state commands check required version (#28025) #30511

Merged
merged 3 commits into from Mar 31, 2022
Merged

Make state commands check required version (#28025) #30511

merged 3 commits into from Mar 31, 2022

Conversation

gabriel376
Copy link
Contributor

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

// 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
Copy link

hashicorp-cla commented Feb 12, 2022

CLA assistant check
All committers have signed the CLA.

@radeksimko
Copy link
Member

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
Copy link
Member

alisdair commented Mar 2, 2022

@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
Copy link
Member

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.

@crw crw added the enhancement label Mar 4, 2022
Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

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?

@alisdair alisdair self-assigned this Mar 28, 2022
@gabriel376
Copy link
Contributor Author

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.

Copy link
Member

@alisdair alisdair left a comment

Choose a reason for hiding this comment

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

Thanks, this is great!

@alisdair alisdair merged commit f5a8608 into hashicorp:main Mar 31, 2022
@github-actions
Copy link

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

@github-actions
Copy link

github-actions bot commented May 1, 2022

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform state commands ignores required_version
5 participants