allow parsing of required_providers containing ref #59
Conversation
The syntax for configuration_aliases contains bare references to match their use in other parts of the configuration. These however cannot be decoded directly without an EvalContext, as they represent variables. Refactor decodeRequiredProvidersBlock to use the lower level ExprMap function.
| if key.Type() != cty.String { | ||
| diags = append(diags, &hcl.Diagnostic{ | ||
| Severity: hcl.DiagError, | ||
| Summary: "Invalid Attribute", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Er .... looking at this again I just misread the error case :(
There was a problem hiding this comment.
Approving this to unblock and also because I understand this is rather a fix, than a new feature from tfconfig perspective.
That said I think we'll need to come up with a way of actually exposing these new aliases somehow. At least we'll need it in LS, see #54
|
Thanks @radeksimko Yeah, like I mentioned, I specifically avoided adding these in, because we are currently only exposing configurations. Providers in modules don't always have an associated configuration, aliased or not, and It wasn't clear if we want the information presented to represent only literal configuration blocks. I can follow up if we want all provider names (which are in essence conceptually referred to as configurations) to be present in the output. |
The syntax for
configuration_aliasescontains bare references to matchtheir use in other parts of the configuration. These however cannot be
decoded directly without an EvalContext, as they represent variables.
Refactor decodeRequiredProvidersBlock to use the lower level
hcl.ExprMapfunction.While
configuration_aliasesdoes define an alternate configuration name for a provider, this PR does not add the alias names toprovider_configsbecause that field is specifically documented as representing aproviderblock. This is consistent with the handling of un-aliased providers too, since they may also be referenced and have requirements without an explicit configuration block.