-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
terraform: Relax provider config ref constraints #25420
terraform: Relax provider config ref constraints #25420
Conversation
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.
📝
}, | ||
}, | ||
}) | ||
|
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 think this step was redundant with the "everything except validate" one a few lines below.
@@ -48,7 +35,6 @@ func ProviderEvalTree(n *NodeApplyableProvider, config *configs.Provider) EvalNo | |||
}, | |||
}) | |||
|
|||
// Apply stuff |
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.
This comment (and the one above) did not seem to mean anything. I'd happily add more comments here if I knew what to write…
Codecov Report
|
62bfcc0
to
c9bf9f3
Compare
When configuring providers, it is normally valid to refer to any value which is known at apply time. This can include resource instance attributes, variables, locals, and so on. The import command has a simpler graph evaluation, which means that many of these values are unknown. We previously prevented this from happening by restricting provider configuration references to input variables (#22862), but this was more restrictive than is necessary. This commit changes how we verify provider configuration for import. We no longer inspect the configuration references during graph building, because this is too early to determine if these values will become known or not. Instead, when the provider is configured during evaluation, we check if the configuration value is wholly known. If not, we fail with a diagnostic error. Includes a test case which verifies that providers can now be configured using locals as well as vars, and an updated test case which verifies that providers cannot be configured with references to resources.
c9bf9f3
to
ac99a3b
Compare
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.
Thanks for taking care of the dead code too!
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. |
When configuring providers, it is normally valid to refer to any value which is known at apply time. This can include resource instance attributes, variables, locals, and so on.
The import command has a simpler graph evaluation, which means that many of these values are unknown. We previously prevented this from happening by restricting provider configuration references to input variables (#22862), but this was more restrictive than is necessary.
This commit changes how we verify provider configuration for import. We no longer inspect the configuration references during graph building, because this is too early to determine if these values will become known
or not.
Instead, when the provider is configured during evaluation, we check if the configuration value is wholly known. If not, we fail with a diagnostic error.
Includes a test case which verifies that providers can now be configured using locals as well as vars, and an updated test case which verifies that providers cannot be configured with references to resources.
Fixes #25407