From 1fdcbc48254173a95a2ae9be203ff82c0170b00b Mon Sep 17 00:00:00 2001 From: Alisdair McDiarmid Date: Thu, 7 May 2020 17:42:40 -0400 Subject: [PATCH] command: Fix 0.13upgrade bug with multiple blocks If a configuration had multiple blocks in the versions.tf file, it would be added to the `rewritePaths` list multiple times. We would then remove it from this slice, but only once, and so the output file would later be rewritten to remove the required providers block. This commit uses a set instead of a list to prevent this case, and adds a regression test. --- command/013_config_upgrade.go | 18 ++++++++---------- .../expected/bar-baz.tf | 3 --- .../013upgrade-multiple-blocks/expected/foo.tf | 3 +++ .../expected/versions.tf | 8 ++++---- .../input/bar-baz.tf | 8 -------- .../013upgrade-multiple-blocks/input/foo.tf | 6 +----- .../input/versions.tf | 12 ++++++++++++ 7 files changed, 28 insertions(+), 30 deletions(-) create mode 100644 command/testdata/013upgrade-multiple-blocks/input/versions.tf diff --git a/command/013_config_upgrade.go b/command/013_config_upgrade.go index ece077db6..997f44f77 100644 --- a/command/013_config_upgrade.go +++ b/command/013_config_upgrade.go @@ -164,12 +164,12 @@ func (c *ZeroThirteenUpgradeCommand) Run(args []string) int { // Build up a list of required providers, uniquely by local name requiredProviders := make(map[string]*configs.RequiredProvider) - var rewritePaths []string + rewritePaths := make(map[string]bool) // Step 1: copy all explicit provider requirements across for path, file := range files { for _, rps := range file.RequiredProviders { - rewritePaths = append(rewritePaths, path) + rewritePaths[path] = true for _, rp := range rps.RequiredProviders { if previous, exist := requiredProviders[rp.Name]; exist { diags = diags.Append(&hcl.Diagnostic{ @@ -254,18 +254,16 @@ func (c *ZeroThirteenUpgradeCommand) Run(args []string) int { // Special case: if we only have one file with a required providers // block, output to that file instead. if len(rewritePaths) == 1 { - filename = rewritePaths[0] + for path := range rewritePaths { + filename = path + break + } } // Remove the output file from the list of paths we want to rewrite // later. Otherwise we'd delete the required providers block after // writing it. - for i, path := range rewritePaths { - if path == filename { - rewritePaths = append(rewritePaths[:i], rewritePaths[i+1:]...) - break - } - } + delete(rewritePaths, filename) // Open or create the output file out, openDiags := c.openOrCreateFile(filename) @@ -456,7 +454,7 @@ func (c *ZeroThirteenUpgradeCommand) Run(args []string) int { // After successfully writing the new configuration, remove all other // required provider blocks from remaining configuration files. - for _, path := range rewritePaths { + for path := range rewritePaths { // Read and parse the existing file config, err := ioutil.ReadFile(path) if err != nil { diff --git a/command/testdata/013upgrade-multiple-blocks/expected/bar-baz.tf b/command/testdata/013upgrade-multiple-blocks/expected/bar-baz.tf index 6887569dd..f2f1d69a0 100644 --- a/command/testdata/013upgrade-multiple-blocks/expected/bar-baz.tf +++ b/command/testdata/013upgrade-multiple-blocks/expected/bar-baz.tf @@ -1,5 +1,2 @@ resource bar_instance a {} resource baz_instance b {} -terraform { - required_version = "> 0.12.0" -} diff --git a/command/testdata/013upgrade-multiple-blocks/expected/foo.tf b/command/testdata/013upgrade-multiple-blocks/expected/foo.tf index 5d9203723..e29e6d315 100644 --- a/command/testdata/013upgrade-multiple-blocks/expected/foo.tf +++ b/command/testdata/013upgrade-multiple-blocks/expected/foo.tf @@ -1 +1,4 @@ +terraform { + required_version = ">= 0.12" +} resource foo_instance c {} diff --git a/command/testdata/013upgrade-multiple-blocks/expected/versions.tf b/command/testdata/013upgrade-multiple-blocks/expected/versions.tf index bcfd89046..ba49a0da7 100644 --- a/command/testdata/013upgrade-multiple-blocks/expected/versions.tf +++ b/command/testdata/013upgrade-multiple-blocks/expected/versions.tf @@ -1,5 +1,9 @@ terraform { required_providers { + foo = { + source = "hashicorp/foo" + version = "0.5" + } bar = { source = "registry.acme.corp/acme/bar" } @@ -7,10 +11,6 @@ terraform { source = "terraform-providers/baz" version = "~> 2.0.0" } - foo = { - source = "hashicorp/foo" - version = "0.5" - } } required_version = ">= 0.13" } diff --git a/command/testdata/013upgrade-multiple-blocks/input/bar-baz.tf b/command/testdata/013upgrade-multiple-blocks/input/bar-baz.tf index d0e6fe8c6..f2f1d69a0 100644 --- a/command/testdata/013upgrade-multiple-blocks/input/bar-baz.tf +++ b/command/testdata/013upgrade-multiple-blocks/input/bar-baz.tf @@ -1,10 +1,2 @@ resource bar_instance a {} resource baz_instance b {} -terraform { - required_version = "> 0.12.0" - required_providers { - bar = { - source = "registry.acme.corp/acme/bar" - } - } -} diff --git a/command/testdata/013upgrade-multiple-blocks/input/foo.tf b/command/testdata/013upgrade-multiple-blocks/input/foo.tf index 99aaf52a0..582cf0e2d 100644 --- a/command/testdata/013upgrade-multiple-blocks/input/foo.tf +++ b/command/testdata/013upgrade-multiple-blocks/input/foo.tf @@ -1,11 +1,7 @@ terraform { + required_version = ">= 0.12" required_providers { baz = "~> 2.0.0" } } -terraform { - required_providers { - foo = "0.5" - } -} resource foo_instance c {} diff --git a/command/testdata/013upgrade-multiple-blocks/input/versions.tf b/command/testdata/013upgrade-multiple-blocks/input/versions.tf new file mode 100644 index 000000000..9252a15e3 --- /dev/null +++ b/command/testdata/013upgrade-multiple-blocks/input/versions.tf @@ -0,0 +1,12 @@ +terraform { + required_providers { + foo = "0.5" + } +} +terraform { + required_providers { + bar = { + source = "registry.acme.corp/acme/bar" + } + } +}