Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow parsing of required_providers containing ref #59

Merged
merged 1 commit into from
Feb 9, 2021
Merged

allow parsing of required_providers containing ref #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 55 additions & 31 deletions tfconfig/provider_ref.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package tfconfig

import (
"fmt"

"github.com/hashicorp/hcl/v2"
"github.com/hashicorp/hcl/v2/gohcl"
"github.com/zclconf/go-cty/cty/gocty"
"github.com/zclconf/go-cty/cty"
)

// ProviderRef is a reference to a provider configuration within a module.
Expand All @@ -23,13 +25,8 @@ func decodeRequiredProvidersBlock(block *hcl.Block) (map[string]*ProviderRequire
attrs, diags := block.Body.JustAttributes()
reqs := make(map[string]*ProviderRequirement)
for name, attr := range attrs {
expr, err := attr.Expr.Value(nil)
if err != nil {
diags = append(diags, err...)
}

switch {
case expr.Type().IsPrimitiveType():
// Look for a legacy version in the attribute first
if expr, err := attr.Expr.Value(nil); err == nil && expr.Type().IsPrimitiveType() {
var version string
valDiags := gohcl.DecodeExpression(attr.Expr, nil, &version)
diags = append(diags, valDiags...)
Expand All @@ -38,46 +35,73 @@ func decodeRequiredProvidersBlock(block *hcl.Block) (map[string]*ProviderRequire
VersionConstraints: []string{version},
}
}
continue
}

kvs, mapDiags := hcl.ExprMap(attr.Expr)
if mapDiags.HasErrors() {
diags = append(diags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Invalid required_providers object",
Detail: "Required providers entries must be strings or objects.",
Subject: attr.Expr.Range().Ptr(),
})
continue
}

var pr ProviderRequirement

for _, kv := range kvs {
key, keyDiags := kv.Key.Value(nil)
if keyDiags.HasErrors() {
diags = append(diags, keyDiags...)
continue
}

case expr.Type().IsObjectType():
var pr ProviderRequirement
if expr.Type().HasAttribute("version") {
var version string
err := gocty.FromCtyValue(expr.GetAttr("version"), &version)
if err == nil {
pr.VersionConstraints = append(pr.VersionConstraints, version)
} else {
if key.Type() != cty.String {
diags = append(diags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Invalid Attribute",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would drop this error, or at least make it a warning instead (though I'm not sure if we have many warnings, so that might not be a good idea either).

We generally ignore unexpected fields as part of the (vague, hand-wavey) future-proofing. terraform-config-inspect is intended to be a very permissive tool that only knows what it knows directly and ignores anything extra.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put this in since it was in the category of "should never happen", and would have generated a parsing error before as well. Come to think of it though, I'm not sure if we could ever get here without a string at all 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at hcl.ExprMap, the returned values are entirely up to the expression implementation, however they should only be returning a value at all if there this is a valid map, which requires string keys/attributes. We can probably just drop this error altogether.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would generally lean also towards permissiveness in the behaviour, esp. because we also use it in the language server, where we expect to be parsing invalid configs pretty often.

That said as long as we continue parsing the rest of the config (which seems to be the case here) and not "stop & return nil and diags", I'm ok with that as diags can always be dealt with.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note, the old code would have returned an error if this is even possible at some level. The string check is basically a failsafe in case a faulty ExprMap implementation returns something incorrect for the key value, but maybe that's overkill here and we should just let it panic in that case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Er .... looking at this again I just misread the error case :(

Detail: fmt.Sprintf("Invalid attribute value for provider requirement: %#v", key),
Subject: kv.Key.Range().Ptr(),
})
continue
}

switch key.AsString() {
case "version":
version, valDiags := kv.Value.Value(nil)
if valDiags.HasErrors() || !version.Type().Equals(cty.String) {
diags = append(diags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Unsuitable value type",
Detail: "Unsuitable value: string required",
Subject: attr.Expr.Range().Ptr(),
})
continue
}
}
if expr.Type().HasAttribute("source") {
var source string
err := gocty.FromCtyValue(expr.GetAttr("source"), &source)
if err == nil {
pr.Source = source
} else {
if !version.IsNull() {
pr.VersionConstraints = append(pr.VersionConstraints, version.AsString())
}

case "source":
source, err := kv.Value.Value(nil)
if err != nil || !source.Type().Equals(cty.String) {
diags = append(diags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Unsuitable value type",
Detail: "Unsuitable value: string required",
Subject: attr.Expr.Range().Ptr(),
})
continue
}

if !source.IsNull() {
pr.Source = source.AsString()
}
}
reqs[name] = &pr

default:
diags = append(diags, &hcl.Diagnostic{
Severity: hcl.DiagError,
Summary: "Unsuitable value type",
Detail: "Unsuitable value: string required",
Subject: attr.Expr.Range().Ptr(),
})
reqs[name] = &pr
}
}

Expand Down
1 change: 1 addition & 0 deletions tfconfig/testdata/provider-aliases/provider-aliases.out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"required_providers": {
"bar": {},
"baz": {},
"bleep": {},
"empty": {},
"foo": {}
},
Expand Down
1 change: 1 addition & 0 deletions tfconfig/testdata/provider-aliases/provider-aliases.out.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
Provider Requirements:
* **bar:** (any version)
* **baz:** (any version)
* **bleep:** (any version)
* **empty:** (any version)
* **foo:** (any version)

8 changes: 8 additions & 0 deletions tfconfig/testdata/provider-aliases/provider-aliases.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
terraform {
required_providers {
bleep = {
configuration_aliases = [ bleep.bloop ]
}
}
}

provider "foo" {
alias = "blue"
}
Expand Down
4 changes: 2 additions & 2 deletions tfconfig/testdata/type-errors/type-errors.out.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,8 +75,8 @@
},
{
"severity": "error",
"summary": "Unsuitable value type",
"detail": "Unsuitable value: string required",
"summary": "Invalid required_providers object",
"detail": "Required providers entries must be strings or objects.",
"pos": {
"filename": "testdata/type-errors/type-errors.tf",
"line": 27
Expand Down
4 changes: 2 additions & 2 deletions tfconfig/testdata/type-errors/type-errors.out.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,9 @@ Provider argument requires a provider name followed by an optional alias, like "

Unsuitable value: string required

## Error: Unsuitable value type
## Error: Invalid required_providers object

(at `testdata/type-errors/type-errors.tf` line 27)

Unsuitable value: string required
Required providers entries must be strings or objects.