From 56a1e0d1c6c9adf69aa5b5477b5e307fdf3a4496 Mon Sep 17 00:00:00 2001 From: kmoe <5575356+kmoe@users.noreply.github.com> Date: Tue, 16 Aug 2022 16:52:57 +0200 Subject: [PATCH] allow cross-package move statements (#31556) --- internal/addrs/move_endpoint_module.go | 6 +- internal/refactoring/move_validate.go | 65 ++----------------- internal/refactoring/move_validate_test.go | 8 +-- .../language/modules/develop/refactoring.mdx | 6 -- 4 files changed, 13 insertions(+), 72 deletions(-) diff --git a/internal/addrs/move_endpoint_module.go b/internal/addrs/move_endpoint_module.go index fdc8a5c25d..f2c1408d66 100644 --- a/internal/addrs/move_endpoint_module.go +++ b/internal/addrs/move_endpoint_module.go @@ -5,8 +5,9 @@ import ( "reflect" "strings" - "github.com/hashicorp/terraform/internal/tfdiags" "github.com/zclconf/go-cty/cty" + + "github.com/hashicorp/terraform/internal/tfdiags" ) // anyKeyImpl is the InstanceKey representation indicating a wildcard, which @@ -179,8 +180,7 @@ func (e *MoveEndpointInModule) InModuleInstance(modInst ModuleInstance) AbsMovea // while selecting a particular object to move. // // This is a rather special-purpose function here mainly to support our -// validation rule that a module can only traverse down into child modules -// that belong to the same module package. +// validation rule that a module can only traverse down into child modules. func (e *MoveEndpointInModule) ModuleCallTraversals() (Module, []ModuleCall) { // We're returning []ModuleCall rather than Module here to make it clearer // that this is a relative sequence of calls rather than an absolute diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index 5fb646ef20..585f623687 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -50,46 +50,13 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts for _, stmt := range stmts { // Earlier code that constructs MoveStatement values should ensure that - // both stmt.From and stmt.To always belong to the same statement and - // thus to the same module. - stmtMod, fromCallSteps := stmt.From.ModuleCallTraversals() - _, toCallSteps := stmt.To.ModuleCallTraversals() + // both stmt.From and stmt.To always belong to the same statement. + fromMod, _ := stmt.From.ModuleCallTraversals() - modCfg := rootCfg.Descendent(stmtMod) - if !stmt.Implied { - // Implied statements can cross module boundaries because we - // generate them only for changing instance keys on a single - // resource. They happen to be generated _as if_ they were written - // in the root module, but the source and destination are always - // in the same module anyway. - if pkgAddr := callsThroughModulePackage(modCfg, fromCallSteps); pkgAddr != nil { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Cross-package move statement", - Detail: fmt.Sprintf( - "This statement declares a move from an object declared in external module package %q. Move statements can be only within a single module package.", - pkgAddr, - ), - Subject: stmt.DeclRange.ToHCL().Ptr(), - }) - } - if pkgAddr := callsThroughModulePackage(modCfg, toCallSteps); pkgAddr != nil { - diags = diags.Append(&hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Cross-package move statement", - Detail: fmt.Sprintf( - "This statement declares a move to an object declared in external module package %q. Move statements can be only within a single module package.", - pkgAddr, - ), - Subject: stmt.DeclRange.ToHCL().Ptr(), - }) - } - } + for _, fromModInst := range declaredInsts.InstancesForModule(fromMod) { + absFrom := stmt.From.InModuleInstance(fromModInst) - for _, modInst := range declaredInsts.InstancesForModule(stmtMod) { - - absFrom := stmt.From.InModuleInstance(modInst) - absTo := stmt.To.InModuleInstance(modInst) + absTo := stmt.To.InModuleInstance(fromModInst) if addrs.Equivalent(absFrom, absTo) { diags = diags.Append(&hcl.Diagnostic{ @@ -199,6 +166,7 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts ), }) } + } } @@ -370,24 +338,3 @@ func movableObjectDeclRange(addr addrs.AbsMoveable, cfg *configs.Config) (tfdiag panic("unsupported AbsMoveable address type") } } - -func callsThroughModulePackage(modCfg *configs.Config, callSteps []addrs.ModuleCall) addrs.ModuleSource { - var sourceAddr addrs.ModuleSource - current := modCfg - for _, step := range callSteps { - call := current.Module.ModuleCalls[step.Name] - if call == nil { - break - } - if call.EntersNewPackage() { - sourceAddr = call.SourceAddr - } - current = modCfg.Children[step.Name] - if current == nil { - // Weird to have a call but not a config, but we'll tolerate - // it to avoid crashing here. - break - } - } - return sourceAddr -} diff --git a/internal/refactoring/move_validate_test.go b/internal/refactoring/move_validate_test.go index 6f57b8e2c6..56d767af51 100644 --- a/internal/refactoring/move_validate_test.go +++ b/internal/refactoring/move_validate_test.go @@ -335,7 +335,7 @@ Each resource can have moved from only one source resource.`, `test.thing`, ), }, - WantError: `Cross-package move statement: This statement declares a move from an object declared in external module package "fake-external:///". Move statements can be only within a single module package.`, + WantError: ``, }, "move to resource in another module package": { Statements: []MoveStatement{ @@ -345,7 +345,7 @@ Each resource can have moved from only one source resource.`, `module.fake_external.test.thing`, ), }, - WantError: `Cross-package move statement: This statement declares a move to an object declared in external module package "fake-external:///". Move statements can be only within a single module package.`, + WantError: ``, }, "move from module call in another module package": { Statements: []MoveStatement{ @@ -355,7 +355,7 @@ Each resource can have moved from only one source resource.`, `module.b`, ), }, - WantError: `Cross-package move statement: This statement declares a move from an object declared in external module package "fake-external:///". Move statements can be only within a single module package.`, + WantError: ``, }, "move to module call in another module package": { Statements: []MoveStatement{ @@ -365,7 +365,7 @@ Each resource can have moved from only one source resource.`, `module.fake_external.module.b`, ), }, - WantError: `Cross-package move statement: This statement declares a move to an object declared in external module package "fake-external:///". Move statements can be only within a single module package.`, + WantError: ``, }, "implied move from resource in another module package": { Statements: []MoveStatement{ diff --git a/website/docs/language/modules/develop/refactoring.mdx b/website/docs/language/modules/develop/refactoring.mdx index 210d025ce0..2831404161 100644 --- a/website/docs/language/modules/develop/refactoring.mdx +++ b/website/docs/language/modules/develop/refactoring.mdx @@ -400,12 +400,6 @@ assumes that all three of these modules are maintained by the same people and distributed together in a single [module package](/language/modules/sources#modules-in-package-sub-directories). -To reduce [coupling](https://en.wikipedia.org/wiki/Coupling_\(computer_programming\)) -between separately-packaged modules, Terraform only allows declarations of -moves between modules in the same package. In other words, Terraform would -not have allowed moving into `module.x` above if the `source` address of -that call had not been a [local path](/language/modules/sources#local-paths). - Terraform resolves module references in `moved` blocks relative to the module instance they are defined in. For example, if the original module above were already a child module named `module.original`, the reference to