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.

Sign up for GitHub

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

Jump to bottom

refactoring: allow cross-package move statements #31556

Merged
merged 1 commit into from Aug 16, 2022
Merged

refactoring: allow cross-package move statements #31556

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions internal/addrs/move_endpoint_module.go
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
65 changes: 6 additions & 59 deletions internal/refactoring/move_validate.go
View file
Open in desktop
Expand Up @@ -49,46 +49,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{
Expand Down Expand Up @@ -198,6 +165,7 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts
),
})
}

}
}

Expand Down Expand Up @@ -369,24 +337,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
}
8 changes: 4 additions & 4 deletions internal/refactoring/move_validate_test.go
View file
Open in desktop
Expand Up @@ -334,7 +334,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{
Expand All @@ -344,7 +344,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{
Expand All @@ -354,7 +354,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{
Expand All @@ -364,7 +364,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{
Expand Down
6 changes: 0 additions & 6 deletions website/docs/language/modules/develop/refactoring.mdx
View file
Open in desktop
Expand Up @@ -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
Expand Down