-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Import block expansion with for_each #33932
Conversation
d634e2f
to
8bb3c2d
Compare
Update the import configuration block to decode for_each and to into raw expressions, so that we can later evaluate those during expansion.
8bb3c2d
to
8abf895
Compare
Refactor the existing resource for_each code so we can reuse some of the code for import expansion as well.
If the -generate-config-out flag was given but there are multiple import blocks for the instance, we can't add multiple nodes for the same resource.
We use the same graph node to start all resource planning, but must deal with the legacy import path differently because we need to end up in a legacy graphNodeImportState.
Let the legacy import path set IDString directly, simplifying the handling slightly. This also uncovered a long broken test which used an invalid import target.
fc3c7e3
to
0a20615
Compare
the plan test file is getting large again, so group import tests together
The diagnostics have been made more accurate, pointing to the problematic line rather than just the import block name.
Because expression from JSON files are handled differently from HCL, we need to deal with these manually when evaluating the special "to" syntax. We take the same approach as was done for replace_triggered_by, where the raw string is extracted from the json expression, and re-evaluated as an HCL expression.
Get rid of the duplicated code, and use the new function which also validates the expression result type that was missed in the existing implementation.
0a20615
to
66637ed
Compare
LegacyAddr: addr, | ||
IDString: args[1], |
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 considered adding "legacy" fields to ImportTarget
in #33618 but decided on the HCL synth kludge instead. Glad we are removing it.
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.
LGTM!
Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch. |
I wanted to express my excitement about this upcoming feature, which is highly anticipated for a specific use case within my role. Could you kindly provide information regarding the expected release date or version for this feature? Thank you in advance! |
@alezanper, the next minor release to be cut from the main development branch will be v1.7. There is no published date for release, though you can see from historical releases that they have been about every 4 months. |
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. |
This PR adds support for expanding import blocks to multiple resource instances via a
for_each
argument. This newfor_each
argument can be used to map import data to multiple resource instances, and across multiple module instances. Because it does not always correspond 1-1 with resource block, and the order of the imports is not directly related to the order of the resource instances, there is nocount
argument andfor_each
can accept all collection types in the same manner as dynamic blocks.The PR is unfortunately a bit large, but that is primarily due to some refactoring. The
refactor for_each evaluation
commit breaks up the evaluation offor_each
for resources into more discrete steps, so that we can share some of the important error conditions and processing with the slightly differentimport
use case. We also move all plan-able import tests into a newcontext_plan_import_test.go
file where the new tests are being added.Other refactoring includes the
configs.Import
type, which now works primarily withhcl.Expression
values, because we must always defer their evaluation until the point of resource expansion. We can statically extract theConfigResource
address (viaparseConfigResourceFromExpression
) from the expression to determine which configuration block to associate with the import, and store that inImport.ToResource
.ImportTarget
now only stores the configuration, the legacy target addr from theimport
command, and the evaluated ID string. When more structured import data becomes available, this can store the extendedcty.Value
as well.The methodology for expansion is still to "bind" the import block to the associated
resource
via theToResource
address when building the graph. This is similar to how theimport
block works now, but we need to move evaluation around a bit to ensure we have all expansion data necessary to evaluate thefor_each
. In essence, the importfor_each
is evaluated in conjunction with the resourcecount
orfor_each
, but within the root module scope. We sacrifice some detailed error help messages in the process because we no longer have all the data available (and maintaining that context all the way through would add a lot more overhead), but these are only edge cases and we make up for it in some more accurate error message with exact source locations. Simply continuing the addition of diagnostics support would likely be the most beneficial overall, and is unrelated to import itself.A simple example using the new
for_each
would look like:We can also expand imports across the same resource in different module instances:
Closes #33624