From 713a356e5444b4e39492d191252846f662b7320f Mon Sep 17 00:00:00 2001 From: Tatiana Romanova Date: Mon, 12 Aug 2019 08:59:17 +0200 Subject: [PATCH 1/9] Add test --- .../fixtures/plan-with-variables/astro.yaml | 18 ++++++++++++++++++ .../fixtures/plan-with-variables/bar/bar.tf | 7 +++++++ .../fixtures/plan-with-variables/foo/foo.tf | 5 +++++ .../plan-with-variables/foo/globals.tf | 4 ++++ 4 files changed, 34 insertions(+) create mode 100644 astro/tests/fixtures/plan-with-variables/astro.yaml create mode 100644 astro/tests/fixtures/plan-with-variables/bar/bar.tf create mode 100644 astro/tests/fixtures/plan-with-variables/foo/foo.tf create mode 100644 astro/tests/fixtures/plan-with-variables/foo/globals.tf diff --git a/astro/tests/fixtures/plan-with-variables/astro.yaml b/astro/tests/fixtures/plan-with-variables/astro.yaml new file mode 100644 index 0000000..62b1400 --- /dev/null +++ b/astro/tests/fixtures/plan-with-variables/astro.yaml @@ -0,0 +1,18 @@ +--- + +modules: + - name: foo + path: ./foo + remote: + backend: local + backend_config: + path: /tmp/terraform-tests/foo.tfstate + + - name: bar + path: ./bar + remote: + backend: local + backend_config: + path: /tmp/terraform-tests/bar.tfstate + variables: + - name: region diff --git a/astro/tests/fixtures/plan-with-variables/bar/bar.tf b/astro/tests/fixtures/plan-with-variables/bar/bar.tf new file mode 100644 index 0000000..883866b --- /dev/null +++ b/astro/tests/fixtures/plan-with-variables/bar/bar.tf @@ -0,0 +1,7 @@ +terraform { + backend "local" {} +} + +variable "region" {} + +resource "null_resource" "bar" {} diff --git a/astro/tests/fixtures/plan-with-variables/foo/foo.tf b/astro/tests/fixtures/plan-with-variables/foo/foo.tf new file mode 100644 index 0000000..c9bc325 --- /dev/null +++ b/astro/tests/fixtures/plan-with-variables/foo/foo.tf @@ -0,0 +1,5 @@ +terraform { + backend "local" {} +} + +resource "null_resource" "foo" {} diff --git a/astro/tests/fixtures/plan-with-variables/foo/globals.tf b/astro/tests/fixtures/plan-with-variables/foo/globals.tf new file mode 100644 index 0000000..1f44969 --- /dev/null +++ b/astro/tests/fixtures/plan-with-variables/foo/globals.tf @@ -0,0 +1,4 @@ +variable "region" { +type = "string" +default = "r" +} From 3deaa0fe14bec174b2715a4471dd256ab5926025 Mon Sep 17 00:00:00 2001 From: Tatiana Romanova Date: Mon, 12 Aug 2019 13:42:41 +0200 Subject: [PATCH 2/9] Don't pass variable to a module if it doesn't require it --- Makefile | 2 +- astro/execution.go | 1 + .../plan-with-variables-0.7.13/astro.yaml | 19 +++++++++++++++++++ .../plan-with-variables-0.7.13/bar/bar.tf | 3 +++ .../plan-with-variables-0.7.13/foo/foo.tf | 1 + .../fixtures/plan-with-variables/astro.yaml | 2 +- .../plan-with-variables/foo/globals.tf | 4 ---- astro/tests/integration_test.go | 13 +++++++++++++ 8 files changed, 39 insertions(+), 6 deletions(-) create mode 100644 astro/tests/fixtures/plan-with-variables-0.7.13/astro.yaml create mode 100644 astro/tests/fixtures/plan-with-variables-0.7.13/bar/bar.tf create mode 100644 astro/tests/fixtures/plan-with-variables-0.7.13/foo/foo.tf delete mode 100644 astro/tests/fixtures/plan-with-variables/foo/globals.tf diff --git a/Makefile b/Makefile index 2793c47..495b956 100644 --- a/Makefile +++ b/Makefile @@ -52,5 +52,5 @@ lint: .PHONY: test test: - go test -timeout 1m -coverprofile=.coverage.out ./... \ + go test -timeout 5m -coverprofile=.coverage.out ./... \ |grep -v -E '^\?' diff --git a/astro/execution.go b/astro/execution.go index ee46f70..9f644fb 100644 --- a/astro/execution.go +++ b/astro/execution.go @@ -137,6 +137,7 @@ type unboundExecution struct { // boundExecution with variable values replaced. An error is returned if // not all required user values were provided. func (e *unboundExecution) bind(userVars map[string]string) (*boundExecution, error) { + // boundVars is the map of execution variables bound to the values provided by user boundVars := make(map[string]string) diff --git a/astro/tests/fixtures/plan-with-variables-0.7.13/astro.yaml b/astro/tests/fixtures/plan-with-variables-0.7.13/astro.yaml new file mode 100644 index 0000000..b96f5b3 --- /dev/null +++ b/astro/tests/fixtures/plan-with-variables-0.7.13/astro.yaml @@ -0,0 +1,19 @@ +--- + +modules: + - name: foo + path: foo/ + remote: + backend: local + backend_config: + path: /tmp/terraform-tests/foo.tfstate + + - name: bar + path: bar/ + remote: + backend: local + backend_config: + path: /tmp/terraform-tests/bar.tfstate + variables: + - name: region + diff --git a/astro/tests/fixtures/plan-with-variables-0.7.13/bar/bar.tf b/astro/tests/fixtures/plan-with-variables-0.7.13/bar/bar.tf new file mode 100644 index 0000000..46f6aef --- /dev/null +++ b/astro/tests/fixtures/plan-with-variables-0.7.13/bar/bar.tf @@ -0,0 +1,3 @@ +variable "region" {} + +resource "null_resource" "bar" {} diff --git a/astro/tests/fixtures/plan-with-variables-0.7.13/foo/foo.tf b/astro/tests/fixtures/plan-with-variables-0.7.13/foo/foo.tf new file mode 100644 index 0000000..3911a2a --- /dev/null +++ b/astro/tests/fixtures/plan-with-variables-0.7.13/foo/foo.tf @@ -0,0 +1 @@ +resource "null_resource" "foo" {} diff --git a/astro/tests/fixtures/plan-with-variables/astro.yaml b/astro/tests/fixtures/plan-with-variables/astro.yaml index 62b1400..93c63e7 100644 --- a/astro/tests/fixtures/plan-with-variables/astro.yaml +++ b/astro/tests/fixtures/plan-with-variables/astro.yaml @@ -2,7 +2,7 @@ modules: - name: foo - path: ./foo + path: ./foo remote: backend: local backend_config: diff --git a/astro/tests/fixtures/plan-with-variables/foo/globals.tf b/astro/tests/fixtures/plan-with-variables/foo/globals.tf deleted file mode 100644 index 1f44969..0000000 --- a/astro/tests/fixtures/plan-with-variables/foo/globals.tf +++ /dev/null @@ -1,4 +0,0 @@ -variable "region" { -type = "string" -default = "r" -} diff --git a/astro/tests/integration_test.go b/astro/tests/integration_test.go index 2e9ab4b..9be7942 100644 --- a/astro/tests/integration_test.go +++ b/astro/tests/integration_test.go @@ -219,3 +219,16 @@ func TestProjectPlanDetachSuccess(t *testing.T) { }) } } + +// TestVariablePassing checks that variables are passed to the modules that declare them and +// not passed to the modules that don't +func TestVariablePassing(t *testing.T) { + for _, version := range terraformVersionsToTest { + t.Run(version, func(t *testing.T) { + result := RunTest(t, []string{"plan", "--trace", "--region", "east1"}, "fixtures/plan-with-variables", version) + assert.Contains(t, result.Stderr.String(), "-out=foo.plan]") + assert.Contains(t, result.Stderr.String(), "-out=bar-east1.plan -var region=east1]") + assert.Equal(t, 0, result.ExitCode) + }) + } +} From 060767594278967d0fcdee689d1799abebf63575 Mon Sep 17 00:00:00 2001 From: Tatiana Romanova Date: Tue, 13 Aug 2019 08:38:20 +0200 Subject: [PATCH 3/9] Move test from integration to internal to speed up the execution --- astro/terraform/version_check.go | 6 ++++++ .../plan-with-variables-0.7.13/astro.yaml | 19 ------------------- .../plan-with-variables-0.7.13/bar/bar.tf | 3 --- .../plan-with-variables-0.7.13/foo/foo.tf | 1 - .../fixtures/plan-with-variables/astro.yaml | 18 ------------------ .../fixtures/plan-with-variables/bar/bar.tf | 7 ------- .../fixtures/plan-with-variables/foo/foo.tf | 5 ----- astro/tests/integration_test.go | 13 ------------- 8 files changed, 6 insertions(+), 66 deletions(-) delete mode 100644 astro/tests/fixtures/plan-with-variables-0.7.13/astro.yaml delete mode 100644 astro/tests/fixtures/plan-with-variables-0.7.13/bar/bar.tf delete mode 100644 astro/tests/fixtures/plan-with-variables-0.7.13/foo/foo.tf delete mode 100644 astro/tests/fixtures/plan-with-variables/astro.yaml delete mode 100644 astro/tests/fixtures/plan-with-variables/bar/bar.tf delete mode 100644 astro/tests/fixtures/plan-with-variables/foo/foo.tf diff --git a/astro/terraform/version_check.go b/astro/terraform/version_check.go index 92dadfc..cde5840 100644 --- a/astro/terraform/version_check.go +++ b/astro/terraform/version_check.go @@ -31,3 +31,9 @@ func VersionMatches(v *version.Version, versionConstraint string) bool { } return constraint.Check(v) } + +// StringVersionMatches returns whether or not the version matches the constraint. +// See VersionMatches for more info. +func StringVersionMatches(v string, versionConstraint string) bool { + return VersionMatches(version.Must(version.NewVersion(v)), versionConstraint) +} diff --git a/astro/tests/fixtures/plan-with-variables-0.7.13/astro.yaml b/astro/tests/fixtures/plan-with-variables-0.7.13/astro.yaml deleted file mode 100644 index b96f5b3..0000000 --- a/astro/tests/fixtures/plan-with-variables-0.7.13/astro.yaml +++ /dev/null @@ -1,19 +0,0 @@ ---- - -modules: - - name: foo - path: foo/ - remote: - backend: local - backend_config: - path: /tmp/terraform-tests/foo.tfstate - - - name: bar - path: bar/ - remote: - backend: local - backend_config: - path: /tmp/terraform-tests/bar.tfstate - variables: - - name: region - diff --git a/astro/tests/fixtures/plan-with-variables-0.7.13/bar/bar.tf b/astro/tests/fixtures/plan-with-variables-0.7.13/bar/bar.tf deleted file mode 100644 index 46f6aef..0000000 --- a/astro/tests/fixtures/plan-with-variables-0.7.13/bar/bar.tf +++ /dev/null @@ -1,3 +0,0 @@ -variable "region" {} - -resource "null_resource" "bar" {} diff --git a/astro/tests/fixtures/plan-with-variables-0.7.13/foo/foo.tf b/astro/tests/fixtures/plan-with-variables-0.7.13/foo/foo.tf deleted file mode 100644 index 3911a2a..0000000 --- a/astro/tests/fixtures/plan-with-variables-0.7.13/foo/foo.tf +++ /dev/null @@ -1 +0,0 @@ -resource "null_resource" "foo" {} diff --git a/astro/tests/fixtures/plan-with-variables/astro.yaml b/astro/tests/fixtures/plan-with-variables/astro.yaml deleted file mode 100644 index 93c63e7..0000000 --- a/astro/tests/fixtures/plan-with-variables/astro.yaml +++ /dev/null @@ -1,18 +0,0 @@ ---- - -modules: - - name: foo - path: ./foo - remote: - backend: local - backend_config: - path: /tmp/terraform-tests/foo.tfstate - - - name: bar - path: ./bar - remote: - backend: local - backend_config: - path: /tmp/terraform-tests/bar.tfstate - variables: - - name: region diff --git a/astro/tests/fixtures/plan-with-variables/bar/bar.tf b/astro/tests/fixtures/plan-with-variables/bar/bar.tf deleted file mode 100644 index 883866b..0000000 --- a/astro/tests/fixtures/plan-with-variables/bar/bar.tf +++ /dev/null @@ -1,7 +0,0 @@ -terraform { - backend "local" {} -} - -variable "region" {} - -resource "null_resource" "bar" {} diff --git a/astro/tests/fixtures/plan-with-variables/foo/foo.tf b/astro/tests/fixtures/plan-with-variables/foo/foo.tf deleted file mode 100644 index c9bc325..0000000 --- a/astro/tests/fixtures/plan-with-variables/foo/foo.tf +++ /dev/null @@ -1,5 +0,0 @@ -terraform { - backend "local" {} -} - -resource "null_resource" "foo" {} diff --git a/astro/tests/integration_test.go b/astro/tests/integration_test.go index 9be7942..2e9ab4b 100644 --- a/astro/tests/integration_test.go +++ b/astro/tests/integration_test.go @@ -219,16 +219,3 @@ func TestProjectPlanDetachSuccess(t *testing.T) { }) } } - -// TestVariablePassing checks that variables are passed to the modules that declare them and -// not passed to the modules that don't -func TestVariablePassing(t *testing.T) { - for _, version := range terraformVersionsToTest { - t.Run(version, func(t *testing.T) { - result := RunTest(t, []string{"plan", "--trace", "--region", "east1"}, "fixtures/plan-with-variables", version) - assert.Contains(t, result.Stderr.String(), "-out=foo.plan]") - assert.Contains(t, result.Stderr.String(), "-out=bar-east1.plan -var region=east1]") - assert.Equal(t, 0, result.ExitCode) - }) - } -} From bed01b2d2c4cefd434f4bf10f1ec12d4228886d2 Mon Sep 17 00:00:00 2001 From: Tatiana Romanova Date: Tue, 13 Aug 2019 08:48:56 +0200 Subject: [PATCH 4/9] Smaller diff --- Makefile | 2 +- astro/execution.go | 1 - astro/terraform/version_check.go | 6 ------ astro/tests/base.go | 1 - 4 files changed, 1 insertion(+), 9 deletions(-) diff --git a/Makefile b/Makefile index 495b956..2793c47 100644 --- a/Makefile +++ b/Makefile @@ -52,5 +52,5 @@ lint: .PHONY: test test: - go test -timeout 5m -coverprofile=.coverage.out ./... \ + go test -timeout 1m -coverprofile=.coverage.out ./... \ |grep -v -E '^\?' diff --git a/astro/execution.go b/astro/execution.go index 9f644fb..ee46f70 100644 --- a/astro/execution.go +++ b/astro/execution.go @@ -137,7 +137,6 @@ type unboundExecution struct { // boundExecution with variable values replaced. An error is returned if // not all required user values were provided. func (e *unboundExecution) bind(userVars map[string]string) (*boundExecution, error) { - // boundVars is the map of execution variables bound to the values provided by user boundVars := make(map[string]string) diff --git a/astro/terraform/version_check.go b/astro/terraform/version_check.go index cde5840..92dadfc 100644 --- a/astro/terraform/version_check.go +++ b/astro/terraform/version_check.go @@ -31,9 +31,3 @@ func VersionMatches(v *version.Version, versionConstraint string) bool { } return constraint.Check(v) } - -// StringVersionMatches returns whether or not the version matches the constraint. -// See VersionMatches for more info. -func StringVersionMatches(v string, versionConstraint string) bool { - return VersionMatches(version.Must(version.NewVersion(v)), versionConstraint) -} diff --git a/astro/tests/base.go b/astro/tests/base.go index 8e7bcae..c644d78 100644 --- a/astro/tests/base.go +++ b/astro/tests/base.go @@ -45,7 +45,6 @@ var ( "0.9.11", "0.10.8", "0.11.5", - "0.12.6", } ) From 5e1f603474258b86456c17b45c221deb267f2a58 Mon Sep 17 00:00:00 2001 From: Tatiana Romanova Date: Tue, 13 Aug 2019 10:40:18 +0200 Subject: [PATCH 5/9] Get plan output with terraform 0.12 --- astro/terraform/version_check.go | 6 ++++++ astro/tests/base.go | 1 + 2 files changed, 7 insertions(+) diff --git a/astro/terraform/version_check.go b/astro/terraform/version_check.go index 92dadfc..a3d5c5c 100644 --- a/astro/terraform/version_check.go +++ b/astro/terraform/version_check.go @@ -31,3 +31,9 @@ func VersionMatches(v *version.Version, versionConstraint string) bool { } return constraint.Check(v) } + +// StringVersionMatches returns whether or not the version passed as string matches the constraint. +// See VersionMatches for more info. +func StringVersionMatches(v string, versionConstraint string) bool { + return VersionMatches(version.Must(version.NewVersion(v)), versionConstraint) +} diff --git a/astro/tests/base.go b/astro/tests/base.go index c644d78..8e7bcae 100644 --- a/astro/tests/base.go +++ b/astro/tests/base.go @@ -45,6 +45,7 @@ var ( "0.9.11", "0.10.8", "0.11.5", + "0.12.6", } ) From e42b365400fbd8954f45d596992a04ba69ca87d5 Mon Sep 17 00:00:00 2001 From: Tatiana Romanova Date: Wed, 14 Aug 2019 10:20:04 +0200 Subject: [PATCH 6/9] Feedback: move StringVersionMatches from terraform package to test --- astro/terraform/version_check.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/astro/terraform/version_check.go b/astro/terraform/version_check.go index a3d5c5c..92dadfc 100644 --- a/astro/terraform/version_check.go +++ b/astro/terraform/version_check.go @@ -31,9 +31,3 @@ func VersionMatches(v *version.Version, versionConstraint string) bool { } return constraint.Check(v) } - -// StringVersionMatches returns whether or not the version passed as string matches the constraint. -// See VersionMatches for more info. -func StringVersionMatches(v string, versionConstraint string) bool { - return VersionMatches(version.Must(version.NewVersion(v)), versionConstraint) -} From 3aa637d02731a70947c9448a9b9c2e05e81a47ec Mon Sep 17 00:00:00 2001 From: Tatiana Romanova Date: Thu, 15 Aug 2019 15:03:16 +0200 Subject: [PATCH 7/9] Add detach support which is compatible with hcl2 used for terraform 0.12 --- .../terraform_remote_state_disable.go | 7 +- .../terraform_remote_state_disable_test.go | 80 ++++++++++++++++++- .../terraform_remote_state_disable_utils.go | 56 ++++++++++--- astro/tests/fixtures/plan-detach/versions.tf | 3 + 4 files changed, 127 insertions(+), 19 deletions(-) create mode 100644 astro/tests/fixtures/plan-detach/versions.tf diff --git a/astro/terraform/terraform_remote_state_disable.go b/astro/terraform/terraform_remote_state_disable.go index 86abfcf..5c3a462 100644 --- a/astro/terraform/terraform_remote_state_disable.go +++ b/astro/terraform/terraform_remote_state_disable.go @@ -120,12 +120,15 @@ func (s *Session) deleteBackendConfig() error { if len(candidates) < 1 { return errors.New("cannot find backend configuration in the Terraform files") } + terraformVersion, err := s.Version() + if err != nil { + return err + } for _, f := range candidates { - if err := deleteTerraformBackendConfigFromFile(f); err != nil { + if err := deleteTerraformBackendConfigFromFile(f, terraformVersion); err != nil { return err } } - return nil } diff --git a/astro/terraform/terraform_remote_state_disable_test.go b/astro/terraform/terraform_remote_state_disable_test.go index 1f43cb1..05b94b8 100644 --- a/astro/terraform/terraform_remote_state_disable_test.go +++ b/astro/terraform/terraform_remote_state_disable_test.go @@ -22,14 +22,16 @@ import ( "github.com/stretchr/testify/assert" ) -func TestDeleteTerraformBackendConfig(t *testing.T) { +// Tests that backend part can be successfully removed from the config +// written in HCL 1.0 language +func TestDeleteTerraformBackendConfigWithHCL(t *testing.T) { input := []byte(` terraform { backend "s3" {} } provider "aws" { - region = "us-east-1" + region = "${var.aws_region}" } module "codecommit" { @@ -44,13 +46,13 @@ terraform { ] }`) - updatedConfig, err := deleteTerraformBackendConfig(input) + updatedConfig, err := deleteTerraformBackendConfigWithHCL(input) assert.NoError(t, err) assert.Equal(t, `terraform {} provider "aws" { - region = "us-east-1" + region = "${var.aws_region}" } module "codecommit" { @@ -66,3 +68,73 @@ module "codecommit" { ] }`, string(updatedConfig)) } + +// Tests that backend part can be successfully removed from the config +// written in HCL 2.0 language +func TestDeleteTerraformBackendConfigWithHCL2Success(t *testing.T) { + tests := []struct { + config string + expected string + }{ + { + `provider "aws"{ + region = var.aws_region + }`, + `provider "aws"{ + region = var.aws_region + }`, + }, + { + `terraform { + version = "v0.12.6" + backend "local" { + path = "path" + } + key = "value" + }`, + `terraform { + version = "v0.12.6" + key = "value" + }`, + }, + { + `terraform {backend "s3" {}} + + provider "aws" { + region = "us-east-1" + }`, + `terraform {} + + provider "aws" { + region = "us-east-1" + }`, + }, + } + for _, tt := range tests { + actual, err := deleteTerraformBackendConfigWithHCL2([]byte(tt.config)) + assert.Equal(t, string(actual), tt.expected) + assert.Nil(t, err) + } +} + +// Tests that trying to delete backend part from configs where +// 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"} + } + }`, + } + for _, tt := range tests { + _, err := deleteTerraformBackendConfigWithHCL2([]byte(tt)) + assert.NotNil(t, err) + } +} diff --git a/astro/terraform/terraform_remote_state_disable_utils.go b/astro/terraform/terraform_remote_state_disable_utils.go index 9b9bc53..6475ff8 100644 --- a/astro/terraform/terraform_remote_state_disable_utils.go +++ b/astro/terraform/terraform_remote_state_disable_utils.go @@ -19,11 +19,14 @@ package terraform import ( "bytes" "errors" + "fmt" "io/ioutil" "os" + "regexp" "github.com/uber/astro/astro/logger" + version "github.com/burl/go-version" "github.com/hashicorp/hcl" "github.com/hashicorp/hcl/hcl/ast" "github.com/hashicorp/hcl/hcl/printer" @@ -41,22 +44,22 @@ func astGet(l *ast.ObjectList, key string) ast.Node { return nil } -// astDel deletes the node at key from l. Returns an error if the key does not -// exist. -func astDel(l *ast.ObjectList, key string) error { +// astDelIfExists deletes the node at key from l if it exists. +// Returns true if item was deleted. +func astDelIfExists(l *ast.ObjectList, key string) bool { for i := range l.Items { for j := range l.Items[i].Keys { if l.Items[i].Keys[j].Token.Text == key { l.Items = append(l.Items[:i], l.Items[i+1:]...) - return nil + return true } } } - return errors.New("cannot delete key %v: does not exist") + return false } -func deleteTerraformBackendConfig(in []byte) (updatedConfig []byte, err error) { - config, err := parseTerraformConfig(in) +func deleteTerraformBackendConfigWithHCL(in []byte) (updatedConfig []byte, err error) { + config, err := parseTerraformConfigWithHCL(in) if err != nil { return nil, err } @@ -66,9 +69,7 @@ func deleteTerraformBackendConfig(in []byte) (updatedConfig []byte, err error) { return nil, errors.New("could not parse \"terraform\" block in config") } - if err := astDel(terraformConfigBlock.List, "backend"); err != nil { - return nil, err - } + astDelIfExists(terraformConfigBlock.List, "backend") buf := &bytes.Buffer{} printer.Fprint(buf, config) @@ -76,14 +77,43 @@ func deleteTerraformBackendConfig(in []byte) (updatedConfig []byte, err error) { return buf.Bytes(), nil } -func deleteTerraformBackendConfigFromFile(file string) error { +// hcl2 (used by terraform 0.12) doesn't provide interface to walk through the AST or +// to modify block values, see https://github.com/hashicorp/hcl2/issues/23 and +// https://github.com/hashicorp/hcl2/issues/88 +// As a work around we'll perform surgery directly on text, if backend config is simple. +// The method returns an error, if the config is too complicated to be parsed with the regexp. +// 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*{`) + // Regexp to find simple backend configuration, which doesn't contain '{}' inside + backendBlockRe := regexp.MustCompile(`(?s)(\s*backend\s+"[^"]+"\s*{[^{]*?})`) + if backendDefinitionRe.Match(in) { + indexes := backendBlockRe.FindSubmatchIndex(in) + if indexes == nil { + return nil, fmt.Errorf("unable to delete backend config: unsupported sytax") + } + // Remove found backend submatch from config + return append(in[:indexes[2]], in[indexes[3]:]...), nil + } + return in, nil +} + +func deleteTerraformBackendConfig(in []byte, v *version.Version) (updatedConfig []byte, err error) { + if VersionMatches(v, "<0.12") { + return deleteTerraformBackendConfigWithHCL(in) + } + return deleteTerraformBackendConfigWithHCL2(in) +} + +func deleteTerraformBackendConfigFromFile(file string, v *version.Version) error { logger.Trace.Printf("terraform: deleting backend config from %v", file) b, err := ioutil.ReadFile(file) if err != nil { return err } - updatedConfig, err := deleteTerraformBackendConfig(b) + updatedConfig, err := deleteTerraformBackendConfig(b, v) if err != nil { return err } @@ -107,7 +137,7 @@ func deleteTerraformBackendConfigFromFile(file string) error { return nil } -func parseTerraformConfig(in []byte) (*ast.ObjectList, error) { +func parseTerraformConfigWithHCL(in []byte) (*ast.ObjectList, error) { astFile, err := hcl.ParseBytes(in) if err != nil { return nil, err diff --git a/astro/tests/fixtures/plan-detach/versions.tf b/astro/tests/fixtures/plan-detach/versions.tf new file mode 100644 index 0000000..bd26e2c --- /dev/null +++ b/astro/tests/fixtures/plan-detach/versions.tf @@ -0,0 +1,3 @@ +terraform { + required_version = ">= 0.8" +} From 64bb150abc4d5e9f60fb379a43728e95ad954095 Mon Sep 17 00:00:00 2001 From: Tatiana Romanova Date: Fri, 27 Sep 2019 09:26:43 +0200 Subject: [PATCH 8/9] Review feedback --- .../terraform_remote_state_disable_test.go | 93 +++++++++++-------- .../terraform_remote_state_disable_utils.go | 30 ++++-- 2 files changed, 77 insertions(+), 46 deletions(-) diff --git a/astro/terraform/terraform_remote_state_disable_test.go b/astro/terraform/terraform_remote_state_disable_test.go index 05b94b8..a5b89dd 100644 --- 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 index 6475ff8..295ea08 100644 --- 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 From 618e7f20345775311e673373e72b9d8e7a2f6c2f Mon Sep 17 00:00:00 2001 From: Tatiana Romanova Date: Fri, 27 Sep 2019 12:24:06 +0200 Subject: [PATCH 9/9] Spaces -> tabs --- .../terraform_remote_state_disable_test.go | 74 +++++++++---------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/astro/terraform/terraform_remote_state_disable_test.go b/astro/terraform/terraform_remote_state_disable_test.go index a5b89dd..6065add 100644 --- a/astro/terraform/terraform_remote_state_disable_test.go +++ b/astro/terraform/terraform_remote_state_disable_test.go @@ -78,42 +78,42 @@ func TestDeleteTerraformBackendConfigWithHCL2Success(t *testing.T) { }{ { config: ` - provider "aws"{ - region = var.aws_region - }`, + provider "aws"{ + region = var.aws_region + }`, expected: ` - provider "aws"{ - region = var.aws_region - }`, + provider "aws"{ + region = var.aws_region + }`, }, { config: ` - terraform { - version = "v0.12.6" - backend "local" { - path = "path" - } - key = "value" - }`, + terraform { + version = "v0.12.6" + backend "local" { + path = "path" + } + key = "value" + }`, expected: ` - terraform { - version = "v0.12.6" - key = "value" - }`, + terraform { + version = "v0.12.6" + key = "value" + }`, }, { config: ` - terraform {backend "s3" {}} - - provider "aws" { - region = "us-east-1" - }`, + terraform {backend "s3" {}} + + provider "aws" { + region = "us-east-1" + }`, expected: ` - terraform {} - - provider "aws" { - region = "us-east-1" - }`, + terraform {} + + provider "aws" { + region = "us-east-1" + }`, }, } for _, tt := range tests { @@ -132,19 +132,19 @@ func TestDeleteTerraformBackendConfigWithHCL2Failure(t *testing.T) { }{ { config: ` - terraform { - backend "local" { - path = "module-{{.environment}}" - } - }`, + terraform { + backend "local" { + path = "module-{{.environment}}" + } + }`, }, { config: ` - terraform { - backend "concil" { - map = {"key": "val"} - } - }`, + terraform { + backend "concil" { + map = {"key": "val"} + } + }`, }, }