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.
This commit is contained in:
Martin Atkins 2019-11-08 14:48:52 -08:00
parent 2423f266fb
commit 79dc808614
12 changed files with 117 additions and 77 deletions

View File

@ -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.",
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. Previous versions of Terraform required quotes, but that usage is now deprecated. Remove the quotes surrounding this reference to silence this warning.",
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,

View File

@ -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)
}
})
}
}

View File

@ -1,8 +0,0 @@
resource "aws_security_group" "firewall" {
}
resource "aws_instance" "web" {
depends_on = [
"aws_security_group.firewall",
]
}

View File

@ -1,5 +0,0 @@
resource "aws_instance" "web" {
lifecycle {
ignore_changes = ["*"]
}
}

View File

@ -1,11 +0,0 @@
{
"resource": {
"aws_instance": {
"web": {
"lifecycle": {
"ignore_changes": ["*"]
}
}
}
}
}

View File

@ -1,7 +0,0 @@
resource "aws_instance" "web" {
lifecycle {
ignore_changes = [
"ami",
]
}
}

View File

@ -1,6 +0,0 @@
resource "aws_security_group" "firewall" {
provisioner "local-exec" {
command = "echo hello"
on_failure = "continue"
}
}

View File

@ -1,6 +0,0 @@
resource "aws_security_group" "firewall" {
provisioner "local-exec" {
command = "echo hello"
when = "destroy"
}
}

View File

@ -0,0 +1,6 @@
resource "null_resource" "a" {
}
resource "null_resource" "b" {
depends_on = ["null_resource.a"] # WARNING: Quoted references are deprecated
}

View File

@ -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
}
}

View File

@ -0,0 +1,7 @@
provider "null" {
alias = "foo"
}
resource "null_resource" "test" {
provider = "null.foo" # WARNING: Quoted references are deprecated
}

View File

@ -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
}
}