From fa934d96d094e34a36ba728ded137dc17be932dd Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 21 Apr 2015 22:07:52 +0200 Subject: [PATCH 1/3] helper/schema: FieldReaderConfig detects computed maps --- helper/schema/field_reader_config.go | 53 ++++++++++++---- helper/schema/field_reader_config_test.go | 73 +++++++++++++++++++++++ terraform/resource.go | 9 +++ 3 files changed, 125 insertions(+), 10 deletions(-) diff --git a/helper/schema/field_reader_config.go b/helper/schema/field_reader_config.go index 3c01e8de2..a6edef099 100644 --- a/helper/schema/field_reader_config.go +++ b/helper/schema/field_reader_config.go @@ -105,34 +105,67 @@ func (r *ConfigFieldReader) readField( } func (r *ConfigFieldReader) readMap(k string) (FieldReadResult, error) { - mraw, ok := r.Config.Get(k) + // We want both the raw value and the interpolated. We use the interpolated + // to store actual values and we use the raw one to check for + // computed keys. + mraw, ok := r.Config.GetRaw(k) if !ok { return FieldReadResult{}, nil } result := make(map[string]interface{}) + computed := false switch m := mraw.(type) { case []interface{}: - for _, innerRaw := range m { - for k, v := range innerRaw.(map[string]interface{}) { - result[k] = v + for i, innerRaw := range m { + for ik, _ := range innerRaw.(map[string]interface{}) { + key := fmt.Sprintf("%s.%d.%s", k, i, ik) + if r.Config.IsComputed(key) { + computed = true + break + } + + v, _ := r.Config.Get(key) + result[ik] = v } } case []map[string]interface{}: - for _, innerRaw := range m { - for k, v := range innerRaw { - result[k] = v + for i, innerRaw := range m { + for ik, _ := range innerRaw { + key := fmt.Sprintf("%s.%d.%s", k, i, ik) + if r.Config.IsComputed(key) { + computed = true + break + } + + v, _ := r.Config.Get(key) + result[ik] = v } } case map[string]interface{}: - result = m + for ik, _ := range m { + key := fmt.Sprintf("%s.%s", k, ik) + if r.Config.IsComputed(key) { + computed = true + break + } + + v, _ := r.Config.Get(key) + result[ik] = v + } default: panic(fmt.Sprintf("unknown type: %#v", mraw)) } + var value interface{} + if !computed { + value = result + } + return FieldReadResult{ - Value: result, - Exists: true, + Value: value, + Exists: true, + Computed: computed, }, nil } diff --git a/helper/schema/field_reader_config_test.go b/helper/schema/field_reader_config_test.go index 33672eff2..96028a89c 100644 --- a/helper/schema/field_reader_config_test.go +++ b/helper/schema/field_reader_config_test.go @@ -135,6 +135,79 @@ func TestConfigFieldReader_DefaultHandling(t *testing.T) { } } +func TestConfigFieldReader_ComputedMap(t *testing.T) { + schema := map[string]*Schema{ + "map": &Schema{ + Type: TypeMap, + Computed: true, + }, + } + + cases := map[string]struct { + Addr []string + Result FieldReadResult + Config *terraform.ResourceConfig + Err bool + }{ + "set, normal": { + []string{"map"}, + FieldReadResult{ + Value: map[string]interface{}{ + "foo": "bar", + }, + Exists: true, + Computed: false, + }, + testConfig(t, map[string]interface{}{ + "map": map[string]interface{}{ + "foo": "bar", + }, + }), + false, + }, + + "computed element": { + []string{"map"}, + FieldReadResult{ + Exists: true, + Computed: true, + }, + testConfigInterpolate(t, map[string]interface{}{ + "map": map[string]interface{}{ + "foo": "${var.foo}", + }, + }, map[string]ast.Variable{ + "var.foo": ast.Variable{ + Value: config.UnknownVariableValue, + Type: ast.TypeString, + }, + }), + false, + }, + } + + for name, tc := range cases { + r := &ConfigFieldReader{ + Schema: schema, + Config: tc.Config, + } + out, err := r.ReadField(tc.Addr) + if (err != nil) != tc.Err { + t.Fatalf("%s: err: %s", name, err) + } + if s, ok := out.Value.(*Set); ok { + // If it is a set, convert to the raw map + out.Value = s.m + if len(s.m) == 0 { + out.Value = nil + } + } + if !reflect.DeepEqual(tc.Result, out) { + t.Fatalf("%s: bad: %#v", name, out) + } + } +} + func TestConfigFieldReader_ComputedSet(t *testing.T) { schema := map[string]*Schema{ "strSet": &Schema{ diff --git a/terraform/resource.go b/terraform/resource.go index 9dacbc7c5..5b9fbf7d2 100644 --- a/terraform/resource.go +++ b/terraform/resource.go @@ -134,6 +134,15 @@ func (c *ResourceConfig) Get(k string) (interface{}, bool) { return c.get(k, c.Raw) } +// GetRaw looks up a configuration value by key and returns the value, +// from the raw, uninterpolated config. +// +// The second return value is true if the get was successful. Get will +// not succeed if the value is being computed. +func (c *ResourceConfig) GetRaw(k string) (interface{}, bool) { + return c.get(k, c.Raw) +} + // IsComputed returns whether the given key is computed or not. func (c *ResourceConfig) IsComputed(k string) bool { _, ok := c.get(k, c.Config) From 9c10a89cf8eb61bb3cddeaadbe62a383a6cccfc2 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 21 Apr 2015 22:11:00 +0200 Subject: [PATCH 2/3] helper/schema: FieldReaderMap should mark map as exists if anything set --- helper/schema/field_reader_map.go | 3 ++- helper/schema/field_reader_map_test.go | 13 ++++++++++++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/helper/schema/field_reader_map.go b/helper/schema/field_reader_map.go index b2ed73605..a4345cbcd 100644 --- a/helper/schema/field_reader_map.go +++ b/helper/schema/field_reader_map.go @@ -56,10 +56,11 @@ func (r *MapFieldReader) readMap(k string) (FieldReadResult, error) { prefix := k + "." r.Map.Range(func(k, v string) bool { if strings.HasPrefix(k, prefix) { + resultSet = true + key := k[len(prefix):] if key != "#" { result[key] = v - resultSet = true } } diff --git a/helper/schema/field_reader_map_test.go b/helper/schema/field_reader_map_test.go index ae4e69b5e..e2d5342ee 100644 --- a/helper/schema/field_reader_map_test.go +++ b/helper/schema/field_reader_map_test.go @@ -49,11 +49,14 @@ func TestMapFieldReader(t *testing.T) { func TestMapFieldReader_extra(t *testing.T) { r := &MapFieldReader{ Schema: map[string]*Schema{ - "mapDel": &Schema{Type: TypeMap}, + "mapDel": &Schema{Type: TypeMap}, + "mapEmpty": &Schema{Type: TypeMap}, }, Map: BasicMapReader(map[string]string{ "mapDel": "", + + "mapEmpty.#": "0", }), } @@ -71,6 +74,14 @@ func TestMapFieldReader_extra(t *testing.T) { false, false, }, + + "mapEmpty": { + []string{"mapEmpty"}, + map[string]interface{}{}, + true, + false, + false, + }, } for name, tc := range cases { From 51951d68f4ecff19acea881bda2c5efa34e57c93 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 21 Apr 2015 22:13:03 +0200 Subject: [PATCH 3/3] helper/schema: change diff logic around maps to fix case #57 and #44 --- helper/schema/schema.go | 26 +++++++++++++++++++------- helper/schema/schema_test.go | 23 +++++++++++++++++++++++ 2 files changed, 42 insertions(+), 7 deletions(-) diff --git a/helper/schema/schema.go b/helper/schema/schema.go index 33c8b8310..b2b8a5cb7 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -626,7 +626,7 @@ func (m schemaMap) diffMap( // First get all the values from the state var stateMap, configMap map[string]string - o, n, _, _ := d.diffChange(k) + o, n, _, nComputed := d.diffChange(k) if err := mapstructure.WeakDecode(o, &stateMap); err != nil { return fmt.Errorf("%s: %s", k, err) } @@ -634,28 +634,40 @@ func (m schemaMap) diffMap( return fmt.Errorf("%s: %s", k, err) } + // Keep track of whether the state _exists_ at all prior to clearing it + stateExists := o != nil + // Delete any count values, since we don't use those delete(configMap, "#") delete(stateMap, "#") - // Check if the number of elements has changed. If we're computing - // a list and there isn't a config, then it hasn't changed. + // Check if the number of elements has changed. oldLen, newLen := len(stateMap), len(configMap) changed := oldLen != newLen if oldLen != 0 && newLen == 0 && schema.Computed { changed = false } - computed := oldLen == 0 && newLen == 0 && schema.Computed - if changed || computed { + + // It is computed if we have no old value, no new value, the schema + // says it is computed, and it didn't exist in the state before. The + // last point means: if it existed in the state, even empty, then it + // has already been computed. + computed := oldLen == 0 && newLen == 0 && schema.Computed && !stateExists + + // If the count has changed or we're computed, then add a diff for the + // count. "nComputed" means that the new value _contains_ a value that + // is computed. We don't do granular diffs for this yet, so we mark the + // whole map as computed. + if changed || computed || nComputed { countSchema := &Schema{ Type: TypeInt, - Computed: schema.Computed, + Computed: schema.Computed || nComputed, ForceNew: schema.ForceNew, } oldStr := strconv.FormatInt(int64(oldLen), 10) newStr := "" - if !computed { + if !computed && !nComputed { newStr = strconv.FormatInt(int64(newLen), 10) } else { oldStr = "" diff --git a/helper/schema/schema_test.go b/helper/schema/schema_test.go index c1233ae50..6ff334e69 100644 --- a/helper/schema/schema_test.go +++ b/helper/schema/schema_test.go @@ -2201,6 +2201,29 @@ func TestSchemaMap_Diff(t *testing.T) { Err: false, }, + + // #57 - Computed map without config that's known to be empty does not + // generate diff + { + Schema: map[string]*Schema{ + "tags": &Schema{ + Type: TypeMap, + Computed: true, + }, + }, + + Config: nil, + + State: &terraform.InstanceState{ + Attributes: map[string]string{ + "tags.#": "0", + }, + }, + + Diff: nil, + + Err: false, + }, } for i, tc := range cases {