diff --git a/internal/addrs/move_endpoint.go b/internal/addrs/move_endpoint.go index 2b44c5cd1..765b66f5e 100644 --- a/internal/addrs/move_endpoint.go +++ b/internal/addrs/move_endpoint.go @@ -19,7 +19,7 @@ import ( // prompt creating a different plan than Terraform would by default. // // To obtain a full address from a MoveEndpoint you must use -// either the package function UnifyMoveEndpoints (to get an AbsMovable) or +// either the package function UnifyMoveEndpoints (to get an AbsMoveable) or // the method ConfigMoveable (to get a ConfigMoveable). type MoveEndpoint struct { // SourceRange is the location of the physical endpoint address @@ -27,15 +27,15 @@ type MoveEndpoint struct { // configuration expresson. SourceRange tfdiags.SourceRange - // Internally we (ab)use AbsMovable as the representation of our + // Internally we (ab)use AbsMoveable as the representation of our // relative address, even though everywhere else in Terraform - // AbsMovable always represents a fully-absolute address. + // AbsMoveable always represents a fully-absolute address. // In practice, due to the implementation of ParseMoveEndpoint, // this is always either a ModuleInstance or an AbsResourceInstance, // and we only consider the possibility of interpreting it as // a AbsModuleCall or an AbsResource in UnifyMoveEndpoints. // This is intentionally unexported to encapsulate this unusual - // meaning of AbsMovable. + // meaning of AbsMoveable. relSubject AbsMoveable } @@ -44,7 +44,7 @@ func (e *MoveEndpoint) ObjectKind() MoveEndpointKind { } func (e *MoveEndpoint) String() string { - // Our internal pseudo-AbsMovable representing the relative + // Our internal pseudo-AbsMoveable representing the relative // address (either ModuleInstance or AbsResourceInstance) is // a good enough proxy for the relative move endpoint address // serialization. diff --git a/internal/addrs/move_endpoint_module.go b/internal/addrs/move_endpoint_module.go index 8bbbe83e7..e2180f25a 100644 --- a/internal/addrs/move_endpoint_module.go +++ b/internal/addrs/move_endpoint_module.go @@ -131,7 +131,7 @@ func (e *MoveEndpointInModule) Module() Module { return e.module } -// InModuleInstance returns an AbsMovable address which concatenates the +// InModuleInstance returns an AbsMoveable address which concatenates the // given module instance address with the receiver's relative object selection // to produce one example of an instance that might be affected by this // move statement. diff --git a/internal/addrs/move_endpoint_test.go b/internal/addrs/move_endpoint_test.go index 6e117139b..602c7e971 100644 --- a/internal/addrs/move_endpoint_test.go +++ b/internal/addrs/move_endpoint_test.go @@ -12,7 +12,7 @@ import ( func TestParseMoveEndpoint(t *testing.T) { tests := []struct { Input string - WantRel AbsMoveable // funny intermediate subset of AbsMovable + WantRel AbsMoveable // funny intermediate subset of AbsMoveable WantErr string }{ { diff --git a/internal/addrs/moveable.go b/internal/addrs/moveable.go index a85fe72c1..b9d2f87f0 100644 --- a/internal/addrs/moveable.go +++ b/internal/addrs/moveable.go @@ -5,7 +5,7 @@ package addrs // with any other similar cross-module state refactoring statements we might // allow. // -// Note that AbsMovable represents an absolute address relative to the root +// Note that AbsMoveable represents an absolute address relative to the root // of the configuration, which is different than the direct representation // of these in configuration where the author gives an address relative to // the current module where the address is defined. The type MoveEndpoint @@ -16,7 +16,7 @@ type AbsMoveable interface { String() string } -// The following are all of the possible AbsMovable address types: +// The following are all of the possible AbsMoveable address types: var ( _ AbsMoveable = AbsResource{} _ AbsMoveable = AbsResourceInstance{} @@ -24,6 +24,19 @@ var ( _ AbsMoveable = AbsModuleCall{} ) +// AbsMoveableResource is an AbsMoveable that is either a resource or a resource +// instance. +type AbsMoveableResource interface { + AbsMoveable + AffectedAbsResource() AbsResource +} + +// The following are all of the possible AbsMoveableResource types: +var ( + _ AbsMoveableResource = AbsResource{} + _ AbsMoveableResource = AbsResourceInstance{} +) + // ConfigMoveable is similar to AbsMoveable but represents a static object in // the configuration, rather than an instance of that object created by // module expansion. diff --git a/internal/addrs/resource.go b/internal/addrs/resource.go index 0ebf89d99..d8596237f 100644 --- a/internal/addrs/resource.go +++ b/internal/addrs/resource.go @@ -186,6 +186,11 @@ func (r AbsResource) String() string { return fmt.Sprintf("%s.%s", r.Module.String(), r.Resource.String()) } +// AffectedAbsResource returns the AbsResource. +func (r AbsResource) AffectedAbsResource() AbsResource { + return r +} + func (r AbsResource) Equal(o AbsResource) bool { return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource) } @@ -267,6 +272,14 @@ func (r AbsResourceInstance) String() string { return fmt.Sprintf("%s.%s", r.Module.String(), r.Resource.String()) } +// AffectedAbsResource returns the AbsResource for the instance. +func (r AbsResourceInstance) AffectedAbsResource() AbsResource { + return AbsResource{ + Module: r.Module, + Resource: r.Resource.Resource, + } +} + func (r AbsResourceInstance) Equal(o AbsResourceInstance) bool { return r.Module.Equal(o.Module) && r.Resource.Equal(o.Resource) } diff --git a/internal/configs/moved_test.go b/internal/configs/moved_test.go index 4e2666a27..433525d28 100644 --- a/internal/configs/moved_test.go +++ b/internal/configs/moved_test.go @@ -9,7 +9,7 @@ import ( "github.com/hashicorp/terraform/internal/addrs" ) -func TestDecodeMovedBlock(t *testing.T) { +func TestMovedBlock_decode(t *testing.T) { blockRange := hcl.Range{ Filename: "mock.tf", Start: hcl.Pos{Line: 3, Column: 12, Byte: 27}, @@ -169,7 +169,7 @@ func TestDecodeMovedBlock(t *testing.T) { } } -func TestMovedBlocksInModule(t *testing.T) { +func TestMovedBlock_inModule(t *testing.T) { parser := NewParser(nil) mod, diags := parser.LoadConfigDir("testdata/valid-modules/moved-blocks") if diags.HasErrors() { diff --git a/internal/refactoring/move_validate.go b/internal/refactoring/move_validate.go index aad356cb2..133698608 100644 --- a/internal/refactoring/move_validate.go +++ b/internal/refactoring/move_validate.go @@ -110,7 +110,7 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts shortNoun = "resource" default: // The above cases should cover all of the AbsMoveable types - panic("unsupported AbsMovable address type") + panic("unsupported AbsMoveable address type") } // It's invalid to have a move statement whose "from" address @@ -178,6 +178,16 @@ func ValidateMoves(stmts []MoveStatement, rootCfg *configs.Config, declaredInsts } } + // Resource types must match. + if resourceTypesDiffer(absFrom, absTo) { + diags = diags.Append(&hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Resource type mismatch", + Detail: fmt.Sprintf( + "This statement declares a move from %s to %s, which is a %s of a different type.", absFrom, absTo, noun, + ), + }) + } } } @@ -234,7 +244,20 @@ func moveableObjectExists(addr addrs.AbsMoveable, in instances.Set) bool { return in.HasResource(addr) default: // The above cases should cover all of the AbsMoveable types - panic("unsupported AbsMovable address type") + panic("unsupported AbsMoveable address type") + } +} + +func resourceTypesDiffer(absFrom, absTo addrs.AbsMoveable) bool { + switch absFrom := absFrom.(type) { + case addrs.AbsMoveableResource: + // addrs.UnifyMoveEndpoints guarantees that both addresses are of the + // same kind, so at this point we can assume that absTo is also an + // addrs.AbsResourceInstance or addrs.AbsResource. + absTo := absTo.(addrs.AbsMoveableResource) + return absFrom.AffectedAbsResource().Resource.Type != absTo.AffectedAbsResource().Resource.Type + default: + return false } } @@ -309,7 +332,7 @@ func movableObjectDeclRange(addr addrs.AbsMoveable, cfg *configs.Config) (tfdiag return tfdiags.SourceRangeFromHCL(rc.DeclRange), true default: // The above cases should cover all of the AbsMoveable types - panic("unsupported AbsMovable address type") + panic("unsupported AbsMoveable address type") } } diff --git a/internal/refactoring/move_validate_test.go b/internal/refactoring/move_validate_test.go index 986c1d5db..53bbbe6c2 100644 --- a/internal/refactoring/move_validate_test.go +++ b/internal/refactoring/move_validate_test.go @@ -386,6 +386,24 @@ Each resource can have moved from only one source resource.`, }, WantError: ``, // This is okay because the call itself is not considered to be inside the package it refers to }, + "resource type mismatch": { + Statements: []MoveStatement{ + makeTestMoveStmt(t, ``, + `test.nonexist1`, + `other.single`, + ), + }, + WantError: `Resource type mismatch: This statement declares a move from test.nonexist1 to other.single, which is a resource of a different type.`, + }, + "resource instance type mismatch": { + Statements: []MoveStatement{ + makeTestMoveStmt(t, ``, + `test.nonexist1[0]`, + `other.single`, + ), + }, + WantError: `Resource type mismatch: This statement declares a move from test.nonexist1[0] to other.single, which is a resource instance of a different type.`, + }, } for name, test := range tests { diff --git a/internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf b/internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf index 492d671db..3cc8504f9 100644 --- a/internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf +++ b/internal/refactoring/testdata/move-validate-zoo/move-validate-root.tf @@ -41,6 +41,9 @@ resource "test" "for_each" { } } +resource "other" "single" { +} + module "fake_external" { # Our configuration fixture loader has a special case for a module call # named "fake_external" where it will mutate the source address after