From de78838cd4f3389be03057245dea3f4c46ee57e0 Mon Sep 17 00:00:00 2001 From: johnthedev97 Date: Fri, 2 Jun 2017 15:18:27 -0400 Subject: [PATCH] Fix issues in Cloudwatch Log Group tag (#14886) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix issues in Cloudwatch Log Group tag 1) Removing tags from terraform doesn’t actually get removed in AWS 2) Trying to update a tag with empty value (“”) to a non-empty value causes terraform to loop forever The issue was caused by a mixup of using tag values where tag name should have used and is corrected in this patch. This patch also removes the comparison of old and new tag values, because AWS api takes care of updates by itself and there is no need to perform an unnecessary UnTag API to update an existing tag value * Updated the test cases to cover the removal and empty update scenarios --- .../aws/resource_aws_cloudwatch_log_group.go | 8 +-- .../resource_aws_cloudwatch_log_group_test.go | 61 ++++++++++++++++++- 2 files changed, 62 insertions(+), 7 deletions(-) diff --git a/builtin/providers/aws/resource_aws_cloudwatch_log_group.go b/builtin/providers/aws/resource_aws_cloudwatch_log_group.go index 886d0a52a..a4ca7b753 100644 --- a/builtin/providers/aws/resource_aws_cloudwatch_log_group.go +++ b/builtin/providers/aws/resource_aws_cloudwatch_log_group.go @@ -213,10 +213,10 @@ func diffCloudWatchTags(oldTags map[string]interface{}, newTags map[string]inter } var remove []*string - for _, t := range oldTags { - old, ok := create[t.(string)] - if !ok || *old != t.(string) { - remove = append(remove, aws.String(t.(string))) + for t, _ := range oldTags { + _, ok := create[t] + if !ok { + remove = append(remove, aws.String(t)) } } diff --git a/builtin/providers/aws/resource_aws_cloudwatch_log_group_test.go b/builtin/providers/aws/resource_aws_cloudwatch_log_group_test.go index 1e8a4ecb8..e1f6e3131 100644 --- a/builtin/providers/aws/resource_aws_cloudwatch_log_group_test.go +++ b/builtin/providers/aws/resource_aws_cloudwatch_log_group_test.go @@ -153,20 +153,44 @@ func TestAccAWSCloudWatchLogGroup_tagging(t *testing.T) { Config: testAccAWSCloudWatchLogGroupConfigWithTags(rInt), Check: resource.ComposeTestCheckFunc( testAccCheckCloudWatchLogGroupExists("aws_cloudwatch_log_group.foobar", &lg), - resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.%", "2"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.%", "3"), resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Environment", "Production"), resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Foo", "Bar"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Empty", ""), + ), + }, + { + Config: testAccAWSCloudWatchLogGroupConfigWithTagsAdded(rInt), + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudWatchLogGroupExists("aws_cloudwatch_log_group.foobar", &lg), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.%", "4"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Environment", "Development"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Foo", "Bar"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Empty", ""), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Bar", "baz"), ), }, { Config: testAccAWSCloudWatchLogGroupConfigWithTagsUpdated(rInt), Check: resource.ComposeTestCheckFunc( testAccCheckCloudWatchLogGroupExists("aws_cloudwatch_log_group.foobar", &lg), - resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.%", "3"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.%", "4"), resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Environment", "Development"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Empty", "NotEmpty"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Foo", "UpdatedBar"), resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Bar", "baz"), ), }, + { + Config: testAccAWSCloudWatchLogGroupConfigWithTags(rInt), + Check: resource.ComposeTestCheckFunc( + testAccCheckCloudWatchLogGroupExists("aws_cloudwatch_log_group.foobar", &lg), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.%", "3"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Environment", "Production"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Foo", "Bar"), + resource.TestCheckResourceAttr("aws_cloudwatch_log_group.foobar", "tags.Empty", ""), + ), + }, }, }) } @@ -243,6 +267,22 @@ resource "aws_cloudwatch_log_group" "foobar" { tags { Environment = "Production" Foo = "Bar" + Empty = "" + } +} +`, rInt) +} + +func testAccAWSCloudWatchLogGroupConfigWithTagsAdded(rInt int) string { + return fmt.Sprintf(` +resource "aws_cloudwatch_log_group" "foobar" { + name = "foo-bar-%d" + + tags { + Environment = "Development" + Foo = "Bar" + Empty = "" + Bar = "baz" } } `, rInt) @@ -255,13 +295,28 @@ resource "aws_cloudwatch_log_group" "foobar" { tags { Environment = "Development" - Foo = "Bar" + Foo = "UpdatedBar" + Empty = "NotEmpty" Bar = "baz" } } `, rInt) } +func testAccAWSCloudWatchLogGroupConfigWithTagsRemoval(rInt int) string { + return fmt.Sprintf(` +resource "aws_cloudwatch_log_group" "foobar" { + name = "foo-bar-%d" + + tags { + Environment = "Production" + Foo = "Bar" + Empty = "" + } +} +`, rInt) +} + func testAccAWSCloudWatchLogGroupConfig_withRetention(rInt int) string { return fmt.Sprintf(` resource "aws_cloudwatch_log_group" "foobar" {