config: Merge works properly

config/config.go

@@ -251,6 +251,84 @@ func (c *Config) allVariables() map[string][]InterpolatedVariable {
 	return result
 }
 
+func (o *Output) mergerName() string {
+	return o.Name
+}
+
+func (o *Output) mergerMerge(m merger) merger {
+	o2 := m.(*Output)
+
+	result := *o
+	result.Name = o2.Name
+	result.RawConfig = result.RawConfig.merge(o2.RawConfig)
+
+	return &result
+}
+
+func (c *ProviderConfig) mergerName() string {
+	return c.Name
+}
+
+func (c *ProviderConfig) mergerMerge(m merger) merger {
+	c2 := m.(*ProviderConfig)
+
+	result := *c
+	result.Name = c2.Name
+	result.RawConfig = result.RawConfig.merge(c2.RawConfig)
+
+	return &result
+}
+
+func (r *Resource) mergerName() string {
+	return fmt.Sprintf("%s.%s", r.Type, r.Name)
+}
+
+func (r *Resource) mergerMerge(m merger) merger {
+	r2 := m.(*Resource)
+
+	result := *r
+	result.Name = r2.Name
+	result.Type = r2.Type
+	result.RawConfig = result.RawConfig.merge(r2.RawConfig)
+
+	if r2.Count > 0 {
+		result.Count = r2.Count
+	}
+
+	if len(r2.Provisioners) > 0 {
+		result.Provisioners = r2.Provisioners
+	}
+
+	return &result
+}
+
+// Merge merges two variables to create a new third variable.
+func (v *Variable) Merge(v2 *Variable) *Variable {
+	// Shallow copy the variable
+	result := *v
+
+	// The names should be the same, but the second name always wins.
+	result.Name = v2.Name
+
+	if v2.defaultSet {
+		result.Default = v2.Default
+		result.defaultSet = true
+	}
+	if v2.Description != "" {
+		result.Description = v2.Description
+	}
+
+	return &result
+}
+
+func (v *Variable) mergerName() string {
+	return v.Name
+}
+
+func (v *Variable) mergerMerge(m merger) merger {
+	return v.Merge(m.(*Variable))
+}
+
 // Required tests whether a variable is required or not.
 func (v *Variable) Required() bool {
 	return !v.defaultSet

config/loader_test.go

@@ -505,6 +505,7 @@ foo
 
 const importProvidersStr = `
 aws
+  bar
   foo
 `
 

config/merge.go

@@ -1,9 +1,5 @@
 package config
 
-import (
-	"fmt"
-)
-
 // Merge merges two configurations into a single configuration.
 //
 // Merge allows for the two configurations to have duplicate resources,
@@ -24,81 +20,135 @@ func Merge(c1, c2 *Config) (*Config, error) {
 		c.unknownKeys = append(c.unknownKeys, k)
 	}
 
-	// Merge variables: Variable merging is quite simple. Set fields in
-	// later set variables override those earlier.
-	if len(c1.Variables) > 0 || len(c2.Variables) > 0 {
-		c.Variables = make([]*Variable, 0, len(c1.Variables)+len(c2.Variables))
-		varMap := make(map[string]*Variable)
-		for _, v := range c1.Variables {
-			varMap[v.Name] = v
-		}
-		for _, v2 := range c2.Variables {
-			v1, ok := varMap[v2.Name]
-			if ok {
-				if v2.Default == "" {
-					v2.Default = v1.Default
-				}
-				if v2.Description == "" {
-					v2.Description = v1.Description
-				}
-			}
+	// NOTE: Everything below is pretty gross. Due to the lack of generics
+	// in Go, there is some hoop-jumping involved to make this merging a
+	// little more test-friendly and less repetitive. Ironically, making it
+	// less repetitive involves being a little repetitive, but I prefer to
+	// be repetitive with things that are less error prone than things that
+	// are more error prone (more logic). Type conversions to an interface
+	// are pretty low-error.
 
-			varMap[v2.Name] = v2
-		}
-		for _, v := range varMap {
-			c.Variables = append(c.Variables, v)
+	var m1, m2, mresult []merger
+
+	// Outputs
+	m1 = make([]merger, 0, len(c1.Outputs))
+	m2 = make([]merger, 0, len(c2.Outputs))
+	for _, v := range c1.Outputs {
+		m1 = append(m1, v)
+	}
+	for _, v := range c2.Outputs {
+		m2 = append(m2, v)
+	}
+	mresult = mergeSlice(m1, m2)
+	if len(mresult) > 0 {
+		c.Outputs = make([]*Output, len(mresult))
+		for i, v := range mresult {
+			c.Outputs[i] = v.(*Output)
 		}
 	}
 
-	// Merge outputs: If they collide, just take the latest one for now. In
-	// the future, we might provide smarter merge functionality.
-	if len(c1.Outputs) > 0 || len(c2.Outputs) > 0 {
-		c.Outputs = make([]*Output, 0, len(c1.Outputs)+len(c2.Outputs))
-		m := make(map[string]*Output)
-		for _, v := range c1.Outputs {
-			m[v.Name] = v
-		}
-		for _, v := range c2.Outputs {
-			m[v.Name] = v
+	// Provider Configs
+	m1 = make([]merger, 0, len(c1.ProviderConfigs))
+	m2 = make([]merger, 0, len(c2.ProviderConfigs))
+	for _, v := range c1.ProviderConfigs {
+		m1 = append(m1, v)
+	}
+	for _, v := range c2.ProviderConfigs {
+		m2 = append(m2, v)
+	}
+	mresult = mergeSlice(m1, m2)
+	if len(mresult) > 0 {
+		c.ProviderConfigs = make([]*ProviderConfig, len(mresult))
+		for i, v := range mresult {
+			c.ProviderConfigs[i] = v.(*ProviderConfig)
 		}
-		for _, v := range m {
-			c.Outputs = append(c.Outputs, v)
+	}
+
+	// Resources
+	m1 = make([]merger, 0, len(c1.Resources))
+	m2 = make([]merger, 0, len(c2.Resources))
+	for _, v := range c1.Resources {
+		m1 = append(m1, v)
+	}
+	for _, v := range c2.Resources {
+		m2 = append(m2, v)
+	}
+	mresult = mergeSlice(m1, m2)
+	if len(mresult) > 0 {
+		c.Resources = make([]*Resource, len(mresult))
+		for i, v := range mresult {
+			c.Resources[i] = v.(*Resource)
 		}
 	}
 
-	// Merge provider configs: If they collide, we just take the latest one
-	// for now. In the future, we might provide smarter merge functionality.
-	if len(c1.ProviderConfigs) > 0 || len(c2.ProviderConfigs) > 0 {
-		m := make(map[string]*ProviderConfig)
-		for _, v := range c1.ProviderConfigs {
-			m[v.Name] = v
+	// Variables
+	m1 = make([]merger, 0, len(c1.Variables))
+	m2 = make([]merger, 0, len(c2.Variables))
+	for _, v := range c1.Variables {
+		m1 = append(m1, v)
+	}
+	for _, v := range c2.Variables {
+		m2 = append(m2, v)
+	}
+	mresult = mergeSlice(m1, m2)
+	if len(mresult) > 0 {
+		c.Variables = make([]*Variable, len(mresult))
+		for i, v := range mresult {
+			c.Variables[i] = v.(*Variable)
 		}
-		for _, v := range c2.ProviderConfigs {
-			m[v.Name] = v
+	}
+
+	return c, nil
+}
+
+// merger is an interface that must be implemented by types that are
+// merge-able. This simplifies the implementation of Merge for the various
+// components of a Config.
+type merger interface {
+	mergerName() string
+	mergerMerge(merger) merger
+}
+
+// mergeSlice merges a slice of mergers.
+func mergeSlice(m1, m2 []merger) []merger {
+	r := make([]merger, len(m1), len(m1)+len(m2))
+	copy(r, m1)
+
+	m := map[string]struct{}{}
+	for _, v2 := range m2 {
+		// If we already saw it, just append it because its a
+		// duplicate and invalid...
+		name := v2.mergerName()
+		if _, ok := m[name]; ok {
+			r = append(r, v2)
+			continue
 		}
+		m[name] = struct{}{}
 
-		c.ProviderConfigs = make([]*ProviderConfig, 0, len(m))
-		for _, v := range m {
-			c.ProviderConfigs = append(c.ProviderConfigs, v)
+		// Find an original to override
+		var original merger
+		originalIndex := -1
+		for i, v := range m1 {
+			if v.mergerName() == name {
+				originalIndex = i
+				original = v
+				break
+			}
 		}
-	}
 
-	// Merge resources: If they collide, we just take the latest one
-	// for now. In the future, we might provide smarter merge functionality.
-	resources := make(map[string]*Resource)
-	for _, r := range c1.Resources {
-		id := fmt.Sprintf("%s[%s]", r.Type, r.Name)
-		resources[id] = r
-	}
-	for _, r := range c2.Resources {
-		id := fmt.Sprintf("%s[%s]", r.Type, r.Name)
-		resources[id] = r
-	}
+		var v merger
+		if original == nil {
+			v = v2
+		} else {
+			v = original.mergerMerge(v2)
+		}
 
-	c.Resources = make([]*Resource, 0, len(resources))
-	for _, r := range resources {
-		c.Resources = append(c.Resources, r)
+		if originalIndex == -1 {
+			r = append(r, v)
+		} else {
+			r[originalIndex] = v
+		}
 	}
 
-	return c, nil
+	return r
 }

config/merge_test.go

@@ -0,0 +1,149 @@
+package config
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestMerge(t *testing.T) {
+	cases := []struct {
+		c1, c2, result *Config
+		err            bool
+	}{
+		// Normal good case.
+		{
+			&Config{
+				Outputs: []*Output{
+					&Output{Name: "foo"},
+				},
+				ProviderConfigs: []*ProviderConfig{
+					&ProviderConfig{Name: "foo"},
+				},
+				Resources: []*Resource{
+					&Resource{Name: "foo"},
+				},
+				Variables: []*Variable{
+					&Variable{Name: "foo"},
+				},
+
+				unknownKeys: []string{"foo"},
+			},
+
+			&Config{
+				Outputs: []*Output{
+					&Output{Name: "bar"},
+				},
+				ProviderConfigs: []*ProviderConfig{
+					&ProviderConfig{Name: "bar"},
+				},
+				Resources: []*Resource{
+					&Resource{Name: "bar"},
+				},
+				Variables: []*Variable{
+					&Variable{Name: "bar"},
+				},
+
+				unknownKeys: []string{"bar"},
+			},
+
+			&Config{
+				Outputs: []*Output{
+					&Output{Name: "foo"},
+					&Output{Name: "bar"},
+				},
+				ProviderConfigs: []*ProviderConfig{
+					&ProviderConfig{Name: "foo"},
+					&ProviderConfig{Name: "bar"},
+				},
+				Resources: []*Resource{
+					&Resource{Name: "foo"},
+					&Resource{Name: "bar"},
+				},
+				Variables: []*Variable{
+					&Variable{Name: "foo"},
+					&Variable{Name: "bar"},
+				},
+
+				unknownKeys: []string{"foo", "bar"},
+			},
+
+			false,
+		},
+
+		// Test that when merging duplicates, it merges into the
+		// first, but keeps the duplicates so that errors still
+		// happen.
+		{
+			&Config{
+				Outputs: []*Output{
+					&Output{Name: "foo"},
+				},
+				ProviderConfigs: []*ProviderConfig{
+					&ProviderConfig{Name: "foo"},
+				},
+				Resources: []*Resource{
+					&Resource{Name: "foo"},
+				},
+				Variables: []*Variable{
+					&Variable{Name: "foo", Default: "foo"},
+					&Variable{Name: "foo"},
+				},
+
+				unknownKeys: []string{"foo"},
+			},
+
+			&Config{
+				Outputs: []*Output{
+					&Output{Name: "bar"},
+				},
+				ProviderConfigs: []*ProviderConfig{
+					&ProviderConfig{Name: "bar"},
+				},
+				Resources: []*Resource{
+					&Resource{Name: "bar"},
+				},
+				Variables: []*Variable{
+					&Variable{Name: "foo", Default: "bar", defaultSet: true},
+					&Variable{Name: "bar"},
+				},
+
+				unknownKeys: []string{"bar"},
+			},
+
+			&Config{
+				Outputs: []*Output{
+					&Output{Name: "foo"},
+					&Output{Name: "bar"},
+				},
+				ProviderConfigs: []*ProviderConfig{
+					&ProviderConfig{Name: "foo"},
+					&ProviderConfig{Name: "bar"},
+				},
+				Resources: []*Resource{
+					&Resource{Name: "foo"},
+					&Resource{Name: "bar"},
+				},
+				Variables: []*Variable{
+					&Variable{Name: "foo", Default: "bar", defaultSet: true},
+					&Variable{Name: "foo"},
+					&Variable{Name: "bar"},
+				},
+
+				unknownKeys: []string{"foo", "bar"},
+			},
+
+			false,
+		},
+	}
+
+	for i, tc := range cases {
+		actual, err := Merge(tc.c1, tc.c2)
+		if (err != nil) != tc.err {
+			t.Fatalf("%d: error fail", i)
+		}
+
+		if !reflect.DeepEqual(actual, tc.result) {
+			t.Fatalf("%d: bad:\n\n%#v", i, actual)
+		}
+	}
+}

config/raw_config.go

@@ -92,6 +92,25 @@ func (r *RawConfig) init() error {
 	return nil
 }
 
+func (r *RawConfig) merge(r2 *RawConfig) *RawConfig {
+	rawRaw, err := copystructure.Copy(r.Raw)
+	if err != nil {
+		panic(err)
+	}
+
+	raw := rawRaw.(map[string]interface{})
+	for k, v := range r2.Raw {
+		raw[k] = v
+	}
+
+	result, err := NewRawConfig(raw)
+	if err != nil {
+		panic(err)
+	}
+
+	return result
+}
+
 // UnknownKeys returns the keys of the configuration that are unknown
 // because they had interpolated variables that must be computed.
 func (r *RawConfig) UnknownKeys() []string {