-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
command: Taint should respect required_version #26345
command: Taint should respect required_version #26345
Conversation
Despite not requiring the configuration for any other reason, the taint subcommand should not execute if the required_version constraints cannot be met. Doing so can result in an undesirable state file upgrade.
Codecov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections to this PR as-is, but I did leave a suggestion for alternative config loading that may or may not be useful.
@@ -62,6 +64,34 @@ func (c *TaintCommand) Run(args []string) int { | |||
return 1 | |||
} | |||
|
|||
// Load the config and check the core version requirements are satisfied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but I'd be curious to see if the earlyconfig
loader (examples in init.go
) is sufficient and perhaps faster for this purpose. loadSingleModule
for e.g. will only load the root module:
terraform/command/meta_config.go
Lines 95 to 97 in 6d86cd4
// loadSingleModule reads configuration from the given directory and returns | |
// a description of that module only, without attempting to assemble a module | |
// tree for referenced child modules. |
Again, this might not actually be faster or better, but it might be worth looking at.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I hadn't thought of that! Unfortunately I think we need to load and check the entire tree's version constraints. In particular, many public registry modules have fairly strict version requirements that we need to honour.
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Despite not requiring the configuration for any other reason, the taint subcommand should not execute if the required_version constraints cannot be met. Doing so can result in an undesirable state file upgrade.
Fixes #26325. Fixes #23049.
Question for reviewers: it feels to me like there ought to be a neater way of loading the config to check the constraints are met. Can you think of one? 🙏