From 43a1946935a3a3008923310fa155685891aba195 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Tue, 12 Nov 2019 12:10:29 -0800 Subject: [PATCH] configs: Warn for deprecated interpolation and quoted type constraints Following on from de652e22a26b, this introduces deprecation warnings for when an attribute value expression is a template with only a single interpolation sequence, and for variable type constraints given in quotes. As with the previous commit, we allowed these deprecated forms with no warning for a few releases after v0.12.0 to ensure that folks who need to write cross-compatible modules for a while during upgrading would be able to do so, but we're now marking these as explicitly deprecated to guide users towards the new idiomatic forms. The "terraform 0.12upgrade" tool would've already updated configurations to not hit these warnings for those who had pre-existing configurations written for Terraform 0.11. The main target audience for these warnings are newcomers to Terraform who are learning from existing examples already published in various spots on the wider internet that may be showing older Terraform syntax, since those folks will not be running their configurations through the upgrade tool. These warnings will hopefully guide them towards modern Terraform usage during their initial experimentation, and thus reduce the chances of inadvertently adopting the less-readable legacy usage patterns in greenfield projects. --- configs/compat_shim.go | 55 +++++++++++++++++++ configs/named_values.go | 18 ++++++ configs/provider.go | 12 +++- configs/resource.go | 11 +++- .../testdata/valid-files/references.tf.json | 11 ++++ .../valid-files/variable-type-quoted.tf | 3 - .../warning-files/redundant_interp.tf | 36 ++++++++++++ .../warning-files/variable_type_quoted.tf | 11 ++++ 8 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 configs/testdata/valid-files/references.tf.json delete mode 100644 configs/testdata/valid-files/variable-type-quoted.tf create mode 100644 configs/testdata/warning-files/redundant_interp.tf create mode 100644 configs/testdata/warning-files/variable_type_quoted.tf diff --git a/configs/compat_shim.go b/configs/compat_shim.go index 79360201e5ed..31158972d749 100644 --- a/configs/compat_shim.go +++ b/configs/compat_shim.go @@ -107,3 +107,58 @@ func shimIsIgnoreChangesStar(expr hcl.Expression) bool { } return val.AsString() == "*" } + +// warnForDeprecatedInterpolations returns warning diagnostics if the given +// body can be proven to contain attributes whose expressions are native +// syntax expressions consisting entirely of a single template interpolation, +// which is a deprecated way to include a non-literal value in configuration. +// +// This is a best-effort sort of thing which relies on the physical HCL native +// syntax AST, so it might not catch everything. The main goal is to catch the +// "obvious" cases in order to help spread awareness that this old form is +// deprecated, when folks copy it from older examples they've found on the +// internet that were written for Terraform 0.11 or earlier. +func warnForDeprecatedInterpolationsInBody(body hcl.Body) hcl.Diagnostics { + var diags hcl.Diagnostics + + nativeBody, ok := body.(*hclsyntax.Body) + if !ok { + // If it's not native syntax then we've nothing to do here. + return diags + } + + for _, attr := range nativeBody.Attributes { + moreDiags := warnForDeprecatedInterpolationsInExpr(attr.Expr) + diags = append(diags, moreDiags...) + } + + for _, block := range nativeBody.Blocks { + // We'll also go hunting in nested blocks + moreDiags := warnForDeprecatedInterpolationsInBody(block.Body) + diags = append(diags, moreDiags...) + } + + return diags +} + +func warnForDeprecatedInterpolationsInExpr(expr hcl.Expression) hcl.Diagnostics { + var diags hcl.Diagnostics + + if _, ok := expr.(*hclsyntax.TemplateWrapExpr); !ok { + // We're only interested in TemplateWrapExpr, because that's how + // the HCL native syntax parser represents the case of a template + // that consists entirely of a single interpolation expression, which + // is therefore subject to the special case of passing through the + // inner value without conversion to string. + return diags + } + + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Interpolation-only expressions are deprecated", + Detail: "Terraform 0.11 and earlier required all non-constant expressions to be provided via interpolation syntax, but this pattern is now deprecated. To silence this warning, remove the \"${ sequence from the start and the }\" sequence from the end of this expression, leaving just the inner expression.\n\nTemplate interpolation syntax can still be used to construct strings from expressions when the template includes multiple interpolation sequences or a mixture of literal strings and interpolations. This deprecation applies only to templates that consist entirely of a single interpolation sequence.", + Subject: expr.Range().Ptr(), + }) + + return diags +} diff --git a/configs/named_values.go b/configs/named_values.go index b88f06bf20a9..280b7069284e 100644 --- a/configs/named_values.go +++ b/configs/named_values.go @@ -138,10 +138,28 @@ func decodeVariableType(expr hcl.Expression) (cty.Type, VariableParsingMode, hcl str := val.AsString() switch str { case "string": + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Quoted type constraints are deprecated", + Detail: "Terraform 0.11 and earlier required type constraints to be given in quotes, but that form is now deprecated and will be removed in a future version of Terraform. To silence this warning, remove the quotes around \"string\".", + Subject: expr.Range().Ptr(), + }) return cty.String, VariableParseLiteral, diags case "list": + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Quoted type constraints are deprecated", + Detail: "Terraform 0.11 and earlier required type constraints to be given in quotes, but that form is now deprecated and will be removed in a future version of Terraform. To silence this warning, remove the quotes around \"list\" and write list(string) instead to explicitly indicate that the list elements are strings.", + Subject: expr.Range().Ptr(), + }) return cty.List(cty.DynamicPseudoType), VariableParseHCL, diags case "map": + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagWarning, + Summary: "Quoted type constraints are deprecated", + Detail: "Terraform 0.11 and earlier required type constraints to be given in quotes, but that form is now deprecated and will be removed in a future version of Terraform. To silence this warning, remove the quotes around \"map\" and write map(string) instead to explicitly indicate that the map elements are strings.", + Subject: expr.Range().Ptr(), + }) return cty.Map(cty.DynamicPseudoType), VariableParseHCL, diags default: return cty.DynamicPseudoType, VariableParseHCL, hcl.Diagnostics{{ diff --git a/configs/provider.go b/configs/provider.go index 30a0629405da..17754a669d34 100644 --- a/configs/provider.go +++ b/configs/provider.go @@ -27,7 +27,17 @@ type Provider struct { } func decodeProviderBlock(block *hcl.Block) (*Provider, hcl.Diagnostics) { - content, config, diags := block.Body.PartialContent(providerBlockSchema) + var diags hcl.Diagnostics + + // Produce deprecation messages for any pre-0.12-style + // single-interpolation-only expressions. We do this up front here because + // then we can also catch instances inside special blocks like "connection", + // before PartialContent extracts them. + moreDiags := warnForDeprecatedInterpolationsInBody(block.Body) + diags = append(diags, moreDiags...) + + content, config, moreDiags := block.Body.PartialContent(providerBlockSchema) + diags = append(diags, moreDiags...) provider := &Provider{ Name: block.Labels[0], diff --git a/configs/resource.go b/configs/resource.go index d63fff4a4236..4d5506e293f7 100644 --- a/configs/resource.go +++ b/configs/resource.go @@ -76,6 +76,7 @@ func (r *Resource) ProviderConfigAddr() addrs.ProviderConfig { } func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { + var diags hcl.Diagnostics r := &Resource{ Mode: addrs.ManagedResourceMode, Type: block.Labels[0], @@ -85,7 +86,15 @@ func decodeResourceBlock(block *hcl.Block) (*Resource, hcl.Diagnostics) { Managed: &ManagedResource{}, } - content, remain, diags := block.Body.PartialContent(resourceBlockSchema) + // Produce deprecation messages for any pre-0.12-style + // single-interpolation-only expressions. We do this up front here because + // then we can also catch instances inside special blocks like "connection", + // before PartialContent extracts them. + moreDiags := warnForDeprecatedInterpolationsInBody(block.Body) + diags = append(diags, moreDiags...) + + content, remain, moreDiags := block.Body.PartialContent(resourceBlockSchema) + diags = append(diags, moreDiags...) r.Config = remain if !hclsyntax.ValidIdentifier(r.Type) { diff --git a/configs/testdata/valid-files/references.tf.json b/configs/testdata/valid-files/references.tf.json new file mode 100644 index 000000000000..3fe7e0afff34 --- /dev/null +++ b/configs/testdata/valid-files/references.tf.json @@ -0,0 +1,11 @@ +{ + "//": "The purpose of this test file is to show that we can use template syntax unwrapping to provide complex expressions without generating the deprecation warnings we'd expect for native syntax.", + "resource": { + "null_resource": { + "baz": { + "//": "This particular use of template syntax is redundant, but we permit it because this is the documented way to use more complex expressions in JSON.", + "triggers": "${ {} }" + } + } + } +} diff --git a/configs/testdata/valid-files/variable-type-quoted.tf b/configs/testdata/valid-files/variable-type-quoted.tf deleted file mode 100644 index 15db803f23b0..000000000000 --- a/configs/testdata/valid-files/variable-type-quoted.tf +++ /dev/null @@ -1,3 +0,0 @@ -variable "bad_type" { - type = "string" -} diff --git a/configs/testdata/warning-files/redundant_interp.tf b/configs/testdata/warning-files/redundant_interp.tf new file mode 100644 index 000000000000..07db23b8c6ae --- /dev/null +++ b/configs/testdata/warning-files/redundant_interp.tf @@ -0,0 +1,36 @@ +# It's redundant to write an expression that is just a single template +# interpolation with another expression inside, like "${foo}", but it +# was required before Terraform v0.12 and so there are lots of existing +# examples out there using that style. +# +# We are generating warnings for that situation in order to guide those +# who are following old examples toward the new idiom. + +variable "triggers" { + type = "map" # WARNING: Quoted type constraints are deprecated +} + +provider "null" { + foo = "${var.triggers["foo"]}" # WARNING: Interpolation-only expressions are deprecated +} + +resource "null_resource" "a" { + triggers = "${var.triggers}" # WARNING: Interpolation-only expressions are deprecated + + connection { + type = "ssh" + host = "${var.triggers["host"]}" # WARNING: Interpolation-only expressions are deprecated + } + + provisioner "local-exec" { + single = "${var.triggers["greeting"]}" # WARNING: Interpolation-only expressions are deprecated + + # No warning for this one, because there's more than just one interpolation + # in the template. + template = " ${var.triggers["greeting"]} " + + # No warning for this one, because it's embedded inside a more complex + # expression and our check is only for direct assignment to attributes. + wrapped = ["${var.triggers["greeting"]}"] + } +} diff --git a/configs/testdata/warning-files/variable_type_quoted.tf b/configs/testdata/warning-files/variable_type_quoted.tf new file mode 100644 index 000000000000..9201ba62eb72 --- /dev/null +++ b/configs/testdata/warning-files/variable_type_quoted.tf @@ -0,0 +1,11 @@ +variable "bad_string" { + type = "string" # WARNING: Quoted type constraints are deprecated +} + +variable "bad_map" { + type = "map" # WARNING: Quoted type constraints are deprecated +} + +variable "bad_list" { + type = "list" # WARNING: Quoted type constraints are deprecated +}