From a3455335730b0c68514d65c32e459b1e2480a3bc Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Sat, 10 Mar 2018 11:44:05 -0800 Subject: [PATCH] configupgrade: Beginnings of Upgrade function This function is the main functionality of this package. So far it just deals with detecting and renaming JSON files that are mislabeled as native syntax files. Other functionality will follow in later commits. --- .../test-fixtures/valid/noop/want/outputs.tf | 4 + .../valid/noop/want/providers.tf | 11 ++ .../valid/noop/want/resources.tf | 3 + .../valid/noop/want/variables.tf | 12 ++ .../test-fixtures/valid/noop/want/versions.tf | 0 .../valid/rename-json-conflict/input/foo.tf | 1 + .../rename-json-conflict/input/foo.tf.json | 3 + .../rename-json-conflict/want/foo-1.tf.json | 1 + .../rename-json-conflict/want/foo.tf.json | 3 + .../rename-json-conflict/want/versions.tf | 0 .../valid/rename-json/input/misnamed-json.tf | 3 + .../rename-json/want/misnamed-json.tf.json | 3 + .../valid/rename-json/want/versions.tf | 0 configs/configupgrade/upgrade.go | 104 ++++++++++++++++++ configs/configupgrade/upgrade_test.go | 98 +++++++++++++++++ 15 files changed, 246 insertions(+) create mode 100644 configs/configupgrade/test-fixtures/valid/noop/want/outputs.tf create mode 100644 configs/configupgrade/test-fixtures/valid/noop/want/providers.tf create mode 100644 configs/configupgrade/test-fixtures/valid/noop/want/resources.tf create mode 100644 configs/configupgrade/test-fixtures/valid/noop/want/variables.tf create mode 100644 configs/configupgrade/test-fixtures/valid/noop/want/versions.tf create mode 100644 configs/configupgrade/test-fixtures/valid/rename-json-conflict/input/foo.tf create mode 100644 configs/configupgrade/test-fixtures/valid/rename-json-conflict/input/foo.tf.json create mode 100644 configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/foo-1.tf.json create mode 100644 configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/foo.tf.json create mode 100644 configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/versions.tf create mode 100644 configs/configupgrade/test-fixtures/valid/rename-json/input/misnamed-json.tf create mode 100644 configs/configupgrade/test-fixtures/valid/rename-json/want/misnamed-json.tf.json create mode 100644 configs/configupgrade/test-fixtures/valid/rename-json/want/versions.tf create mode 100644 configs/configupgrade/upgrade.go create mode 100644 configs/configupgrade/upgrade_test.go diff --git a/configs/configupgrade/test-fixtures/valid/noop/want/outputs.tf b/configs/configupgrade/test-fixtures/valid/noop/want/outputs.tf new file mode 100644 index 000000000..e6706feea --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/noop/want/outputs.tf @@ -0,0 +1,4 @@ + +output "foo" { + value = "jeepers ${var.bar}" +} diff --git a/configs/configupgrade/test-fixtures/valid/noop/want/providers.tf b/configs/configupgrade/test-fixtures/valid/noop/want/providers.tf new file mode 100644 index 000000000..b5cdd2295 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/noop/want/providers.tf @@ -0,0 +1,11 @@ + +terraform { + required_version = ">= 0.7.0, <0.13.0" + + backend "local" { + path = "foo.tfstate" + } +} + +provider "test" { +} diff --git a/configs/configupgrade/test-fixtures/valid/noop/want/resources.tf b/configs/configupgrade/test-fixtures/valid/noop/want/resources.tf new file mode 100644 index 000000000..9e2c24666 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/noop/want/resources.tf @@ -0,0 +1,3 @@ + +resource "test_resource" "example" { +} diff --git a/configs/configupgrade/test-fixtures/valid/noop/want/variables.tf b/configs/configupgrade/test-fixtures/valid/noop/want/variables.tf new file mode 100644 index 000000000..dcd9c1177 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/noop/want/variables.tf @@ -0,0 +1,12 @@ + +# This comment should survive +variable "foo" { + default = 1 // This comment should also survive +} + +variable "bar" { + /* This comment should survive too */ + description = "bar the baz" +} + +// This comment that isn't attached to anything should survive. diff --git a/configs/configupgrade/test-fixtures/valid/noop/want/versions.tf b/configs/configupgrade/test-fixtures/valid/noop/want/versions.tf new file mode 100644 index 000000000..e69de29bb diff --git a/configs/configupgrade/test-fixtures/valid/rename-json-conflict/input/foo.tf b/configs/configupgrade/test-fixtures/valid/rename-json-conflict/input/foo.tf new file mode 100644 index 000000000..0967ef424 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/rename-json-conflict/input/foo.tf @@ -0,0 +1 @@ +{} diff --git a/configs/configupgrade/test-fixtures/valid/rename-json-conflict/input/foo.tf.json b/configs/configupgrade/test-fixtures/valid/rename-json-conflict/input/foo.tf.json new file mode 100644 index 000000000..2e2809093 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/rename-json-conflict/input/foo.tf.json @@ -0,0 +1,3 @@ +{ + "terraform": {} +} diff --git a/configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/foo-1.tf.json b/configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/foo-1.tf.json new file mode 100644 index 000000000..0967ef424 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/foo-1.tf.json @@ -0,0 +1 @@ +{} diff --git a/configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/foo.tf.json b/configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/foo.tf.json new file mode 100644 index 000000000..2e2809093 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/foo.tf.json @@ -0,0 +1,3 @@ +{ + "terraform": {} +} diff --git a/configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/versions.tf b/configs/configupgrade/test-fixtures/valid/rename-json-conflict/want/versions.tf new file mode 100644 index 000000000..e69de29bb diff --git a/configs/configupgrade/test-fixtures/valid/rename-json/input/misnamed-json.tf b/configs/configupgrade/test-fixtures/valid/rename-json/input/misnamed-json.tf new file mode 100644 index 000000000..2e2809093 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/rename-json/input/misnamed-json.tf @@ -0,0 +1,3 @@ +{ + "terraform": {} +} diff --git a/configs/configupgrade/test-fixtures/valid/rename-json/want/misnamed-json.tf.json b/configs/configupgrade/test-fixtures/valid/rename-json/want/misnamed-json.tf.json new file mode 100644 index 000000000..2e2809093 --- /dev/null +++ b/configs/configupgrade/test-fixtures/valid/rename-json/want/misnamed-json.tf.json @@ -0,0 +1,3 @@ +{ + "terraform": {} +} diff --git a/configs/configupgrade/test-fixtures/valid/rename-json/want/versions.tf b/configs/configupgrade/test-fixtures/valid/rename-json/want/versions.tf new file mode 100644 index 000000000..e69de29bb diff --git a/configs/configupgrade/upgrade.go b/configs/configupgrade/upgrade.go new file mode 100644 index 000000000..3e0c3d0df --- /dev/null +++ b/configs/configupgrade/upgrade.go @@ -0,0 +1,104 @@ +package configupgrade + +import ( + "bytes" + "fmt" + + "github.com/hashicorp/terraform/tfdiags" + + hcl2 "github.com/hashicorp/hcl2/hcl" +) + +// Upgrade takes some input module sources and produces a new ModuleSources +// that should be equivalent to the input but use the configuration idioms +// associated with the new configuration loader. +// +// The result of this function will probably not be accepted by this function, +// because it will contain constructs that are known only to the new +// loader. +// +// The result may include additional files that were not present in the +// input. The result may also include nil entries for filenames that were +// present in the input, indicating that these files should be deleted. +// In particular, file renames are represented as a new entry accompanied +// by a nil entry for the old name. +// +// If the returned diagnostics contains errors, the caller should not write +// the resulting sources to disk since they will probably be incomplete. If +// only warnings are present then the files may be written to disk. Most +// warnings are also represented as "TF-UPGRADE-TODO:" comments in the +// generated source files so that users can visit them all and decide what to +// do with them. +func Upgrade(input ModuleSources) (ModuleSources, tfdiags.Diagnostics) { + ret := make(ModuleSources) + var diags tfdiags.Diagnostics + + for name, src := range input { + ext := fileExt(name) + if ext == "" { + // This should never happen because we ignore files that don't + // have our conventional extensions during LoadModule, but we'll + // silently pass through such files assuming that the caller + // has been tampering with the sources map somehow. + ret[name] = src + continue + } + + isJSON := (ext == ".tf.json") + + // The legacy loader allowed JSON syntax inside files named just .tf, + // so we'll detect that case and rename them here so that the new + // loader will accept the JSON. However, JSON files are usually + // generated so we'll also generate a warning to the user to update + // whatever program generated the file to use the new name. + if !isJSON { + trimSrc := bytes.TrimSpace(src) + if len(trimSrc) > 0 && (trimSrc[0] == '{' || trimSrc[0] == '[') { + isJSON = true + + // Rename in the output + ret[name] = nil // mark for deletion + oldName := name + name = input.UnusedFilename(name + ".json") + + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagWarning, + Summary: "JSON configuration file was renamed", + Detail: fmt.Sprintf( + "The file %q appears to be in JSON format, so it was renamed to %q. If this file is generated by another program, that program must be updated to use this new name.", + oldName, name, + ), + }) + } + } + + if isJSON { + // We don't do any automatic rewriting for JSON files, since they + // are usually generated and thus it's the generating program that + // needs to be updated, rather than its output. + ret[name] = src + diags = diags.Append(&hcl2.Diagnostic{ + Severity: hcl2.DiagWarning, + Summary: "JSON configuration file was not rewritten", + Detail: fmt.Sprintf( + "The JSON configuration file %q was skipped, because JSON files are assumed to be generated. The program that generated this file may need to be updated for changes to the configuration language.", + name, + ), + }) + continue + } + + // TODO: Actually rewrite this .tf file. + ret[name] = src + } + + // Generate our versions.tf file that both records the fact that we now + // require Terraform Core 0.12 and gathers all of the provider version + // requirements that might previously have been scattered in various + // "provider" blocks elsewhere. + versionsName := ret.UnusedFilename("versions.tf") + // TODO: Actually populate this. + ret[versionsName] = make([]byte, 0) + + return ret, diags +} diff --git a/configs/configupgrade/upgrade_test.go b/configs/configupgrade/upgrade_test.go new file mode 100644 index 000000000..0f03d3c59 --- /dev/null +++ b/configs/configupgrade/upgrade_test.go @@ -0,0 +1,98 @@ +package configupgrade + +import ( + "bytes" + "io/ioutil" + "path/filepath" + "testing" +) + +func TestUpgradeValid(t *testing.T) { + // This test uses the contents of the test-fixtures/valid directory as + // a table of tests. Every directory there must have both "input" and + // "want" subdirectories, where "input" is the configuration to be + // upgraded and "want" is the expected result. + fixtureDir := "test-fixtures/valid" + testDirs, err := ioutil.ReadDir(fixtureDir) + if err != nil { + t.Fatal(err) + } + + for _, entry := range testDirs { + if !entry.IsDir() { + continue + } + t.Run(entry.Name(), func(t *testing.T) { + inputDir := filepath.Join(fixtureDir, entry.Name(), "input") + wantDir := filepath.Join(fixtureDir, entry.Name(), "want") + + inputSrc, err := LoadModule(inputDir) + if err != nil { + t.Fatal(err) + } + wantSrc, err := LoadModule(wantDir) + if err != nil { + t.Fatal(err) + } + + gotSrc, diags := Upgrade(inputSrc) + if diags.HasErrors() { + t.Error(diags.Err()) + } + + // Upgrade uses a nil entry as a signal to delete a file, which + // we can't test here because we aren't modifying an existing + // dir in place, so we'll just ignore those and leave that mechanism + // to be tested elsewhere. + + for name, got := range gotSrc { + if gotSrc[name] == nil { + delete(gotSrc, name) + continue + } + want, wanted := wantSrc[name] + if !wanted { + t.Errorf("unexpected extra output file %q\n=== GOT ===\n%s", name, got) + continue + } + + if !bytes.Equal(got, want) { + t.Errorf("wrong content in %q\n=== GOT ===\n%s\n=== WANT ===\n%s", name, got, want) + } + } + + for name, want := range wantSrc { + if _, present := gotSrc[name]; !present { + t.Errorf("missing output file %q\n=== WANT ===\n%s", name, want) + } + } + }) + } +} + +func TestUpgradeRenameJSON(t *testing.T) { + inputDir := filepath.Join("test-fixtures/valid/rename-json/input") + inputSrc, err := LoadModule(inputDir) + if err != nil { + t.Fatal(err) + } + + gotSrc, diags := Upgrade(inputSrc) + if diags.HasErrors() { + t.Error(diags.Err()) + } + + // This test fixture is also fully covered by TestUpgradeValid, so + // we're just testing that the file was renamed here. + src, exists := gotSrc["misnamed-json.tf"] + if src != nil { + t.Errorf("misnamed-json.tf still has content") + } else if !exists { + t.Errorf("misnamed-json.tf not marked for deletion") + } + + src, exists = gotSrc["misnamed-json.tf.json"] + if src == nil || !exists { + t.Errorf("misnamed-json.tf.json was not created") + } +}