Review feedback

uber · Sep 27, 2019 · 64bb150 · 64bb150
1 parent 3aa637d
commit 64bb150
Show file tree

Hide file tree

Showing 2 changed files with 77 additions and 46 deletions.
diff --git a/astro/terraform/terraform_remote_state_disable_test.go b/astro/terraform/terraform_remote_state_disable_test.go
@@ -24,7 +24,7 @@ import (
 
 // Tests that backend part can be successfully removed from the config
 // written in HCL 1.0 language
-func TestDeleteTerraformBackendConfigWithHCL(t *testing.T) {
+func TestDeleteTerraformBackendConfigWithHCL1(t *testing.T) {
 	input := []byte(`
 terraform {
     backend "s3" {}
@@ -46,7 +46,7 @@ terraform {
     ]
 }`)
 
-	updatedConfig, err := deleteTerraformBackendConfigWithHCL(input)
+	updatedConfig, err := deleteTerraformBackendConfigWithHCL1(input)
 	assert.NoError(t, err)
 
 	assert.Equal(t, `terraform {}
@@ -77,37 +77,43 @@ func TestDeleteTerraformBackendConfigWithHCL2Success(t *testing.T) {
 		expected string
 	}{
 		{
-			`provider "aws"{
-        region = var.aws_region
-      }`,
-			`provider "aws"{
-        region = var.aws_region
-      }`,
+			config: `
+        provider "aws"{
+          region = var.aws_region
+        }`,
+			expected: `
+        provider "aws"{
+          region = var.aws_region
+        }`,
 		},
 		{
-			`terraform {
-         version = "v0.12.6"
-         backend "local" {
-           path = "path"
-         }
-         key = "value"
-       }`,
-			`terraform {
-         version = "v0.12.6"
-         key = "value"
-       }`,
+			config: `
+        terraform {
+          version = "v0.12.6"
+          backend "local" {
+            path = "path"
+          }
+          key = "value"
+        }`,
+			expected: `
+        terraform {
+          version = "v0.12.6"
+          key = "value"
+        }`,
 		},
 		{
-			`terraform {backend "s3" {}}
+			config: `
+        terraform {backend "s3" {}}
     
-      provider "aws" {
-        region = "us-east-1"
-      }`,
-			`terraform {}
+        provider "aws" {
+          region = "us-east-1"
+        }`,
+			expected: `
+        terraform {}
     
-      provider "aws" {
-        region = "us-east-1"
-      }`,
+        provider "aws" {
+          region = "us-east-1"
+        }`,
 		},
 	}
 	for _, tt := range tests {
@@ -121,20 +127,29 @@ func TestDeleteTerraformBackendConfigWithHCL2Success(t *testing.T) {
 // backend secions contains parenthesis fails. See comment on
 // deleteTerraformBackendConfigWithHCL2 for clarification.
 func TestDeleteTerraformBackendConfigWithHCL2Failure(t *testing.T) {
-	tests := []string{
-		`terraform {
-         backend "local" {
-           path = "module-{{.environment}}"
-         }
-       }`,
-		`terraform {
-        backend "concil" {
-          map = {"key": "val"}
-        }
-    }`,
+	tests := []struct {
+		config string
+	}{
+		{
+			config: `
+        terraform {
+          backend "local" {
+            path = "module-{{.environment}}"
+          }
+        }`,
+		},
+		{
+			config: `
+        terraform {
+          backend "concil" {
+            map = {"key": "val"}
+          }
+        }`,
+		},
 	}
+
 	for _, tt := range tests {
-		_, err := deleteTerraformBackendConfigWithHCL2([]byte(tt))
+		_, err := deleteTerraformBackendConfigWithHCL2([]byte(tt.config))
 		assert.NotNil(t, err)
 	}
 }
diff --git a/astro/terraform/terraform_remote_state_disable_utils.go b/astro/terraform/terraform_remote_state_disable_utils.go
@@ -58,8 +58,8 @@ func astDelIfExists(l *ast.ObjectList, key string) bool {
 	return false
 }
 
-func deleteTerraformBackendConfigWithHCL(in []byte) (updatedConfig []byte, err error) {
-	config, err := parseTerraformConfigWithHCL(in)
+func deleteTerraformBackendConfigWithHCL1(in []byte) (updatedConfig []byte, err error) {
+	config, err := parseTerraformConfigWithHCL1(in)
 	if err != nil {
 		return nil, err
 	}
@@ -85,13 +85,29 @@ func deleteTerraformBackendConfigWithHCL(in []byte) (updatedConfig []byte, err e
 // This method should be rewritten once hcl2 supports AST traversal and modification.
 func deleteTerraformBackendConfigWithHCL2(in []byte) (updatedConfig []byte, err error) {
 	// Regexp to find if any backend configuration exists
-	backendDefinitionRe := regexp.MustCompile(`(?s)[{\s+]backend\s+"[^"]+"\s*{`)
+	backendDefinitionRe := regexp.MustCompile(
+		// make sure `\s` matches line breaks
+		`(?s)` +
+			// match `{backend ` or ` backend `, but not `some_backend` or ` backend_confg`
+			`[{\s+]backend\s+` +
+			// backend name and opening of the configuration, e.g. `"s3" {`
+			`"[^"]+"\s*{`,
+	)
 	// Regexp to find simple backend configuration, which doesn't contain '{}' inside
-	backendBlockRe := regexp.MustCompile(`(?s)(\s*backend\s+"[^"]+"\s*{[^{]*?})`)
+	backendBlockRe := regexp.MustCompile(
+		// make sure `\s` matches line breaks
+		`(?s)` +
+			// match backend and it's name, e.g. `backend "s3"` or ` backend "s3"`,
+			// note, that opening brace before `backend` is not included in the regex,
+			// because it should not be removed.
+			`(\s*backend\s+"[^"]+"\s*` +
+			// match backend configuration block, that doesn't have inner braces
+			`{[^{]*?})`,
+	)
 	if backendDefinitionRe.Match(in) {
 		indexes := backendBlockRe.FindSubmatchIndex(in)
 		if indexes == nil {
-			return nil, fmt.Errorf("unable to delete backend config: unsupported sytax")
+			return nil, fmt.Errorf("unable to delete backend config: unsupported syntax")
 		}
 		// Remove found backend submatch from config
 		return append(in[:indexes[2]], in[indexes[3]:]...), nil
@@ -101,7 +117,7 @@ func deleteTerraformBackendConfigWithHCL2(in []byte) (updatedConfig []byte, err
 
 func deleteTerraformBackendConfig(in []byte, v *version.Version) (updatedConfig []byte, err error) {
 	if VersionMatches(v, "<0.12") {
-		return deleteTerraformBackendConfigWithHCL(in)
+		return deleteTerraformBackendConfigWithHCL1(in)
 	}
 	return deleteTerraformBackendConfigWithHCL2(in)
 }
@@ -137,7 +153,7 @@ func deleteTerraformBackendConfigFromFile(file string, v *version.Version) error
 	return nil
 }
 
-func parseTerraformConfigWithHCL(in []byte) (*ast.ObjectList, error) {
+func parseTerraformConfigWithHCL1(in []byte) (*ast.ObjectList, error) {
 	astFile, err := hcl.ParseBytes(in)
 	if err != nil {
 		return nil, err