search

hashicorp / terraform-config-inspect

A helper library for shallow inspection of Terraform configurations
Mozilla Public License 2.0
383 stars 76 forks source link

tfconfig: decode provider aliases #54

Closed radeksimko closed 3 years ago

radeksimko commented 3 years ago

The first commit is just https://github.com/hashicorp/terraform-config-inspect/pull/53 and it's part of the PR because it would otherwise be likely difficult to merge the two, given the overlap. I can rebase this PR when #53 is merged.

This patch introduces new field for Module - ProviderConfigs, which is a map where key is the provider "identification" (e.g. aws.west or just aws if no alias is present).

The use case I have in mind for this is the Terraform language server, where we need to know how to complete resource or data blocks. Aliases technically do not affect the schema, esp. because there's only ever 1 version of a provider (= 1 version of the schema), but knowing all the aliases aids the decoder in quick map lookups where key is a combination of the 1st label (resource/data source type) and provider key (if any), otherwise we'd have to range over the whole map of all resources/data sources of all providers.

I'm assuming this doesn't pose any issue from compatibility perspective since provider aliases have been around for a while and the block schema even already contains that attribute, it was just never used:

https://github.com/hashicorp/terraform-config-inspect/blob/c481b8bfa41ea9dca417c2a8a98fd21bd0399e14/tfconfig/schema.go#L53-L62

radeksimko commented 3 years ago

I think I might misunderstand your use case, because from reading the description I imagined the map to be inverted compared to how it is: from alias to provider.

I don't have a strong opinion on the exact way it's returned (happy to change it), but I think the current structure actually better reflects how we interpret provider aliases, because there's no alias without a provider and there can be the same alias associated with many providers.

In other words if we wanted it as map[string]string where key is alias and value is provider, then we are likely to run into conflicts between providers (e.g. aws.west and google.west)?

Technically we could store it as

map[string]string{
  "aws.west": "aws",
  "google.west": "google",
}

but that means we would probably have to introduce some more new types to represent the traversals properly and not like a dot-separated string. Is that what you're suggesting?

mildwonkey commented 3 years ago

What do you think about staying closer to terraform's idea of a configs.Module with a ProviderConfigs map, where the map key would be the full provider with (optional) alias ("aws.west") and the value is the provider configuration (name, alias, whatever other information you need from provider config)? If that doesn't meet your use case I'd say ignore this suggestion, but if it works it might make it easier for us to extend this later.

alisdair commented 3 years ago

My previous suggestion of inverted the map was nonsensical, as I had completely forgotten that aliases are scoped to the provider name. Sorry about that!

I like Kristin's suggestion about using an analog to the ProviderConfigs map, if that suits the use case.

radeksimko commented 3 years ago

@mildwonkey I think that should work for me too.

I just wasn't sure about introducing the dot-separated string (as opposed to a slice of strings or even hcl.Traversal), but now I can see we use that for resource addresses already, so I suppose that's something callers have to be aware of and deal with somehow.

radeksimko commented 3 years ago

Finally got around to finishing this and I believe I addressed all of your comments. Do you mind taking another look, before I merge this?