Merge pull request #4796 from svanharmelen/f-tweak-tags

provider/cloudstack: policing up the new `tags` code from @Carles-Figuerola

builtin/providers/cloudstack/resource_cloudstack_network.go

@@ -178,7 +178,7 @@ func resourceCloudStackNetworkCreate(d *schema.ResourceData, meta interface{}) e
 
 	err = setTags(cs, d, "network")
 	if err != nil {
-		return fmt.Errorf("Error setting tags")
+		return fmt.Errorf("Error setting tags: %s", err)
 	}
 
 	return resourceCloudStackNetworkRead(d, meta)
@@ -206,7 +206,7 @@ func resourceCloudStackNetworkRead(d *schema.ResourceData, meta interface{}) err
 	d.Set("gateway", n.Gateway)
 
 	// Read the tags and store them in a map
-	tags := make(map[string]string)
+	tags := make(map[string]interface{})
 	for item := range n.Tags {
 		tags[n.Tags[item].Key] = n.Tags[item].Value
 	}
@@ -262,9 +262,11 @@ func resourceCloudStackNetworkUpdate(d *schema.ResourceData, meta interface{}) e
 	}
 
 	// Update tags if they have changed
+	if d.HasChange("tags") {
 		err = setTags(cs, d, "network")
 		if err != nil {
-		return fmt.Errorf("Error updating tags")
+			return fmt.Errorf("Error updating tags: %s", err)
+		}
 	}
 
 	return resourceCloudStackNetworkRead(d, meta)

builtin/providers/cloudstack/tags.go

@@ -7,27 +7,27 @@ import (
 	"github.com/xanzy/go-cloudstack/cloudstack"
 )
 
-// tagsSchema returns the schema to use for tags.
+// tagsSchema returns the schema to use for tags
-//
 func tagsSchema() *schema.Schema {
 	return &schema.Schema{
 		Type:     schema.TypeMap,
 		Optional: true,
+		Computed: true,
 	}
 }
 
 // setTags is a helper to set the tags for a resource. It expects the
 // tags field to be named "tags"
 func setTags(cs *cloudstack.CloudStackClient, d *schema.ResourceData, resourcetype string) error {
-	if d.HasChange("tags") {
 	oraw, nraw := d.GetChange("tags")
 	o := oraw.(map[string]interface{})
 	n := nraw.(map[string]interface{})
-		create, remove := diffTags(tagsFromSchema(o), tagsFromSchema(n))
+
+	remove, create := diffTags(tagsFromSchema(o), tagsFromSchema(n))
 	log.Printf("[DEBUG] tags to remove: %v", remove)
 	log.Printf("[DEBUG] tags to create: %v", create)
 
-		// Set tags
+	// First remove any obsolete tags
 	if len(remove) > 0 {
 		log.Printf("[DEBUG] Removing tags: %v from %s", remove, d.Id())
 		p := cs.Resourcetags.NewDeleteTagsParams([]string{d.Id()}, resourcetype)
@@ -37,6 +37,8 @@ func setTags(cs *cloudstack.CloudStackClient, d *schema.ResourceData, resourcety
 			return err
 		}
 	}
+
+	// Then add any new tags
 	if len(create) > 0 {
 		log.Printf("[DEBUG] Creating tags: %v for %s", create, d.Id())
 		p := cs.Resourcetags.NewCreateTagsParams([]string{d.Id()}, resourcetype, create)
@@ -45,34 +47,27 @@ func setTags(cs *cloudstack.CloudStackClient, d *schema.ResourceData, resourcety
 			return err
 		}
 	}
-	}
 
 	return nil
 }
 
-// diffTags takes our tags locally and the ones remotely and returns
+// diffTags takes the old and the new tag sets and returns the difference of
-// the set of tags that must be destroyed, and the set of tags that must
+// both. The remaining tags are those that need to be removed and created
-// be created.
 func diffTags(oldTags, newTags map[string]string) (map[string]string, map[string]string) {
-	remove := make(map[string]string)
+	for k, old := range oldTags {
-	for k, v := range oldTags {
+		new, ok := newTags[k]
-		old, ok := newTags[k]
+		if ok && old == new {
-		if !ok || old != v {
+			// We should avoid removing or creating tags we already have
-			// Delete it!
+			delete(oldTags, k)
-			remove[k] = v
-		} else {
-			// We need to remove the modified tags to create them again,
-			// but we should avoid creating what we already have
 			delete(newTags, k)
 		}
 	}
 
-	return newTags, remove
+	return oldTags, newTags
 }
 
-// tagsFromSchema returns the tags for the given tags schema.
+// tagsFromSchema takes the raw schema tags and returns them as a
-// It's needed to properly unpack all string:interface types
+// properly asserted map[string]string
-// to a proper string:string map
 func tagsFromSchema(m map[string]interface{}) map[string]string {
 	result := make(map[string]string, len(m))
 	for k, v := range m {

builtin/providers/cloudstack/tags_test.go

@@ -45,27 +45,21 @@ func TestDiffTags(t *testing.T) {
 	}
 
 	for i, tc := range cases {
-		c, r := diffTags(tagsFromSchema(tc.Old), tagsFromSchema(tc.New))
+		r, c := diffTags(tagsFromSchema(tc.Old), tagsFromSchema(tc.New))
-		if !reflect.DeepEqual(c, tc.Create) {
-			t.Fatalf("%d: bad create: %#v", i, c)
-		}
 		if !reflect.DeepEqual(r, tc.Remove) {
 			t.Fatalf("%d: bad remove: %#v", i, r)
 		}
+		if !reflect.DeepEqual(c, tc.Create) {
+			t.Fatalf("%d: bad create: %#v", i, c)
+		}
 	}
 }
 
 // testAccCheckTags can be used to check the tags on a resource.
-func testAccCheckTags(
+func testAccCheckTags(tags map[string]string, key string, value string) error {
-	tags map[string]string, key string, value string) error {
 	v, ok := tags[key]
-	if value != "" && !ok {
+	if !ok {
 		return fmt.Errorf("Missing tag: %s", key)
-	} else if value == "" && ok {
-		return fmt.Errorf("Extra tag: %s", key)
-	}
-	if value == "" {
-		return nil
 	}
 
 	if v != value {