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

Import block expansion with for_each #33932

Merged
merged 24 commits into from
Sep 29, 2023
Merged

Import block expansion with for_each #33932

merged 24 commits into from
Sep 29, 2023

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Sep 22, 2023

This PR adds support for expanding import blocks to multiple resource instances via a for_each argument. This new for_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 no count argument and for_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 of for_each for resources into more discrete steps, so that we can share some of the important error conditions and processing with the slightly different import use case. We also move all plan-able import tests into a new context_plan_import_test.go file where the new tests are being added.

Other refactoring includes the configs.Import type, which now works primarily with hcl.Expression values, because we must always defer their evaluation until the point of resource expansion. We can statically extract the ConfigResource address (via parseConfigResourceFromExpression) from the expression to determine which configuration block to associate with the import, and store that in Import.ToResource. ImportTarget now only stores the configuration, the legacy target addr from the import command, and the evaluated ID string. When more structured import data becomes available, this can store the extended cty.Value as well.

The methodology for expansion is still to "bind" the import block to the associated resource via the ToResource address when building the graph. This is similar to how the import block works now, but we need to move evaluation around a bit to ensure we have all expansion data necessary to evaluate the for_each. In essence, the import for_each is evaluated in conjunction with the resource count or for_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:

import {
  for_each = {
    "staging" = "bucket1"
    "uat"     = "bucket2"
    "prod"    = "bucket3"
  }
  to = aws_s3_bucket.this[each.key]
  id = each.value
}

resource "aws_sw_bucket" "this" {
  ...
}

We can also expand imports across the same resource in different module instances:

locals {
  bucket_ids = [
    { 
      group = "one"
      key   = "bucket1"
      id    = "one_1"
    },
    {
      group = "one"
      key   = "bucket2"
      id    = "one_2"
    },
    {
      group = "two"
      key   = "bucket1"
      id    = "two_1"
    },
    {
      group = "one"
      key   = "bucket2"
      id    = "two_2"
    },
  ]
}

import {
  for_each = local.bucket_ids
  id = each.value.id
  to = module.group[each.value.group].aws_s3_bucket.this[each.value.key]
}

Closes #33624

Update the import configuration block to decode for_each and to into raw
expressions, so that we can later evaluate those during expansion.
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.
@jbardin jbardin force-pushed the jbardin/import-for-each branch from fc3c7e3 to 0a20615 Compare September 26, 2023 15:31
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.
@jbardin jbardin force-pushed the jbardin/import-for-each branch from 0a20615 to 66637ed Compare September 26, 2023 15:37
@jbardin jbardin marked this pull request as ready for review September 26, 2023 16:00
@jbardin jbardin requested a review from a team September 26, 2023 16:00
Comment on lines +238 to +239
LegacyAddr: addr,
IDString: args[1],
Copy link
Member

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.

Copy link
Member

@kmoe kmoe left a comment

Choose a reason for hiding this comment

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

LGTM!

@jbardin jbardin merged commit 2664c06 into main Sep 29, 2023
@jbardin jbardin deleted the jbardin/import-for-each branch September 29, 2023 13:47
@github-actions
Copy link
Contributor

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

@alezanper
Copy link

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!

@jbardin
Copy link
Member Author

jbardin commented Oct 13, 2023

@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.

Copy link
Contributor

github-actions bot commented Dec 7, 2023

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 Dec 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plannable Import: Support for_each
5 participants