From 36fb5b52e79c66f6476c0e1ee93da9dea438f47a Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 15 Feb 2018 10:17:36 -0800 Subject: [PATCH] configs: quoted keywords/references are warnings, not errors In our new loader we are changing certain values in configuration to be naked keywords or references rather than quoted strings as before. Since many of these have been shown in books, tutorials, and our own documentation we will make the old forms generate deprecation warnings rather than errors so that newcomers starting from older documentation can be eased into the new syntax, rather than getting blocked. This will also avoid creating a hard compatibility wall for reusable modules that are already published, allowing them to still be used in spite of these warnings and then fixed when the maintainer is able. --- configs/compat_shim.go | 81 +++++++++++++++++++ configs/depends_on.go | 18 +---- configs/named_values.go | 30 +++---- configs/parser_config_dir_test.go | 4 +- configs/parser_config_test.go | 59 ++++++++++---- configs/provisioner.go | 52 +++++------- configs/resource.go | 13 ++- .../valid-files/resources-dependson-quoted.tf | 8 ++ .../resources-ignorechanges-quoted.tf | 7 ++ .../resources-provisioner-onfailure-quoted.tf | 6 ++ .../resources-provisioner-when-quoted.tf | 6 ++ .../variable-type-quoted.tf | 0 12 files changed, 195 insertions(+), 89 deletions(-) create mode 100644 configs/compat_shim.go create mode 100644 configs/test-fixtures/valid-files/resources-dependson-quoted.tf create mode 100644 configs/test-fixtures/valid-files/resources-ignorechanges-quoted.tf create mode 100644 configs/test-fixtures/valid-files/resources-provisioner-onfailure-quoted.tf create mode 100644 configs/test-fixtures/valid-files/resources-provisioner-when-quoted.tf rename configs/test-fixtures/{invalid-files => valid-files}/variable-type-quoted.tf (100%) diff --git a/configs/compat_shim.go b/configs/compat_shim.go new file mode 100644 index 000000000..69d8b0d0f --- /dev/null +++ b/configs/compat_shim.go @@ -0,0 +1,81 @@ +package configs + +import ( + "github.com/hashicorp/hcl2/hcl" + "github.com/hashicorp/hcl2/hcl/hclsyntax" +) + +// ------------------------------------------------------------------------- +// Functions in this file are compatibility shims intended to ease conversion +// from the old configuration loader. Any use of these functions that makes +// a change should generate a deprecation warning explaining to the user how +// to update their code for new patterns. +// +// Shims are particularly important for any patterns that have been widely +// documented in books, tutorials, etc. Users will still be starting from +// these examples and we want to help them adopt the latest patterns rather +// than leave them stranded. +// ------------------------------------------------------------------------- + +// shimTraversalInString takes any arbitrary expression and checks if it is +// a quoted string in the native syntax. If it _is_, then it is parsed as a +// traversal and re-wrapped into a synthetic traversal expression and a +// warning is generated. Otherwise, the given expression is just returned +// verbatim. +// +// This function has no effect on expressions from the JSON syntax, since +// traversals in strings are the required pattern in that syntax. +// +// If wantKeyword is set, the generated warning diagnostic will talk about +// keywords rather than references. The behavior is otherwise unchanged, and +// the caller remains responsible for checking that the result is indeed +// a keyword, e.g. using hcl.ExprAsKeyword. +func shimTraversalInString(expr hcl.Expression, wantKeyword bool) (hcl.Expression, hcl.Diagnostics) { + if !exprIsNativeQuotedString(expr) { + return expr, nil + } + + strVal, diags := expr.Value(nil) + if diags.HasErrors() || strVal.IsNull() || !strVal.IsKnown() { + // Since we're not even able to attempt a shim here, we'll discard + // the diagnostics we saw so far and let the caller's own error + // handling take care of reporting the invalid expression. + return expr, nil + } + + // The position handling here isn't _quite_ right because it won't + // take into account any escape sequences in the literal string, but + // it should be close enough for any error reporting to make sense. + srcRange := expr.Range() + startPos := srcRange.Start // copy + startPos.Column++ // skip initial quote + startPos.Byte++ // skip initial quote + + traversal, tDiags := hclsyntax.ParseTraversalAbs( + []byte(strVal.AsString()), + srcRange.Filename, + startPos, + ) + diags = append(diags, tDiags...) + + 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, + }) + } + + return &hclsyntax.ScopeTraversalExpr{ + Traversal: traversal, + SrcRange: srcRange, + }, diags +} diff --git a/configs/depends_on.go b/configs/depends_on.go index f20c58167..b1984768f 100644 --- a/configs/depends_on.go +++ b/configs/depends_on.go @@ -1,8 +1,6 @@ package configs import ( - "fmt" - "github.com/hashicorp/hcl2/hcl" ) @@ -11,20 +9,8 @@ func decodeDependsOn(attr *hcl.Attribute) ([]hcl.Traversal, hcl.Diagnostics) { exprs, diags := hcl.ExprList(attr.Expr) for _, expr := range exprs { - // A dependency reference was given as a string literal in the legacy - // configuration language and there are lots of examples out there - // showing that usage, so we'll sniff for that situation here and - // produce a specialized error message for it to help users find - // the new correct form. - if exprIsNativeQuotedString(expr) { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid explicit dependency reference", - Detail: fmt.Sprintf("%s elements must not be given in quotes.", attr.Name), - Subject: attr.Expr.Range().Ptr(), - }) - continue - } + expr, shimDiags := shimTraversalInString(expr, false) + diags = append(diags, shimDiags...) traversal, travDiags := hcl.AbsTraversalForExpr(expr) diags = append(diags, travDiags...) diff --git a/configs/named_values.go b/configs/named_values.go index 42ae4750d..d1d93a57d 100644 --- a/configs/named_values.go +++ b/configs/named_values.go @@ -68,7 +68,10 @@ func decodeVariableBlock(block *hcl.Block) (*Variable, hcl.Diagnostics) { } if attr, exists := content.Attributes["type"]; exists { - switch hcl.ExprAsKeyword(attr.Expr) { + expr, shimDiags := shimTraversalInString(attr.Expr, true) + diags = append(diags, shimDiags...) + + switch hcl.ExprAsKeyword(expr) { case "string": v.TypeHint = TypeHintString case "list": @@ -76,25 +79,12 @@ func decodeVariableBlock(block *hcl.Block) (*Variable, hcl.Diagnostics) { case "map": v.TypeHint = TypeHintMap default: - // In our legacy configuration format these keywords would've been - // provided as quoted strings, so we'll generate a special error - // message for that to help those who find outdated examples and - // would otherwise be confused. - if exprIsNativeQuotedString(attr.Expr) { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid variable type hint", - Detail: "The type hint keyword must not be given in quotes.", - Subject: attr.Expr.Range().Ptr(), - }) - } else { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid variable type hint", - Detail: "The type argument requires one of the following keywords: string, list, or map.", - Subject: attr.Expr.Range().Ptr(), - }) - } + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid variable type hint", + Detail: "The type argument requires one of the following keywords: string, list, or map.", + Subject: expr.Range().Ptr(), + }) } } diff --git a/configs/parser_config_dir_test.go b/configs/parser_config_dir_test.go index 703fc122d..b35f30127 100644 --- a/configs/parser_config_dir_test.go +++ b/configs/parser_config_dir_test.go @@ -60,8 +60,8 @@ func TestParserLoadConfigDirSuccess(t *testing.T) { }) _, diags := parser.LoadConfigDir("mod") - if len(diags) != 0 { - t.Errorf("unexpected diagnostics") + if diags.HasErrors() { + t.Errorf("unexpected error diagnostics") for _, diag := range diags { t.Logf("- %s", diag) } diff --git a/configs/parser_config_test.go b/configs/parser_config_test.go index 2f118933c..ae7c8856d 100644 --- a/configs/parser_config_test.go +++ b/configs/parser_config_test.go @@ -34,8 +34,8 @@ func TestParserLoadConfigFileSuccess(t *testing.T) { }) _, diags := parser.LoadConfigFile(name) - if len(diags) != 0 { - t.Errorf("unexpected diagnostics") + if diags.HasErrors() { + t.Errorf("unexpected error diagnostics") for _, diag := range diags { t.Logf("- %s", diag) } @@ -85,38 +85,65 @@ func TestParserLoadConfigFileFailure(t *testing.T) { // file produces the expected diagnostic summary. func TestParserLoadConfigFileFailureMessages(t *testing.T) { tests := []struct { - Filename string - WantError string + Filename string + WantSeverity hcl.DiagnosticSeverity + WantDiag string }{ { - "data-resource-lifecycle.tf", + "invalid-files/data-resource-lifecycle.tf", + hcl.DiagError, "Unsupported lifecycle block", }, { - "variable-type-unknown.tf", + "invalid-files/variable-type-unknown.tf", + hcl.DiagError, "Invalid variable type hint", }, { - "variable-type-quoted.tf", - "Invalid variable type hint", + "valid-files/variable-type-quoted.tf", + hcl.DiagWarning, + "Quoted keywords are deprecated", }, { - "unexpected-attr.tf", + "invalid-files/unexpected-attr.tf", + hcl.DiagError, "Unsupported attribute", }, { - "unexpected-block.tf", + "invalid-files/unexpected-block.tf", + hcl.DiagError, "Unsupported block type", }, { - "resource-lifecycle-badbool.tf", + "invalid-files/resource-lifecycle-badbool.tf", + hcl.DiagError, "Unsuitable value type", }, + { + "valid-files/resources-dependson-quoted.tf", + hcl.DiagWarning, + "Quoted references are deprecated", + }, + { + "valid-files/resources-ignorechanges-quoted.tf", + hcl.DiagWarning, + "Quoted references are deprecated", + }, + { + "valid-files/resources-provisioner-when-quoted.tf", + hcl.DiagWarning, + "Quoted keywords are deprecated", + }, + { + "valid-files/resources-provisioner-onfailure-quoted.tf", + hcl.DiagWarning, + "Quoted keywords are deprecated", + }, } for _, test := range tests { t.Run(test.Filename, func(t *testing.T) { - src, err := ioutil.ReadFile(filepath.Join("test-fixtures/invalid-files", test.Filename)) + src, err := ioutil.ReadFile(filepath.Join("test-fixtures", test.Filename)) if err != nil { t.Fatal(err) } @@ -133,11 +160,11 @@ func TestParserLoadConfigFileFailureMessages(t *testing.T) { } return } - if diags[0].Severity != hcl.DiagError { - t.Errorf("Wrong diagnostic severity %s; want %s", diags[0].Severity, hcl.DiagError) + if diags[0].Severity != test.WantSeverity { + t.Errorf("Wrong diagnostic severity %#v; want %#v", diags[0].Severity, test.WantSeverity) } - if diags[0].Summary != test.WantError { - t.Errorf("Wrong diagnostic summary\ngot: %s\nwant: %s", diags[0].Summary, test.WantError) + if diags[0].Summary != test.WantDiag { + t.Errorf("Wrong diagnostic summary\ngot: %s\nwant: %s", diags[0].Summary, test.WantDiag) } }) } diff --git a/configs/provisioner.go b/configs/provisioner.go index 03c14e16c..843f4c0f9 100644 --- a/configs/provisioner.go +++ b/configs/provisioner.go @@ -33,52 +33,40 @@ func decodeProvisionerBlock(block *hcl.Block) (*Provisioner, hcl.Diagnostics) { pv.Config = config if attr, exists := content.Attributes["when"]; exists { - switch hcl.ExprAsKeyword(attr.Expr) { + expr, shimDiags := shimTraversalInString(attr.Expr, true) + diags = append(diags, shimDiags...) + + switch hcl.ExprAsKeyword(expr) { case "create": pv.When = ProvisionerWhenCreate case "destroy": pv.When = ProvisionerWhenDestroy default: - if exprIsNativeQuotedString(attr.Expr) { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid \"when\" keyword", - Detail: "The \"when\" argument keyword must not be given in quotes.", - Subject: attr.Expr.Range().Ptr(), - }) - } else { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid \"when\" keyword", - Detail: "The \"when\" argument requires one of the following keywords: create or destroy.", - Subject: attr.Expr.Range().Ptr(), - }) - } + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid \"when\" keyword", + Detail: "The \"when\" argument requires one of the following keywords: create or destroy.", + Subject: expr.Range().Ptr(), + }) } } if attr, exists := content.Attributes["on_failure"]; exists { - switch hcl.ExprAsKeyword(attr.Expr) { + expr, shimDiags := shimTraversalInString(attr.Expr, true) + diags = append(diags, shimDiags...) + + switch hcl.ExprAsKeyword(expr) { case "continue": pv.OnFailure = ProvisionerOnFailureContinue case "fail": pv.OnFailure = ProvisionerOnFailureFail default: - if exprIsNativeQuotedString(attr.Expr) { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid \"on_failure\" keyword", - Detail: "The \"on_failure\" argument keyword must not be given in quotes.", - Subject: attr.Expr.Range().Ptr(), - }) - } else { - diags = append(diags, &hcl.Diagnostic{ - Severity: hcl.DiagError, - Summary: "Invalid \"on_failure\" keyword", - Detail: "The \"on_failure\" argument requires one of the following keywords: continue or fail.", - Subject: attr.Expr.Range().Ptr(), - }) - } + diags = append(diags, &hcl.Diagnostic{ + Severity: hcl.DiagError, + Summary: "Invalid \"on_failure\" keyword", + Detail: "The \"on_failure\" argument requires one of the following keywords: continue or fail.", + Subject: attr.Expr.Range().Ptr(), + }) } } diff --git a/configs/resource.go b/configs/resource.go index 3892c86b9..1fa16553f 100644 --- a/configs/resource.go +++ b/configs/resource.go @@ -122,6 +122,9 @@ func decodeResourceBlock(block *hcl.Block) (*ManagedResource, hcl.Diagnostics) { diags = append(diags, listDiags...) for _, expr := range exprs { + expr, shimDiags := shimTraversalInString(expr, false) + diags = append(diags, shimDiags...) + traversal, travDiags := hcl.RelTraversalForExpr(expr) diags = append(diags, travDiags...) if len(traversal) != 0 { @@ -257,7 +260,11 @@ type ProviderConfigRef struct { func decodeProviderConfigRef(attr *hcl.Attribute) (*ProviderConfigRef, hcl.Diagnostics) { var diags hcl.Diagnostics - traversal, travDiags := hcl.AbsTraversalForExpr(attr.Expr) + + expr, shimDiags := shimTraversalInString(attr.Expr, false) + diags = append(diags, shimDiags...) + + traversal, travDiags := hcl.AbsTraversalForExpr(expr) // AbsTraversalForExpr produces only generic errors, so we'll discard // the errors given and produce our own with extra context. If we didn't @@ -277,7 +284,7 @@ func decodeProviderConfigRef(attr *hcl.Attribute) (*ProviderConfigRef, hcl.Diagn Severity: hcl.DiagError, Summary: "Invalid provider configuration reference", Detail: "A provider configuration reference must not be given in quotes.", - Subject: attr.Expr.Range().Ptr(), + Subject: expr.Range().Ptr(), }) return nil, diags } @@ -286,7 +293,7 @@ func decodeProviderConfigRef(attr *hcl.Attribute) (*ProviderConfigRef, hcl.Diagn Severity: hcl.DiagError, Summary: "Invalid provider configuration reference", Detail: fmt.Sprintf("The %s argument requires a provider type name, optionally followed by a period and then a configuration alias.", attr.Name), - Subject: attr.Expr.Range().Ptr(), + Subject: expr.Range().Ptr(), }) return nil, diags } diff --git a/configs/test-fixtures/valid-files/resources-dependson-quoted.tf b/configs/test-fixtures/valid-files/resources-dependson-quoted.tf new file mode 100644 index 000000000..3bf188f19 --- /dev/null +++ b/configs/test-fixtures/valid-files/resources-dependson-quoted.tf @@ -0,0 +1,8 @@ +resource "aws_security_group" "firewall" { +} + +resource "aws_instance" "web" { + depends_on = [ + "aws_security_group.firewall", + ] +} diff --git a/configs/test-fixtures/valid-files/resources-ignorechanges-quoted.tf b/configs/test-fixtures/valid-files/resources-ignorechanges-quoted.tf new file mode 100644 index 000000000..cba5be59d --- /dev/null +++ b/configs/test-fixtures/valid-files/resources-ignorechanges-quoted.tf @@ -0,0 +1,7 @@ +resource "aws_instance" "web" { + lifecycle { + ignore_changes = [ + "ami", + ] + } +} diff --git a/configs/test-fixtures/valid-files/resources-provisioner-onfailure-quoted.tf b/configs/test-fixtures/valid-files/resources-provisioner-onfailure-quoted.tf new file mode 100644 index 000000000..dcec1eb08 --- /dev/null +++ b/configs/test-fixtures/valid-files/resources-provisioner-onfailure-quoted.tf @@ -0,0 +1,6 @@ +resource "aws_security_group" "firewall" { + provisioner "local-exec" { + command = "echo hello" + on_failure = "continue" + } +} diff --git a/configs/test-fixtures/valid-files/resources-provisioner-when-quoted.tf b/configs/test-fixtures/valid-files/resources-provisioner-when-quoted.tf new file mode 100644 index 000000000..6a66b085f --- /dev/null +++ b/configs/test-fixtures/valid-files/resources-provisioner-when-quoted.tf @@ -0,0 +1,6 @@ +resource "aws_security_group" "firewall" { + provisioner "local-exec" { + command = "echo hello" + when = "destroy" + } +} diff --git a/configs/test-fixtures/invalid-files/variable-type-quoted.tf b/configs/test-fixtures/valid-files/variable-type-quoted.tf similarity index 100% rename from configs/test-fixtures/invalid-files/variable-type-quoted.tf rename to configs/test-fixtures/valid-files/variable-type-quoted.tf