From c07b0a7806921261dec654cd39ed51fd81b3cd40 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 6 Apr 2018 11:07:39 -0700 Subject: [PATCH] configs: Re-unify the ManagedResource and DataResource types Initially the intent here was to tease these apart a little more since they don't really share much behavior in common in core, but in practice it'll take a lot of refactoring to tease apart these assumptions in core right now and so we'll keep these things unified at the configuration layer in the interests of minimizing disruption at the core layer. The two types are still kept in separate maps to help reinforce the fact that they are separate concepts with some behaviors in common, rather than the same concept. --- configs/module.go | 12 ++-- configs/module_merge.go | 77 +++++++++------------- configs/resource.go | 101 ++++++++++++----------------- terraform/resource_address.go | 24 +++++-- terraform/resource_address_test.go | 38 +++++++---- 5 files changed, 123 insertions(+), 129 deletions(-) diff --git a/configs/module.go b/configs/module.go index dec21d045..ebb109c37 100644 --- a/configs/module.go +++ b/configs/module.go @@ -21,8 +21,8 @@ type Module struct { ModuleCalls map[string]*ModuleCall - ManagedResources map[string]*ManagedResource - DataResources map[string]*DataResource + ManagedResources map[string]*Resource + DataResources map[string]*Resource } // File describes the contents of a single configuration file. @@ -49,8 +49,8 @@ type File struct { ModuleCalls []*ModuleCall - ManagedResources []*ManagedResource - DataResources []*DataResource + ManagedResources []*Resource + DataResources []*Resource } // NewModule takes a list of primary files and a list of override files and @@ -70,8 +70,8 @@ func NewModule(primaryFiles, overrideFiles []*File) (*Module, hcl.Diagnostics) { Locals: map[string]*Local{}, Outputs: map[string]*Output{}, ModuleCalls: map[string]*ModuleCall{}, - ManagedResources: map[string]*ManagedResource{}, - DataResources: map[string]*DataResource{}, + ManagedResources: map[string]*Resource{}, + DataResources: map[string]*Resource{}, } for _, file := range primaryFiles { diff --git a/configs/module_merge.go b/configs/module_merge.go index ce3bcd4b7..12614c1d6 100644 --- a/configs/module_merge.go +++ b/configs/module_merge.go @@ -3,6 +3,8 @@ package configs import ( "fmt" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/hcl2/hcl" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/convert" @@ -188,54 +190,14 @@ func (mc *ModuleCall) merge(omc *ModuleCall) hcl.Diagnostics { return diags } -func (r *ManagedResource) merge(or *ManagedResource) hcl.Diagnostics { +func (r *Resource) merge(or *Resource) hcl.Diagnostics { var diags hcl.Diagnostics - if or.Connection != nil { - r.Connection = or.Connection + if r.Mode != or.Mode { + // This is always a programming error, since managed and data resources + // are kept in separate maps in the configuration structures. + panic(fmt.Errorf("can't merge %s into %s", or.Mode, r.Mode)) } - if or.Count != nil { - r.Count = or.Count - } - if or.CreateBeforeDestroySet { - r.CreateBeforeDestroy = or.CreateBeforeDestroy - r.CreateBeforeDestroySet = or.CreateBeforeDestroySet - } - if or.ForEach != nil { - r.ForEach = or.ForEach - } - if len(or.IgnoreChanges) != 0 { - r.IgnoreChanges = or.IgnoreChanges - } - if or.PreventDestroySet { - r.PreventDestroy = or.PreventDestroy - r.PreventDestroySet = or.PreventDestroySet - } - if or.ProviderConfigRef != nil { - r.ProviderConfigRef = or.ProviderConfigRef - } - if len(or.Provisioners) != 0 { - r.Provisioners = or.Provisioners - } - - r.Config = MergeBodies(r.Config, or.Config) - - // We don't allow depends_on to be overridden because that is likely to - // cause confusing misbehavior. - if len(r.DependsOn) != 0 { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Unsupported override", - Detail: "The depends_on argument may not be overridden.", - Subject: r.DependsOn[0].SourceRange().Ptr(), // the first item is the closest range we have - }) - } - - return diags -} - -func (r *DataResource) merge(or *DataResource) hcl.Diagnostics { - var diags hcl.Diagnostics if or.Count != nil { r.Count = or.Count @@ -246,17 +208,38 @@ func (r *DataResource) merge(or *DataResource) hcl.Diagnostics { if or.ProviderConfigRef != nil { r.ProviderConfigRef = or.ProviderConfigRef } + if r.Mode == addrs.ManagedResourceMode { + // or.Managed is always non-nil for managed resource mode + + if or.Managed.Connection != nil { + r.Managed.Connection = or.Managed.Connection + } + if or.Managed.CreateBeforeDestroySet { + r.Managed.CreateBeforeDestroy = or.Managed.CreateBeforeDestroy + r.Managed.CreateBeforeDestroySet = or.Managed.CreateBeforeDestroySet + } + if len(or.Managed.IgnoreChanges) != 0 { + r.Managed.IgnoreChanges = or.Managed.IgnoreChanges + } + if or.Managed.PreventDestroySet { + r.Managed.PreventDestroy = or.Managed.PreventDestroy + r.Managed.PreventDestroySet = or.Managed.PreventDestroySet + } + if len(or.Managed.Provisioners) != 0 { + r.Managed.Provisioners = or.Managed.Provisioners + } + } r.Config = MergeBodies(r.Config, or.Config) // We don't allow depends_on to be overridden because that is likely to // cause confusing misbehavior. - if len(r.DependsOn) != 0 { + if len(or.DependsOn) != 0 { diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Unsupported override", Detail: "The depends_on argument may not be overridden.", - Subject: r.DependsOn[0].SourceRange().Ptr(), // the first item is the closest range we have + Subject: or.DependsOn[0].SourceRange().Ptr(), // the first item is the closest range we have }) } diff --git a/configs/resource.go b/configs/resource.go index f9534762e..c3193bf61 100644 --- a/configs/resource.go +++ b/configs/resource.go @@ -4,13 +4,16 @@ import ( "fmt" "strings" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/hcl2/gohcl" "github.com/hashicorp/hcl2/hcl" "github.com/hashicorp/hcl2/hcl/hclsyntax" ) -// ManagedResource represents a "resource" block in a module or file. -type ManagedResource struct { +// Resource represents a "resource" or "data" block in a module or file. +type Resource struct { + Mode addrs.ResourceMode Name string Type string Config hcl.Body @@ -21,6 +24,17 @@ type ManagedResource struct { DependsOn []hcl.Traversal + // Managed is populated only for Mode = addrs.ManagedResourceMode, + // containing the additional fields that apply to managed resources. + // For all other resource modes, this field is nil. + Managed *ManagedResource + + DeclRange hcl.Range + TypeRange hcl.Range +} + +// ManagedResource represents a "resource" block in a module or file. +type ManagedResource struct { Connection *Connection Provisioners []*Provisioner @@ -31,20 +45,24 @@ type ManagedResource struct { CreateBeforeDestroySet bool PreventDestroySet bool - - DeclRange hcl.Range - TypeRange hcl.Range } -func (r *ManagedResource) moduleUniqueKey() string { - return fmt.Sprintf("%s.%s", r.Name, r.Type) +func (r *Resource) moduleUniqueKey() string { + switch r.Mode { + case addrs.ManagedResourceMode: + return fmt.Sprintf("%s.%s", r.Name, r.Type) + case addrs.DataResourceMode: + return fmt.Sprintf("data.%s.%s", r.Name, r.Type) + default: + panic(fmt.Errorf("Resource has invalid resource mode %s", r.Mode)) + } } // ProviderConfigKey returns a string key for the provider configuration // that should be used for this resource. This function implements the // default behavior of extracting the type from the resource type name if // an explicit "provider" argument was not provided. -func (r *ManagedResource) ProviderConfigKey() string { +func (r *Resource) ProviderConfigKey() string { if r.ProviderConfigRef == nil { typeName := r.Type if under := strings.Index(typeName, "_"); under != -1 { @@ -56,12 +74,14 @@ func (r *ManagedResource) ProviderConfigKey() string { return r.ProviderConfigRef.String() } -func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) { - r := &ManagedResource{ +func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { + r := &Resource{ + Mode: addrs.ManagedResourceMode, Type: block.Labels[0], Name: block.Labels[1], DeclRange: block.DefRange, TypeRange: block.LabelRanges[0], + Managed: &ManagedResource{}, } content, remain, diags := block.Body.PartialContent(resourceBlockSchema) @@ -124,15 +144,15 @@ func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) { diags = append(diags, lcDiags...) if attr, exists := lcContent.Attributes["create_before_destroy"]; exists { - valDiags := gohcl.DecodeExpression(attr.Expr, nil, &r.CreateBeforeDestroy) + valDiags := gohcl.DecodeExpression(attr.Expr, nil, &r.Managed.CreateBeforeDestroy) diags = append(diags, valDiags...) - r.CreateBeforeDestroySet = true + r.Managed.CreateBeforeDestroySet = true } if attr, exists := lcContent.Attributes["prevent_destroy"]; exists { - valDiags := gohcl.DecodeExpression(attr.Expr, nil, &r.PreventDestroy) + valDiags := gohcl.DecodeExpression(attr.Expr, nil, &r.Managed.PreventDestroy) diags = append(diags, valDiags...) - r.PreventDestroySet = true + r.Managed.PreventDestroySet = true } if attr, exists := lcContent.Attributes["ignore_changes"]; exists { @@ -151,7 +171,7 @@ func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) { switch { case kw == "all": - r.IgnoreAllChanges = true + r.Managed.IgnoreAllChanges = true default: exprs, listDiags := hcl.ExprList(attr.Expr) diags = append(diags, listDiags...) @@ -163,7 +183,7 @@ func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) { // our expr might be the literal string "*", which // we accept as a deprecated way of saying "all". if shimIsIgnoreChangesStar(expr) { - r.IgnoreAllChanges = true + r.Managed.IgnoreAllChanges = true ignoreAllRange = expr.Range() diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagWarning, @@ -180,11 +200,11 @@ func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) { traversal, travDiags := hcl.RelTraversalForExpr(expr) diags = append(diags, travDiags...) if len(traversal) != 0 { - r.IgnoreChanges = append(r.IgnoreChanges, traversal) + r.Managed.IgnoreChanges = append(r.Managed.IgnoreChanges, traversal) } } - if r.IgnoreAllChanges && len(r.IgnoreChanges) != 0 { + if r.Managed.IgnoreAllChanges && len(r.Managed.IgnoreChanges) != 0 { diags = append(diags, &hcl.Diagnostic{ Severity: hcl.DiagError, Summary: "Invalid ignore_changes ruleset", @@ -212,13 +232,13 @@ func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) { conn, connDiags := decodeConnectionBlock(block) diags = append(diags, connDiags...) - r.Connection = conn + r.Managed.Connection = conn case "provisioner": pv, pvDiags := decodeProvisionerBlock(block) diags = append(diags, pvDiags...) if pv != nil { - r.Provisioners = append(r.Provisioners, pv) + r.Managed.Provisioners = append(r.Managed.Provisioners, pv) } default: @@ -231,44 +251,9 @@ func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) { return r, diags } -// DataResource represents a "data" block in a module or file. -type DataResource struct { - Name string - Type string - Config hcl.Body - Count hcl.Expression - ForEach hcl.Expression - - ProviderConfigRef *ProviderConfigRef - - DependsOn []hcl.Traversal - - DeclRange hcl.Range - TypeRange hcl.Range -} - -func (r *DataResource) moduleUniqueKey() string { - return fmt.Sprintf("data.%s.%s", r.Name, r.Type) -} - -// ProviderConfigKey returns a string key for the provider configuration -// that should be used for this resource. This function implements the -// default behavior of extracting the type from the resource type name if -// an explicit "provider" argument was not provided. -func (r *DataResource) ProviderConfigKey() string { - if r.ProviderConfigRef == nil { - typeName := r.Type - if under := strings.Index(typeName, "_"); under != -1 { - return typeName[:under] - } - return typeName - } - - return r.ProviderConfigRef.String() -} - -func decodeDataBlock(block *hcl.Block) (*DataResource, hcl.Diagnostics) { - r := &DataResource{ +func decodeDataBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { + r := &Resource{ + Mode: addrs.DataResourceMode, Type: block.Labels[0], Name: block.Labels[1], DeclRange: block.DefRange, diff --git a/terraform/resource_address.go b/terraform/resource_address.go index ee7f2117b..cb9392f39 100644 --- a/terraform/resource_address.go +++ b/terraform/resource_address.go @@ -7,6 +7,8 @@ import ( "strconv" "strings" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/configs" ) @@ -109,7 +111,7 @@ func (r *ResourceAddress) WholeModuleAddress() *ResourceAddress { } } -// MatchesManagedResourceConfig returns true if the receiver matches the given +// MatchesResourceConfig returns true if the receiver matches the given // configuration resource within the given _static_ module path. Note that // the module path in a resource address is a _dynamic_ module path, and // multiple dynamic resource paths may map to a single static path if @@ -118,10 +120,21 @@ func (r *ResourceAddress) WholeModuleAddress() *ResourceAddress { // Since resource configuration blocks represent all of the instances of // a multi-instance resource, the index of the address (if any) is not // considered. -func (r *ResourceAddress) MatchesManagedResourceConfig(path []string, rc *configs.ManagedResource) bool { +func (r *ResourceAddress) MatchesResourceConfig(path addrs.Module, rc *configs.Resource) bool { if r.HasResourceSpec() { - if r.Mode != config.ManagedResourceMode { - return false + // FIXME: Some ugliness while we are between worlds. Functionality + // in "addrs" should eventually replace this ResourceAddress idea + // completely, but for now we'll need to translate to the old + // way of representing resource modes. + switch r.Mode { + case config.ManagedResourceMode: + if rc.Mode != addrs.ManagedResourceMode { + return false + } + case config.DataResourceMode: + if rc.Mode != addrs.DataResourceMode { + return false + } } if r.Type != rc.Type || r.Name != rc.Name { return false @@ -137,7 +150,8 @@ func (r *ResourceAddress) MatchesManagedResourceConfig(path []string, rc *config if len(path) == 0 { path = nil } - return reflect.DeepEqual(addrPath, path) + rawPath := []string(path) + return reflect.DeepEqual(addrPath, rawPath) } // stateId returns the ID that this resource should be entered with diff --git a/terraform/resource_address_test.go b/terraform/resource_address_test.go index c286984cc..c7dbfd274 100644 --- a/terraform/resource_address_test.go +++ b/terraform/resource_address_test.go @@ -5,6 +5,8 @@ import ( "reflect" "testing" + "github.com/hashicorp/terraform/addrs" + "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/configs" ) @@ -1046,7 +1048,7 @@ func TestResourceAddressWholeModuleAddress(t *testing.T) { } } -func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { +func TestResourceAddressMatchesResourceConfig(t *testing.T) { root := []string(nil) child := []string{"child"} grandchild := []string{"child", "grandchild"} @@ -1055,7 +1057,7 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { tests := []struct { Addr *ResourceAddress ModulePath []string - Resource *configs.ManagedResource + Resource *configs.Resource Want bool }{ { @@ -1066,7 +1068,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, root, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "null_resource", Name: "baz", }, @@ -1081,7 +1084,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, child, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "null_resource", Name: "baz", }, @@ -1096,7 +1100,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, grandchild, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "null_resource", Name: "baz", }, @@ -1108,7 +1113,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, child, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "null_resource", Name: "baz", }, @@ -1120,7 +1126,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, grandchild, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "null_resource", Name: "baz", }, @@ -1134,7 +1141,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, irrelevant, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "null_resource", Name: "baz", }, @@ -1148,7 +1156,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, irrelevant, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "null_resource", Name: "pizza", }, @@ -1162,7 +1171,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, irrelevant, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "aws_instance", Name: "baz", }, @@ -1177,7 +1187,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, child, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "null_resource", Name: "baz", }, @@ -1192,7 +1203,8 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { Index: -1, }, grandchild, - &configs.ManagedResource{ + &configs.Resource{ + Mode: addrs.ManagedResourceMode, Type: "null_resource", Name: "baz", }, @@ -1202,7 +1214,7 @@ func TestResourceAddressMatchesManagedResourceConfig(t *testing.T) { for i, test := range tests { t.Run(fmt.Sprintf("%02d-%s", i, test.Addr), func(t *testing.T) { - got := test.Addr.MatchesManagedResourceConfig(test.ModulePath, test.Resource) + got := test.Addr.MatchesResourceConfig(test.ModulePath, test.Resource) if got != test.Want { t.Errorf( "wrong result\naddr: %s\nmod: %#v\nrsrc: %#v\ngot: %#v\nwant: %#v",