From 79dc80861433c7d3096937f5c503178242aba045 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 8 Nov 2019 14:48:52 -0800 Subject: [PATCH] configs: Emit warnings for deprecated quoted references/keywords Terraform 0.12.0 removed the need for putting references and keywords in quotes, but we disabled the deprecation warnings for the initial release in order to avoid creating noise for folks who were intentionally attempting to maintain modules that were cross-compatible with both Terraform 0.11 and Terraform 0.12. However, with Terraform 0.12 now more widely used, the lack of these warnings seems to be causing newcomers to copy the quoted versions from existing examples on the internet, which is perpetuating the old and confusing quoted form in newer configurations. In preparation for phasing out these deprecated forms altogether in a future major release, and for the shorter-term benefit of giving better feedback to newcomers when they are learning from outdated examples, we'll now re-enable those deprecation warnings, and be explicit that the old forms are intended for removal in a future release. In order to properly test this, we establish a new set of test configurations that explicitly mark which warnings they are expecting and verify that they do indeed produce those expected warnings. We also verify that the "success" tests do _not_ produce warnings, while removing the ones that were previously written to succeed but have their warnings ignored. --- configs/compat_shim.go | 37 ++++---- configs/parser_config_test.go | 84 ++++++++++++++++--- .../valid-files/resources-dependson-quoted.tf | 8 -- .../resources-ignorechanges-all-legacy.tf | 5 -- ...resources-ignorechanges-all-legacy.tf.json | 11 --- .../resources-ignorechanges-quoted.tf | 7 -- .../resources-provisioner-onfailure-quoted.tf | 6 -- .../resources-provisioner-when-quoted.tf | 6 -- configs/testdata/warning-files/depends_on.tf | 6 ++ .../testdata/warning-files/ignore_changes.tf | 11 +++ .../testdata/warning-files/provider_ref.tf | 7 ++ .../warning-files/provisioner_keyword.tf | 6 ++ 12 files changed, 117 insertions(+), 77 deletions(-) delete mode 100644 configs/testdata/valid-files/resources-dependson-quoted.tf delete mode 100644 configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf delete mode 100644 configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf.json delete mode 100644 configs/testdata/valid-files/resources-ignorechanges-quoted.tf delete mode 100644 configs/testdata/valid-files/resources-provisioner-onfailure-quoted.tf delete mode 100644 configs/testdata/valid-files/resources-provisioner-when-quoted.tf create mode 100644 configs/testdata/warning-files/depends_on.tf create mode 100644 configs/testdata/warning-files/ignore_changes.tf create mode 100644 configs/testdata/warning-files/provider_ref.tf create mode 100644 configs/testdata/warning-files/provisioner_keyword.tf diff --git a/configs/compat_shim.go b/configs/compat_shim.go index e594ebd40..79360201e 100644 --- a/configs/compat_shim.go +++ b/configs/compat_shim.go @@ -69,28 +69,21 @@ func shimTraversalInString(expr hcl.Expression, wantKeyword bool) (hcl.Expressio ) diags = append(diags, tDiags...) - // For initial release our deprecation warnings are disabled to allow - // a period where modules can be compatible with both old and new - // conventions. - // FIXME: Re-enable these deprecation warnings in a release prior to - // Terraform 0.13 and then remove the shims altogether for 0.13. - /* - if wantKeyword { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "Quoted keywords are deprecated", - Detail: "In this context, keywords are expected literally rather than in quotes. Previous versions of Terraform required quotes, but that usage is now deprecated. Remove the quotes surrounding this keyword to silence this warning.", - Subject: &srcRange, - }) - } else { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagWarning, - Summary: "Quoted references are deprecated", - Detail: "In this context, references are expected literally rather than in quotes. Previous versions of Terraform required quotes, but that usage is now deprecated. Remove the quotes surrounding this reference to silence this warning.", - Subject: &srcRange, - }) - } - */ + if wantKeyword { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Quoted keywords are deprecated", + Detail: "In this context, keywords are expected literally rather than in quotes. Terraform 0.11 and earlier required quotes, but quoted keywords are now deprecated and will be removed in a future version of Terraform. Remove the quotes surrounding this keyword to silence this warning.", + Subject: &srcRange, + }) + } else { + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Quoted references are deprecated", + Detail: "In this context, references are expected literally rather than in quotes. Terraform 0.11 and earlier required quotes, but quoted references are now deprecated and will be removed in a future version of Terraform. Remove the quotes surrounding this reference to silence this warning.", + Subject: &srcRange, + }) + } return &hclsyntax.ScopeTraversalExpr{ Traversal: traversal, diff --git a/configs/parser_config_test.go b/configs/parser_config_test.go index 22272cf07..13d90fed6 100644 --- a/configs/parser_config_test.go +++ b/configs/parser_config_test.go @@ -1,10 +1,15 @@ package configs import ( + "bufio" + "bytes" "io/ioutil" "path/filepath" + "strings" "testing" + "github.com/google/go-cmp/cmp" + "github.com/hashicorp/hcl/v2" ) @@ -34,8 +39,8 @@ func TestParserLoadConfigFileSuccess(t *testing.T) { }) _, diags := parser.LoadConfigFile(name) - if diags.HasErrors() { - t.Errorf("unexpected error diagnostics") + if len(diags) != 0 { + t.Errorf("unexpected diagnostics") for _, diag := range diags { t.Logf("- %s", diag) } @@ -124,16 +129,6 @@ func TestParserLoadConfigFileFailureMessages(t *testing.T) { hcl.DiagError, "Unsuitable value type", }, - { - "valid-files/resources-ignorechanges-all-legacy.tf", - hcl.DiagWarning, - "Deprecated ignore_changes wildcard", - }, - { - "valid-files/resources-ignorechanges-all-legacy.tf.json", - hcl.DiagWarning, - "Deprecated ignore_changes wildcard", - }, } for _, test := range tests { @@ -164,3 +159,68 @@ func TestParserLoadConfigFileFailureMessages(t *testing.T) { }) } } + +// TestParseLoadConfigFileWarning is a test that verifies files from +// testdata/warning-files produce particular warnings. +// +// This test does not verify that reading these files produces the correct +// file element contents in spite of those warnings. More detailed assertions +// may be made on some subset of these configuration files in other tests. +func TestParserLoadConfigFileWarning(t *testing.T) { + files, err := ioutil.ReadDir("testdata/warning-files") + if err != nil { + t.Fatal(err) + } + + for _, info := range files { + name := info.Name() + t.Run(name, func(t *testing.T) { + src, err := ioutil.ReadFile(filepath.Join("testdata/warning-files", name)) + if err != nil { + t.Fatal(err) + } + + // First we'll scan the file to see what warnings are expected. + // That's declared inside the files themselves by using the + // string "WARNING: " somewhere on each line that is expected + // to produce a warning, followed by the expected warning summary + // text. A single-line comment (with #) is the main way to do that. + const marker = "WARNING: " + sc := bufio.NewScanner(bytes.NewReader(src)) + wantWarnings := make(map[int]string) + lineNum := 1 + for sc.Scan() { + lineText := sc.Text() + if idx := strings.Index(lineText, marker); idx != -1 { + summaryText := lineText[idx+len(marker):] + wantWarnings[lineNum] = summaryText + } + lineNum++ + } + + parser := testParser(map[string]string{ + name: string(src), + }) + + _, diags := parser.LoadConfigFile(name) + if diags.HasErrors() { + t.Errorf("unexpected error diagnostics") + for _, diag := range diags { + t.Logf("- %s", diag) + } + } + + gotWarnings := make(map[int]string) + for _, diag := range diags { + if diag.Severity != hcl.DiagWarning || diag.Subject == nil { + continue + } + gotWarnings[diag.Subject.Start.Line] = diag.Summary + } + + if diff := cmp.Diff(wantWarnings, gotWarnings); diff != "" { + t.Errorf("wrong warnings\n%s", diff) + } + }) + } +} diff --git a/configs/testdata/valid-files/resources-dependson-quoted.tf b/configs/testdata/valid-files/resources-dependson-quoted.tf deleted file mode 100644 index 3bf188f19..000000000 --- a/configs/testdata/valid-files/resources-dependson-quoted.tf +++ /dev/null @@ -1,8 +0,0 @@ -resource "aws_security_group" "firewall" { -} - -resource "aws_instance" "web" { - depends_on = [ - "aws_security_group.firewall", - ] -} diff --git a/configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf b/configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf deleted file mode 100644 index 6b5e61a9c..000000000 --- a/configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf +++ /dev/null @@ -1,5 +0,0 @@ -resource "aws_instance" "web" { - lifecycle { - ignore_changes = ["*"] - } -} diff --git a/configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf.json b/configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf.json deleted file mode 100644 index 5502dcd50..000000000 --- a/configs/testdata/valid-files/resources-ignorechanges-all-legacy.tf.json +++ /dev/null @@ -1,11 +0,0 @@ -{ - "resource": { - "aws_instance": { - "web": { - "lifecycle": { - "ignore_changes": ["*"] - } - } - } - } -} diff --git a/configs/testdata/valid-files/resources-ignorechanges-quoted.tf b/configs/testdata/valid-files/resources-ignorechanges-quoted.tf deleted file mode 100644 index cba5be59d..000000000 --- a/configs/testdata/valid-files/resources-ignorechanges-quoted.tf +++ /dev/null @@ -1,7 +0,0 @@ -resource "aws_instance" "web" { - lifecycle { - ignore_changes = [ - "ami", - ] - } -} diff --git a/configs/testdata/valid-files/resources-provisioner-onfailure-quoted.tf b/configs/testdata/valid-files/resources-provisioner-onfailure-quoted.tf deleted file mode 100644 index dcec1eb08..000000000 --- a/configs/testdata/valid-files/resources-provisioner-onfailure-quoted.tf +++ /dev/null @@ -1,6 +0,0 @@ -resource "aws_security_group" "firewall" { - provisioner "local-exec" { - command = "echo hello" - on_failure = "continue" - } -} diff --git a/configs/testdata/valid-files/resources-provisioner-when-quoted.tf b/configs/testdata/valid-files/resources-provisioner-when-quoted.tf deleted file mode 100644 index 6a66b085f..000000000 --- a/configs/testdata/valid-files/resources-provisioner-when-quoted.tf +++ /dev/null @@ -1,6 +0,0 @@ -resource "aws_security_group" "firewall" { - provisioner "local-exec" { - command = "echo hello" - when = "destroy" - } -} diff --git a/configs/testdata/warning-files/depends_on.tf b/configs/testdata/warning-files/depends_on.tf new file mode 100644 index 000000000..17e1bf34a --- /dev/null +++ b/configs/testdata/warning-files/depends_on.tf @@ -0,0 +1,6 @@ +resource "null_resource" "a" { +} + +resource "null_resource" "b" { + depends_on = ["null_resource.a"] # WARNING: Quoted references are deprecated +} diff --git a/configs/testdata/warning-files/ignore_changes.tf b/configs/testdata/warning-files/ignore_changes.tf new file mode 100644 index 000000000..5678fe7bd --- /dev/null +++ b/configs/testdata/warning-files/ignore_changes.tf @@ -0,0 +1,11 @@ +resource "null_resource" "one" { + lifecycle { + ignore_changes = ["triggers"] # WARNING: Quoted references are deprecated + } +} + +resource "null_resource" "all" { + lifecycle { + ignore_changes = ["*"] # WARNING: Deprecated ignore_changes wildcard + } +} diff --git a/configs/testdata/warning-files/provider_ref.tf b/configs/testdata/warning-files/provider_ref.tf new file mode 100644 index 000000000..6f5525ed7 --- /dev/null +++ b/configs/testdata/warning-files/provider_ref.tf @@ -0,0 +1,7 @@ +provider "null" { + alias = "foo" +} + +resource "null_resource" "test" { + provider = "null.foo" # WARNING: Quoted references are deprecated +} diff --git a/configs/testdata/warning-files/provisioner_keyword.tf b/configs/testdata/warning-files/provisioner_keyword.tf new file mode 100644 index 000000000..61fe72bdd --- /dev/null +++ b/configs/testdata/warning-files/provisioner_keyword.tf @@ -0,0 +1,6 @@ +resource "null_resource" "a" { + provisioner "local-exec" { + when = "create" # WARNING: Quoted keywords are deprecated + on_failure = "fail" # WARNING: Quoted keywords are deprecated + } +}