Fix invalid filter and tags for aws_s3_bucket_lifecycle_configuration

Fixes #29, #35 Non empty tags should be wrapped by an `and` block. When both prefix and tags were empty but defined, it will result in a migration plan diff, so remove them and put an empty filter.
minamijoyo · Jul 6, 2022 · b72d153 · b72d153
1 parent 09d6a33
commit b72d153
Show file tree

Hide file tree

Showing 2 changed files with 187 additions and 21 deletions.
diff --git a/filter/awsv4upgrade/aws_s3_bucket_lifecycle_rule.go b/filter/awsv4upgrade/aws_s3_bucket_lifecycle_rule.go
@@ -87,32 +87,48 @@ func (f *AWSS3BucketLifecycleRuleFilter) ResourceFilter(inFile *tfwrite.File, re
 		// }
 		// Create a filter block
 		filterBlock := tfwrite.NewEmptyNestedBlock("filter")
-		// A child block points a `filter` or an `and` block.
-		var childBlock *tfwrite.NestedBlock
 		nestedBlock.AppendNestedBlock(filterBlock)
 		prefixAttr := nestedBlock.GetAttribute("prefix")
 		tagsAttr := nestedBlock.GetAttribute("tags")
-		if tagsAttr != nil && prefixAttr != nil {
-			andBlock := tfwrite.NewEmptyNestedBlock("and")
-			filterBlock.AppendNestedBlock(andBlock)
-			childBlock = andBlock
-		} else {
-			childBlock = filterBlock
-		}
-		if prefixAttr != nil {
-			childBlock.SetAttributeRaw("prefix", prefixAttr.ValueAsTokens())
-			nestedBlock.RemoveAttribute("prefix")
-		} else {
-			// If a prefix attribute is not found, set an empty string by default.
-			// According to the upgrade guide,
-			// when aws s3api get-bucket-lifecycle-configuration returns `"Filter" : {}`,
-			// we should not set prefix, however we cannot know it without an API call,
-			// so we just assume it contains `"Filter" : { "Prefix": "" }` here.
-			childBlock.SetAttributeValue("prefix", cty.StringVal(""))
-		}
 		if tagsAttr != nil {
-			childBlock.SetAttributeRaw("tags", tagsAttr.ValueAsTokens())
+			tags, err := tagsAttr.ValueAsString()
+			if err == nil {
+				if tags != "{}" {
+					// Non empty tags should be wrapped by an `and` block.
+					andBlock := tfwrite.NewEmptyNestedBlock("and")
+					filterBlock.AppendNestedBlock(andBlock)
+					if prefixAttr != nil {
+						andBlock.SetAttributeRaw("prefix", prefixAttr.ValueAsTokens())
+						nestedBlock.RemoveAttribute("prefix")
+					} else {
+						// If a prefix attribute is not found, set an empty string by default.
+						andBlock.SetAttributeValue("prefix", cty.StringVal(""))
+					}
+					andBlock.SetAttributeRaw("tags", tagsAttr.ValueAsTokens())
+				} else {
+					// When both prefix and tags were empty but defined, it will result
+					// in a migration plan diff, so remove them and put an empty filter.
+					if prefixAttr != nil {
+						prefix, err := prefixAttr.ValueAsString()
+						if err == nil && prefix == `""` {
+							nestedBlock.RemoveAttribute("prefix")
+						}
+					}
+				}
+			}
 			nestedBlock.RemoveAttribute("tags")
+		} else {
+			if prefixAttr != nil {
+				filterBlock.SetAttributeRaw("prefix", prefixAttr.ValueAsTokens())
+				nestedBlock.RemoveAttribute("prefix")
+			} else {
+				// If a prefix attribute is not found, set an empty string by default.
+				// According to the upgrade guide,
+				// when aws s3api get-bucket-lifecycle-configuration returns `"Filter" : {}`,
+				// we should not set prefix, however we cannot know it without an API call,
+				// so we just assume it contains `"Filter" : { "Prefix": "" }` here.
+				filterBlock.SetAttributeValue("prefix", cty.StringVal(""))
+			}
 		}
 
 		// Convert a timestamp format for a date attribute in transition.

diff --git a/filter/awsv4upgrade/aws_s3_bucket_lifecycle_rule_test.go b/filter/awsv4upgrade/aws_s3_bucket_lifecycle_rule_test.go
@@ -291,6 +291,156 @@ resource "aws_s3_bucket_lifecycle_configuration" "example" {
     }
   }
 }
+`,
+		},
+		{
+			name: "with empty prefix and tags",
+			src: `
+resource "aws_s3_bucket" "example" {
+  bucket = "tfedit-test"
+
+  lifecycle_rule {
+    id      = "log"
+    enabled = true
+    prefix  = ""
+    tags    = {}
+
+    noncurrent_version_transition {
+      days          = 30
+      storage_class = "GLACIER"
+    }
+
+    noncurrent_version_expiration {
+      days = 90
+    }
+  }
+}
+`,
+			ok: true,
+			want: `
+resource "aws_s3_bucket" "example" {
+  bucket = "tfedit-test"
+
+}
+
+resource "aws_s3_bucket_lifecycle_configuration" "example" {
+  bucket = aws_s3_bucket.example.id
+
+  rule {
+    id = "log"
+
+    noncurrent_version_transition {
+      storage_class   = "GLACIER"
+      noncurrent_days = 30
+    }
+
+    noncurrent_version_expiration {
+      noncurrent_days = 90
+    }
+    status = "Enabled"
+
+    filter {
+    }
+  }
+}
+`,
+		},
+		{
+			name: "with tags but no prefix",
+			src: `
+resource "aws_s3_bucket" "example" {
+  bucket = "tfedit-test"
+
+  lifecycle_rule {
+    id      = "log"
+    enabled = true
+
+    tags = {
+      rule      = "log"
+      autoclean = "true"
+    }
+
+    transition {
+      days          = 30
+      storage_class = "STANDARD_IA"
+    }
+
+    transition {
+      days          = 60
+      storage_class = "GLACIER"
+    }
+
+    expiration {
+      days = 90
+    }
+  }
+
+  lifecycle_rule {
+    id      = "tmp"
+    prefix  = "tmp/"
+    enabled = true
+
+    expiration {
+      days = 90
+    }
+  }
+}
+`,
+			ok: true,
+			want: `
+resource "aws_s3_bucket" "example" {
+  bucket = "tfedit-test"
+
+
+}
+
+resource "aws_s3_bucket_lifecycle_configuration" "example" {
+  bucket = aws_s3_bucket.example.id
+
+  rule {
+    id = "log"
+
+
+    transition {
+      days          = 30
+      storage_class = "STANDARD_IA"
+    }
+
+    transition {
+      days          = 60
+      storage_class = "GLACIER"
+    }
+
+    expiration {
+      days = 90
+    }
+    status = "Enabled"
+
+    filter {
+
+      and {
+        prefix = ""
+        tags = {
+          rule      = "log"
+          autoclean = "true"
+        }
+      }
+    }
+  }
+
+  rule {
+    id = "tmp"
+
+    expiration {
+      days = 90
+    }
+    status = "Enabled"
+
+    filter {
+      prefix = "tmp/"
+    }
+  }
+}
 `,
 		},
 		{