configs: allow overrides files to omit args that primary files can't

Some of the fields in our config structs are either mandatory in primary
files or there is a default value that we apply if absent.

Unfortunately override files impose the additional constraint that we
be allowed to omit required fields (which have presumably already been
set in the primary files) and that we are able to distinguish between a
default value and omitting a value entirely.

Since most of our fields were already acceptable for override files, here
we just add some new fields to deal with the few cases where special
handling is required and a helper function to disable the "Required" flag
on attributes in a given schema.

configs/module_call.go

@@ -12,6 +12,7 @@ type ModuleCall struct {
 
 	SourceAddr      string
 	SourceAddrRange hcl.Range
+	SourceSet       bool
 
 	Config hcl.Body
 
@@ -25,13 +26,18 @@ type ModuleCall struct {
 	DeclRange hcl.Range
 }
 
-func decodeModuleBlock(block *hcl.Block) (*ModuleCall, hcl.Diagnostics) {
+func decodeModuleBlock(block *hcl.Block, override bool) (*ModuleCall, hcl.Diagnostics) {
 	mc := &ModuleCall{
 		Name:      block.Labels[0],
 		DeclRange: block.DefRange,
 	}
 
-	content, remain, diags := block.Body.PartialContent(moduleBlockSchema)
+	schema := moduleBlockSchema
+	if override {
+		schema = schemaForOverrides(schema)
+	}
+
+	content, remain, diags := block.Body.PartialContent(schema)
 	mc.Config = remain
 
 	if !hclsyntax.ValidIdentifier(mc.Name) {
@@ -47,6 +53,7 @@ func decodeModuleBlock(block *hcl.Block) (*ModuleCall, hcl.Diagnostics) {
 		valDiags := gohcl.DecodeExpression(attr.Expr, nil, &mc.SourceAddr)
 		diags = append(diags, valDiags...)
 		mc.SourceAddrRange = attr.Expr.Range()
+		mc.SourceSet = true
 	}
 
 	if attr, exists := content.Attributes["version"]; exists {

configs/module_merge.go

@@ -45,8 +45,9 @@ func mergeProviderVersionConstraints(recv map[string][]VersionConstraint, ovrd [
 func (v *Variable) merge(ov *Variable) hcl.Diagnostics {
 	var diags hcl.Diagnostics
 
-	if ov.Description != "" {
+	if ov.DescriptionSet {
 		v.Description = ov.Description
+		v.DescriptionSet = ov.DescriptionSet
 	}
 	if ov.Default != cty.NilVal {
 		v.Default = ov.Default
@@ -80,10 +81,9 @@ func (o *Output) merge(oo *Output) hcl.Diagnostics {
 	if oo.Expr != nil {
 		o.Expr = oo.Expr
 	}
-	if oo.Sensitive {
+	if oo.SensitiveSet {
-		// Since this is just a bool, we can't distinguish false from unset
-		// and so the override can only make the output _more_ sensitive.
 		o.Sensitive = oo.Sensitive
+		o.SensitiveSet = oo.SensitiveSet
 	}
 
 	// We don't allow depends_on to be overridden because that is likely to
@@ -103,9 +103,10 @@ func (o *Output) merge(oo *Output) hcl.Diagnostics {
 func (mc *ModuleCall) merge(omc *ModuleCall) hcl.Diagnostics {
 	var diags hcl.Diagnostics
 
-	if omc.SourceAddr != "" {
+	if omc.SourceSet {
 		mc.SourceAddr = omc.SourceAddr
 		mc.SourceAddrRange = omc.SourceAddrRange
+		mc.SourceSet = omc.SourceSet
 	}
 
 	if omc.Count != nil {
@@ -145,9 +146,9 @@ func (r *ManagedResource) merge(or *ManagedResource) hcl.Diagnostics {
 	if or.Count != nil {
 		r.Count = or.Count
 	}
-	if or.CreateBeforeDestroy {
+	if or.CreateBeforeDestroySet {
-		// We can't distinguish false from unset here
 		r.CreateBeforeDestroy = or.CreateBeforeDestroy
+		r.CreateBeforeDestroySet = or.CreateBeforeDestroySet
 	}
 	if or.ForEach != nil {
 		r.ForEach = or.ForEach
@@ -155,9 +156,9 @@ func (r *ManagedResource) merge(or *ManagedResource) hcl.Diagnostics {
 	if len(or.IgnoreChanges) != 0 {
 		r.IgnoreChanges = or.IgnoreChanges
 	}
-	if or.PreventDestroy {
+	if or.PreventDestroySet {
-		// We can't distinguish false from unset here
 		r.PreventDestroy = or.PreventDestroy
+		r.PreventDestroySet = or.PreventDestroySet
 	}
 	if or.ProviderConfigRef != nil {
 		r.ProviderConfigRef = or.ProviderConfigRef

configs/module_merge_test.go

@@ -0,0 +1,61 @@
+package configs
+
+import (
+	"testing"
+
+	"github.com/hashicorp/hcl2/hcl"
+	"github.com/zclconf/go-cty/cty"
+)
+
+func TestModuleOverrideVariable(t *testing.T) {
+	mod, diags := testModuleFromDir("test-fixtures/valid-modules/override-variable")
+	assertNoDiagnostics(t, diags)
+	if mod == nil {
+		t.Fatalf("module is nil")
+	}
+
+	got := mod.Variables
+	want := map[string]*Variable{
+		"fully_overridden": {
+			Name:           "fully_overridden",
+			Description:    "b_override description",
+			DescriptionSet: true,
+			Default:        cty.StringVal("b_override"),
+			TypeHint:       TypeHintString,
+			DeclRange: hcl.Range{
+				Filename: "test-fixtures/valid-modules/override-variable/primary.tf",
+				Start: hcl.Pos{
+					Line:   1,
+					Column: 1,
+					Byte:   0,
+				},
+				End: hcl.Pos{
+					Line:   1,
+					Column: 28,
+					Byte:   27,
+				},
+			},
+		},
+		"partially_overridden": {
+			Name:           "partially_overridden",
+			Description:    "base description",
+			DescriptionSet: true,
+			Default:        cty.StringVal("b_override partial"),
+			TypeHint:       TypeHintString,
+			DeclRange: hcl.Range{
+				Filename: "test-fixtures/valid-modules/override-variable/primary.tf",
+				Start: hcl.Pos{
+					Line:   7,
+					Column: 1,
+					Byte:   103,
+				},
+				End: hcl.Pos{
+					Line:   7,
+					Column: 32,
+					Byte:   134,
+				},
+			},
+		},
+	}
+	assertResultDeepEqual(t, got, want)
+}

configs/named_values.go

@@ -19,6 +19,8 @@ type Variable struct {
 	Default     cty.Value
 	TypeHint    VariableTypeHint
 
+	DescriptionSet bool
+
 	DeclRange hcl.Range
 }
 
@@ -56,6 +58,7 @@ func decodeVariableBlock(block *hcl.Block) (*Variable, hcl.Diagnostics) {
 	if attr, exists := content.Attributes["description"]; exists {
 		valDiags := gohcl.DecodeExpression(attr.Expr, nil, &v.Description)
 		diags = append(diags, valDiags...)
+		v.DescriptionSet = true
 	}
 
 	if attr, exists := content.Attributes["default"]; exists {
@@ -106,16 +109,24 @@ type Output struct {
 	DependsOn   []hcl.Traversal
 	Sensitive   bool
 
+	DescriptionSet bool
+	SensitiveSet   bool
+
 	DeclRange hcl.Range
 }
 
-func decodeOutputBlock(block *hcl.Block) (*Output, hcl.Diagnostics) {
+func decodeOutputBlock(block *hcl.Block, override bool) (*Output, hcl.Diagnostics) {
 	o := &Output{
 		Name:      block.Labels[0],
 		DeclRange: block.DefRange,
 	}
 
-	content, diags := block.Body.Content(outputBlockSchema)
+	schema := outputBlockSchema
+	if override {
+		schema = schemaForOverrides(schema)
+	}
+
+	content, diags := block.Body.Content(schema)
 
 	if !hclsyntax.ValidIdentifier(o.Name) {
 		diags = append(diags, &hcl.Diagnostic{
@@ -129,6 +140,7 @@ func decodeOutputBlock(block *hcl.Block) (*Output, hcl.Diagnostics) {
 	if attr, exists := content.Attributes["description"]; exists {
 		valDiags := gohcl.DecodeExpression(attr.Expr, nil, &o.Description)
 		diags = append(diags, valDiags...)
+		o.DescriptionSet = true
 	}
 
 	if attr, exists := content.Attributes["value"]; exists {
@@ -138,6 +150,7 @@ func decodeOutputBlock(block *hcl.Block) (*Output, hcl.Diagnostics) {
 	if attr, exists := content.Attributes["sensitive"]; exists {
 		valDiags := gohcl.DecodeExpression(attr.Expr, nil, &o.Sensitive)
 		diags = append(diags, valDiags...)
+		o.SensitiveSet = true
 	}
 
 	if attr, exists := content.Attributes["depends_on"]; exists {

configs/parser_config.go

@@ -19,6 +19,18 @@ import (
 // This method wraps LoadHCLFile, and so it inherits the syntax selection
 // behaviors documented for that method.
 func (p *Parser) LoadConfigFile(path string) (*File, hcl.Diagnostics) {
+	return p.loadConfigFile(path, false)
+}
+
+// LoadConfigFileOverride is the same as LoadConfigFile except that it relaxes
+// certain required attribute constraints in order to interpret the given
+// file as an overrides file.
+func (p *Parser) LoadConfigFileOverride(path string) (*File, hcl.Diagnostics) {
+	return p.loadConfigFile(path, true)
+}
+
+func (p *Parser) loadConfigFile(path string, override bool) (*File, hcl.Diagnostics) {
+
 	body, diags := p.LoadHCLFile(path)
 	if body == nil {
 		return nil, diags
@@ -86,14 +98,14 @@ func (p *Parser) LoadConfigFile(path string) (*File, hcl.Diagnostics) {
 			file.Locals = append(file.Locals, defs...)
 
 		case "output":
-			cfg, cfgDiags := decodeOutputBlock(block)
+			cfg, cfgDiags := decodeOutputBlock(block, override)
 			diags = append(diags, cfgDiags...)
 			if cfg != nil {
 				file.Outputs = append(file.Outputs, cfg)
 			}
 
 		case "module":
-			cfg, cfgDiags := decodeModuleBlock(block)
+			cfg, cfgDiags := decodeModuleBlock(block, override)
 			diags = append(diags, cfgDiags...)
 			if cfg != nil {
 				file.ModuleCalls = append(file.ModuleCalls, cfg)

configs/parser_config_dir.go

@@ -33,9 +33,9 @@ func (p *Parser) LoadConfigDir(path string) (*Module, hcl.Diagnostics) {
 		return nil, diags
 	}
 
-	primary, fDiags := p.loadFiles(primaryPaths)
+	primary, fDiags := p.loadFiles(primaryPaths, false)
 	diags = append(diags, fDiags...)
-	override, fDiags := p.loadFiles(overridePaths)
+	override, fDiags := p.loadFiles(overridePaths, true)
 	diags = append(diags, fDiags...)
 
 	mod, modDiags := NewModule(primary, override)
@@ -52,12 +52,18 @@ func (p *Parser) IsConfigDir(path string) bool {
 	return (len(primaryPaths) + len(overridePaths)) > 0
 }
 
-func (p *Parser) loadFiles(paths []string) ([]*File, hcl.Diagnostics) {
+func (p *Parser) loadFiles(paths []string, override bool) ([]*File, hcl.Diagnostics) {
 	var files []*File
 	var diags hcl.Diagnostics
 
 	for _, path := range paths {
-		f, fDiags := p.LoadConfigFile(path)
+		var f *File
+		var fDiags hcl.Diagnostics
+		if override {
+			f, fDiags = p.LoadConfigFileOverride(path)
+		} else {
+			f, fDiags = p.LoadConfigFile(path)
+		}
 		diags = append(diags, fDiags...)
 		if f != nil {
 			files = append(files, f)

configs/parser_test.go

@@ -3,7 +3,12 @@ package configs
 import (
 	"os"
 	"path"
+	"reflect"
+	"testing"
 
+	"github.com/davecgh/go-spew/spew"
+
+	"github.com/hashicorp/hcl2/hcl"
 	"github.com/spf13/afero"
 )
 
@@ -29,3 +34,62 @@ func testParser(files map[string]string) *Parser {
 
 	return NewParser(fs)
 }
+
+// testModuleFromFile reads a single file, wraps it in a module, and returns
+// it. This is a helper for use in unit tests.
+func testModuleFromFile(filename string) (*Module, hcl.Diagnostics) {
+	parser := NewParser(nil)
+	f, diags := parser.LoadConfigFile(filename)
+	mod, modDiags := NewModule([]*File{f}, nil)
+	diags = append(diags, modDiags...)
+	return mod, modDiags
+}
+
+// testModuleFromDir reads configuration from the given directory path as
+// a module and returns it. This is a helper for use in unit tests.
+func testModuleFromDir(path string) (*Module, hcl.Diagnostics) {
+	parser := NewParser(nil)
+	return parser.LoadConfigDir(path)
+}
+
+func assertNoDiagnostics(t *testing.T, diags hcl.Diagnostics) bool {
+	t.Helper()
+	return assertDiagnosticCount(t, diags, 0)
+}
+
+func assertDiagnosticCount(t *testing.T, diags hcl.Diagnostics, want int) bool {
+	t.Helper()
+	if len(diags) != 0 {
+		t.Errorf("wrong number of diagnostics %d; want %d", len(diags), want)
+		for _, diag := range diags {
+			t.Logf("- %s", diag)
+		}
+		return true
+	}
+	return false
+}
+
+func assertDiagnosticSummary(t *testing.T, diags hcl.Diagnostics, want string) bool {
+	t.Helper()
+
+	for _, diag := range diags {
+		if diag.Summary == want {
+			return false
+		}
+	}
+
+	t.Errorf("missing diagnostic summary %q", want)
+	for _, diag := range diags {
+		t.Logf("- %s", diag)
+	}
+	return true
+}
+
+func assertResultDeepEqual(t *testing.T, got, want interface{}) bool {
+	t.Helper()
+	if !reflect.DeepEqual(got, want) {
+		t.Errorf("wrong result\ngot: %swant: %s", spew.Sdump(got), spew.Sdump(want))
+		return true
+	}
+	return false
+}

configs/resource.go

@@ -27,6 +27,9 @@ type ManagedResource struct {
 	PreventDestroy      bool
 	IgnoreChanges       []hcl.Traversal
 
+	CreateBeforeDestroySet bool
+	PreventDestroySet      bool
+
 	DeclRange hcl.Range
 	TypeRange hcl.Range
 }
@@ -105,11 +108,13 @@ func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) {
 			if attr, exists := lcContent.Attributes["create_before_destroy"]; exists {
 				valDiags := gohcl.DecodeExpression(attr.Expr, nil, &r.CreateBeforeDestroy)
 				diags = append(diags, valDiags...)
+				r.CreateBeforeDestroySet = true
 			}
 
 			if attr, exists := lcContent.Attributes["prevent_destroy"]; exists {
 				valDiags := gohcl.DecodeExpression(attr.Expr, nil, &r.PreventDestroy)
 				diags = append(diags, valDiags...)
+				r.PreventDestroySet = true
 			}
 
 			if attr, exists := lcContent.Attributes["ignore_changes"]; exists {

configs/test-fixtures/valid-modules/override-module/a_override.tf

@@ -0,0 +1,5 @@
+
+module "example" {
+  foo = "a_override foo"
+  new = "a_override new"
+}

configs/test-fixtures/valid-modules/override-module/b_override.tf

@@ -0,0 +1,5 @@
+
+module "example" {
+  new   = "b_override new"
+  newer = "b_override newer"
+}

configs/test-fixtures/valid-modules/override-module/primary.tf

@@ -0,0 +1,7 @@
+
+module "example" {
+  source = "./example2"
+
+  kept = "primary kept"
+  foo  = "primary foo"
+}

configs/test-fixtures/valid-modules/override-output-sensitive/override.tf

@@ -0,0 +1,3 @@
+output "foo" {
+  sensitive = true
+}

configs/test-fixtures/valid-modules/override-output-sensitive/primary.tf

@@ -0,0 +1,3 @@
+output "foo" {
+  value = "Hello World"
+}

configs/util.go

@@ -16,3 +16,30 @@ func exprIsNativeQuotedString(expr hcl.Expression) bool {
 	_, ok := expr.(*hclsyntax.TemplateExpr)
 	return ok
 }
+
+// schemaForOverrides takes a *hcl.BodySchema and produces a new one that is
+// equivalent except that any required attributes are forced to not be required.
+//
+// This is useful for dealing with "override" config files, which are allowed
+// to omit things that they don't wish to override from the main configuration.
+//
+// The returned schema may have some pointers in common with the given schema,
+// so neither the given schema nor the returned schema should be modified after
+// using this function in order to avoid confusion.
+//
+// Overrides are rarely used, so it's recommended to just create the override
+// schema on the fly only when it's needed, rather than storing it in a global
+// variable as we tend to do for a primary schema.
+func schemaForOverrides(schema *hcl.BodySchema) *hcl.BodySchema {
+	ret := &hcl.BodySchema{
+		Attributes: make([]hcl.AttributeSchema, len(schema.Attributes)),
+		Blocks:     schema.Blocks,
+	}
+
+	for i, attrS := range schema.Attributes {
+		ret.Attributes[i] = attrS
+		ret.Attributes[i].Required = false
+	}
+
+	return ret
+}