-
Notifications
You must be signed in to change notification settings - Fork 10.3k
feat(import): support import blocks inside child modules #38352
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
base: main
Are you sure you want to change the base?
Changes from all commits
044a9ad
58c07f7
2ec7f39
9d74c30
e085a3c
a8010ba
de3cfda
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| kind: NEW FEATURES | ||
| body: feat(import): add support for import blocks inside modules | ||
| time: 2026-04-10T08:57:29.804462-04:00 | ||
| custom: | ||
| Issue: "38352" |
This file was deleted.
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| provider "random" { | ||
| alias = "thisone" | ||
| } | ||
|
|
||
| import { | ||
| to = random_string.test1 | ||
| provider = localname | ||
| id = "importlocalname" | ||
| } | ||
|
|
||
| import { | ||
| to = random_string.test2 | ||
| provider = random.thisone | ||
| id = "importaliased" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| module "child" { | ||
| source = "./child" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| // Copyright IBM Corp. 2014, 2026 | ||
| // SPDX-License-Identifier: BUSL-1.1 | ||
|
|
||
| package refactoring | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not super sold on putting this file in refactoring, but this package was the inspiration for it so 🤷🏻 happy to move it or nah.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah 🤔, I don't think prior to this PR that I would have thought of import as a "refactoring" operation but it doesn't seem too far off in concept I guess. The only other location I can think that would make sense is just closer to the caller/tests for this logic back in |
||
|
|
||
| import ( | ||
| "github.com/hashicorp/terraform/internal/addrs" | ||
| "github.com/hashicorp/terraform/internal/configs" | ||
| ) | ||
|
|
||
| type ImportStatement struct { | ||
| // AbsToResource is the original ImportConfig ToResource+ContainingModule | ||
| AbsToResource addrs.ConfigResource | ||
| ContainingModule addrs.Module | ||
| Import *configs.Import | ||
| } | ||
|
|
||
| // FindImportStatements recurses through the modules of the given configuration | ||
| // and returns a set of all "import" blocks defined within after deduplication | ||
| // on the To address. | ||
| // | ||
| // An "import" block in a parent module overrides an import block in a child | ||
| // module when both target the same configuration object. | ||
| func FindImportStatements(rootCfg *configs.Config) addrs.Map[addrs.ConfigResource, ImportStatement] { | ||
| imports := findImportStatements(rootCfg, addrs.MakeMap[addrs.ConfigResource, ImportStatement]()) | ||
| return imports | ||
| } | ||
|
|
||
| func findImportStatements(cfg *configs.Config, into addrs.Map[addrs.ConfigResource, ImportStatement]) addrs.Map[addrs.ConfigResource, ImportStatement] { | ||
| for _, mi := range cfg.Module.Import { | ||
| // First, stitch together the module path and the RelSubject to form | ||
| // the absolute address of the config resource being removed. | ||
| res := mi.ToResource | ||
| toAddr := addrs.ConfigResource{ | ||
| Module: append(cfg.Path, res.Module...), | ||
| Resource: res.Resource, | ||
| } | ||
|
|
||
| // If we already have an import statement for this ConfigResource, it | ||
| // must have come from a parent module, because duplicate import | ||
| // blocks in the same module result in an error. | ||
| // The import block in the parent module overrides the block in the | ||
| // child module. | ||
|
Comment on lines
+39
to
+43
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not immediately clear to me what the use-case for overriding an import block inside a module would be, maybe just a situation where a module author doesn't provide a proper way to influence the import command? 🤔 (with like a variable for example) Although, I'm also not well versed in why the existing
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed; this was weird so I copied the Though ... never mind overriding an import block, to be honest I am not very comfortable with this feature at all. the only use-cases I can actually come up with are solved by data sources, not importing resources into your state from nested modules. I can't think of many instances where I'd want to blanket import something in to every instance of a module that's being used multiple places, even back when I was managing relatively well contained modules and environments. If I owned everything, I could write the import statement in the root module, and if I don't own everything ... I don't want imports I didn't write. But I also came from a fairly specific way of working so I tried to let all that go. .... though now that I'm thinking about it, wearing my infra hat, I'd prefer want the ability to control this behavior (ie, block nested-module imports/fail/etc) from the root module (as a security stance), but I guess a sentinel policy could cover that. |
||
| existingResource, ok := into.GetOk(toAddr) | ||
| if ok { | ||
| if existingResource.AbsToResource.Equal(toAddr) { | ||
| continue | ||
| } | ||
| } | ||
|
|
||
| into.Put(toAddr, ImportStatement{ | ||
| AbsToResource: toAddr, | ||
| ContainingModule: cfg.Path, | ||
| Import: mi, | ||
| }) | ||
| } | ||
|
|
||
| for _, childCfg := range cfg.Children { | ||
| into = findImportStatements(childCfg, into) | ||
| } | ||
|
|
||
| return into | ||
| } | ||
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.
Are we changing any user assumptions by allowing modules to now use import blocks by default? (and if so, would there be value in only allowing local modules initially?)
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.
Yep, and I think it could be a security concern (see my above comment). On some level I didn't think of it because I wouldn't use terraform modules I didn't write or code review myself 🤔