Add logic to build configuration for multiple Terraform providers support #7189
Conversation
lakshmimsft
changed the title
lakshmimsft
force-pushed
the
lakshmimsft/terraformproviders
branch
2 times, most recently
from
da01510 to
725f211
Compare
lakshmimsft
force-pushed
the
lakshmimsft/terraformproviders
branch
from
725f211 to
12e1e49
Compare
lakshmimsft
force-pushed
the
lakshmimsft/terraformproviders
branch
5 times, most recently
from
aa89820 to
a01464d
Compare
There was a problem hiding this comment.
PR title and description are out of date. Please update them. Also, it is good practice to write the description in a way that anyone can understand what the PR is about, think about someone looking at the commit in the future to understand what this change is, why it was etc. Same feedback for #7192
lakshmimsft
force-pushed
the
lakshmimsft/terraformproviders
branch
from
a01464d to
bfc7466
Compare
lakshmimsft
changed the title
lakshmimsft
changed the title
lakshmimsft
force-pushed
the
lakshmimsft/terraformproviders
branch
3 times, most recently
from
f3dde3e to
0a7d6a8
Compare
lakshmimsft
changed the title
| for _, configDetails := range config { | ||
| configList = append(configList, configDetails.AdditionalProperties) | ||
| } |
There was a problem hiding this comment.
Should check if AdditionalProperties is non-empty/non-nil?
| for _, configDetails := range config { | |
| configList = append(configList, configDetails.AdditionalProperties) | |
| } | |
| for _, configDetails := range config { | |
| if configDetails.AdditionalProperties != nil && len(configDetails.AdditionalProperties) > 0 { | |
| configList = append(configList, configDetails.AdditionalProperties) | |
| } | |
| } |
There was a problem hiding this comment.
@kachawla this is allowed here to accomodate 'default provider configuration' where no details are input. Tests are added for this case both in types_test.go and here: https://github.com/radius-project/radius/pull/7189/files#diff-c5c89506f1ba2fe9bd8ecf4eedc856e93714c251c295c32a15d875f5924df3bdR515-R530
a example resulting main.tf.json is here: pkg/recipes/terraform/config/testdata/providers-envrecipedefaultconfig.tf.json
I did a manual test to see provider configuration load successfully.
There was a problem hiding this comment.
Sounds good, thank you. During manual testing, in addition to checking that the config was loaded successfully, could you also confirm that terraform apply runs successfully for this scenario?
There was a problem hiding this comment.
Also, can you please expand on "default provider configuration"? I'm not sure what we mean by this. Feel free to point me to the right section in the design if it is captured in the doc already.
There was a problem hiding this comment.
yes, confirming, terraform apply runs successfully for this scenario.
There was a problem hiding this comment.
Why do we want to populate empty config though?
There was a problem hiding this comment.
There is no concept of default empty configuration afaik, let's add this nil check unless there is a terraform documentation that mentions otherwise.
There was a problem hiding this comment.
added nil check.
per documentation , https://developer.hashicorp.com/terraform/language/providers/configuration#default-provider-configurations
default configuration is a block without alias argument. and if we provide all configurations with an alias, it picks the "implied empty configuration" as the default configuration.
The null input does not seem to be documented but seems to be handled by terraform without throwing errors.
There was a problem hiding this comment.
From the doc - "Terraform assumes an empty default configuration for any provider that is not explicitly configured." - this is already handled by Terraform. By adding empty configuration here, we are overriding any non-empty config that a user might have in their module. Unless there is a strong use case for passing empty config, I wouldn't recommend doing this.
lakshmimsft
force-pushed
the
lakshmimsft/terraformproviders
branch
2 times, most recently
from
ea95250 to
3365e5c
Compare
lakshmimsft
force-pushed
the
lakshmimsft/terraformproviders
branch
3 times, most recently
from
20cdcb1 to
30a4a2e
Compare
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
pkg/recipes/terraform/config/testdata/providers-envrecipedefaultconfig.tf.json
Outdated
Show resolved
Hide resolved
| for _, configDetails := range config { | ||
| configList = append(configList, configDetails.AdditionalProperties) | ||
| } |
There was a problem hiding this comment.
There is no concept of default empty configuration afaik, let's add this nil check unless there is a terraform documentation that mentions otherwise.
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
lakshmimsft
force-pushed
the
lakshmimsft/terraformproviders
branch
from
20e5a9d to
ed988b7
Compare
lakshmimsft
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
…port (radius-project#7189) # Description Adding logic to build configuration for multiple Terraform provider support. This is constructed from a combination of environment level recipe configuration located under RecipeConfig/Terraform/Providers section and the provider configurations registered with UCP. The environment level recipe configuration for providers takes precedence over UCP provider configurations. [link to PR for design doc](radius-project/design-notes#39) The design document describes a SecretReference type and ability to fetch data from secrets and populate Provider Configuration. This will be implemented in subsequent PRs. ## Type of change - This pull request adds or changes features of Radius and has an approved issue radius-project#6539 Fixes: Part of radius-project#6539
Description
Adding logic to build configuration for multiple Terraform provider support. This is constructed from a combination of environment level recipe configuration located under RecipeConfig/Terraform/Providers section and the provider configurations registered with UCP. The environment level recipe configuration for providers takes precedence over UCP provider configurations.
link to PR for design doc
The design document describes a SecretReference type and ability to fetch data from secrets and populate Provider Configuration. This will be implemented in subsequent PRs.
Type of change
Fixes: Part of #6539