stacks: Don't require built-in providers to be listed under required providers. #37234
Conversation
There was a problem hiding this comment.
nit: this file is in the package builtin/providers so I don't think we need to repeat that in the file name - just providers.go or maybe factories.go or something?
There was a problem hiding this comment.
Good point - I'll go with factories
6aa4881
| provider "github.com/hashicorp/terraform/internal/providers" | ||
| ) | ||
|
|
||
| func BuiltInProviders() map[string]provider.Factory { |
There was a problem hiding this comment.
this could be a static map wrapped in a var () block I think?
There was a problem hiding this comment.
Wouldn't that make it globally mutable? If I had to choose between immutability that the factory function offers and the global var - I'd rather choose immutability.
Let me know if I'm missing something here.
There was a problem hiding this comment.
ha, yeah that's fair. I think we do have the pattern of global vars elsewhere in Terraform, and we save some level of computation by not creating the map every time something just wants to list the built in providers.
Either way, I don't feel that strongly about this - so happy to just do what you think is best 👍
| // occurring later in the process. | ||
| if _, ok := builtinProviders.BuiltInProviders()[localName]; ok { | ||
| return addrs.Provider{}, true | ||
| } | ||
| return addrs.Provider{}, false | ||
| } | ||
| obj, ok := pr.Requirements[localName] |
There was a problem hiding this comment.
I'm surprised you didn't need to change here as well - what if the provider requirements block was written but it doesn't have the builtin provider present? Perhaps adding another test where there is a different provider defined in the required providers but not the builtin, but the builtin is then referred to would expose this?
There could also be another explanation as to why this isn't required!
There was a problem hiding this comment.
You're surprised for valid reasons. I made an oversight. This should be fixed here 45136b4
Thanks for catching on this!
| // built-in provider names. In that case, customers will see an error | ||
| // occurring later in the process. | ||
| if _, ok := builtinProviders.BuiltInProviders()[localName]; ok { | ||
| return addrs.Provider{}, true |
There was a problem hiding this comment.
I think there's like an addrs.BuiltinProvider(localName) function you can call to return a proper address here?
There was a problem hiding this comment.
Thanks - there is addrs.NewDefaultProvider - I believe this function does what's necessary here.
| } | ||
| } | ||
|
|
||
| for providerName, _ := range builtinProviders.BuiltInProviders() { |
There was a problem hiding this comment.
nit: don't need the second assignment here
| ret = includeBuiltInProviders(ret) | ||
| return ret, diags |
There was a problem hiding this comment.
| ret = includeBuiltInProviders(ret) | |
| return ret, diags | |
| return includeBuiltInProviders(ret), diags |
style nit! optional
| ret = includeBuiltInProviders(ret) | ||
|
|
||
| return ret, diags |
There was a problem hiding this comment.
| ret = includeBuiltInProviders(ret) | |
| return ret, diags | |
| return includeBuiltInProviders(ret), diags |
style nit, optional
liamcervante
added
the
no-changelog-needed
matejrisek
deleted the
feature/stacks/ignore-required-providers-if-builtin
branch
stacks: Do NOT require built-in providers to have to be listed under required providers.
Target Release
1.13.x
Rollback Plan
Changes to Security Controls
N/A
CHANGELOG entry