diff --git a/builtin/providers/cloudstack/resource_cloudstack_network.go b/builtin/providers/cloudstack/resource_cloudstack_network.go index 0492d449b8..261d0ec508 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_network.go +++ b/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 - err = setTags(cs, d, "network") - if err != nil { - return fmt.Errorf("Error updating tags") + if d.HasChange("tags") { + err = setTags(cs, d, "network") + if err != nil { + return fmt.Errorf("Error updating tags: %s", err) + } } return resourceCloudStackNetworkRead(d, meta) diff --git a/builtin/providers/cloudstack/resource_cloudstack_network_test.go b/builtin/providers/cloudstack/resource_cloudstack_network_test.go index 6c0cff6ed8..3bc1744b9b 100644 --- a/builtin/providers/cloudstack/resource_cloudstack_network_test.go +++ b/builtin/providers/cloudstack/resource_cloudstack_network_test.go @@ -163,13 +163,13 @@ func testAccCheckCloudStackNetworkDestroy(s *terraform.State) error { var testAccCloudStackNetwork_basic = fmt.Sprintf(` resource "cloudstack_network" "foo" { - name = "terraform-network" - cidr = "%s" - network_offering = "%s" - zone = "%s" - tags = { - terraform-tag = "true" - } + name = "terraform-network" + cidr = "%s" + network_offering = "%s" + zone = "%s" + tags = { + terraform-tag = "true" + } }`, CLOUDSTACK_NETWORK_2_CIDR, CLOUDSTACK_NETWORK_2_OFFERING, @@ -184,11 +184,11 @@ resource "cloudstack_vpc" "foobar" { } resource "cloudstack_network" "foo" { - name = "terraform-network" - cidr = "%s" - network_offering = "%s" - vpc = "${cloudstack_vpc.foobar.name}" - zone = "${cloudstack_vpc.foobar.zone}" + name = "terraform-network" + cidr = "%s" + network_offering = "%s" + vpc = "${cloudstack_vpc.foobar.name}" + zone = "${cloudstack_vpc.foobar.zone}" }`, CLOUDSTACK_VPC_CIDR_1, CLOUDSTACK_VPC_OFFERING, diff --git a/builtin/providers/cloudstack/tags.go b/builtin/providers/cloudstack/tags.go index 015728f5be..389cdb47f2 100644 --- a/builtin/providers/cloudstack/tags.go +++ b/builtin/providers/cloudstack/tags.go @@ -7,72 +7,67 @@ 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)) - log.Printf("[DEBUG] tags to remove: %v", remove) - log.Printf("[DEBUG] tags to create: %v", create) + oraw, nraw := d.GetChange("tags") + o := oraw.(map[string]interface{}) + n := nraw.(map[string]interface{}) - // Set tags - if len(remove) > 0 { - log.Printf("[DEBUG] Removing tags: %v from %s", remove, d.Id()) - p := cs.Resourcetags.NewDeleteTagsParams([]string{d.Id()}, resourcetype) - p.SetTags(remove) - _, err := cs.Resourcetags.DeleteTags(p) - if err != nil { - return err - } + remove, create := diffTags(tagsFromSchema(o), tagsFromSchema(n)) + log.Printf("[DEBUG] tags to remove: %v", remove) + log.Printf("[DEBUG] tags to create: %v", create) + + // 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) + p.SetTags(remove) + _, err := cs.Resourcetags.DeleteTags(p) + if err != nil { + return err } - if len(create) > 0 { - log.Printf("[DEBUG] Creating tags: %v for %s", create, d.Id()) - p := cs.Resourcetags.NewCreateTagsParams([]string{d.Id()}, resourcetype, create) - _, err := cs.Resourcetags.CreateTags(p) - if err != nil { - 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) + _, err := cs.Resourcetags.CreateTags(p) + if err != nil { + return err } } return nil } -// diffTags takes our tags locally and the ones remotely and returns -// the set of tags that must be destroyed, and the set of tags that must -// be created. +// diffTags takes the old and the new tag sets and returns the difference of +// both. The remaining tags are those that need to be removed and created func diffTags(oldTags, newTags map[string]string) (map[string]string, map[string]string) { - remove := make(map[string]string) - for k, v := range oldTags { - old, ok := newTags[k] - if !ok || old != v { - // Delete it! - remove[k] = v - } else { - // We need to remove the modified tags to create them again, - // but we should avoid creating what we already have + for k, old := range oldTags { + new, ok := newTags[k] + if ok && old == new { + // We should avoid removing or creating tags we already have + delete(oldTags, k) delete(newTags, k) } } - return newTags, remove + return oldTags, newTags } -// tagsFromSchema returns the tags for the given tags schema. -// It's needed to properly unpack all string:interface types -// to a proper string:string map +// tagsFromSchema takes the raw schema tags and returns them as a +// properly asserted map[string]string func tagsFromSchema(m map[string]interface{}) map[string]string { result := make(map[string]string, len(m)) for k, v := range m { diff --git a/builtin/providers/cloudstack/tags_test.go b/builtin/providers/cloudstack/tags_test.go index 05f31bff59..fba9cadd7f 100644 --- a/builtin/providers/cloudstack/tags_test.go +++ b/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)) - if !reflect.DeepEqual(c, tc.Create) { - t.Fatalf("%d: bad create: %#v", i, c) - } + r, c := diffTags(tagsFromSchema(tc.Old), tagsFromSchema(tc.New)) 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( - tags map[string]string, key string, value string) error { +func testAccCheckTags(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 {