Open youngbupark opened 1 year ago
So does this go in the generated wrapper module? I'm trying to understand from bug whether this is about:
If it's an implementation detail of Radius are there any downsides to option 1? Channeling my inner language designer here: I would strongly prefer using the type system over using sentinel values as a workaround.
So does this go in the generated wrapper module? I'm trying to understand from bug whether this is about:
- Generated code that is used as plumbing to invoke the user's module (an implementation detail of Radius) VS
- Guidance for how users write recipes in terraform
Does wrapper module
mean main.tf.json
template Radius creates to use recipe module? Still confusing the terminology :) Then yes your points are right.
If it's an implementation detail of Radius are there any downsides to option 1? Channeling my inner language designer here: I would strongly prefer using the type system over using sentinel values as a workaround.
Actually, I tried option 1 and 2. Apart from the downsides above in option 2, populating empty properties give user the wrong impression like a bug. so I prefer option 1 too.
In PR https://github.com/project-radius/radius/pull/5999, we add optional keyword to azure and aws objects to address the problem.
I think there is room for confusion in how the problem and solutions are phrased. Adding optional field in this test file https://github.com/project-radius/radius/blob/a2d1ebe786aee71e2bf4215e0e3daf5671e6ae1d/pkg/recipes/terraform/testdata/.terraform/modules/test-module-recipe-context/variables.tf will not fix real user scenario. In real scenario this file is controlled by the user in the module/module wrapper they are using as a Recipe, so to support option 1, we are expecting that we can require users to always mark certain fields as optional which is not realistic.
As I mentioned on the PR, Terraform does not throw an error if certain fields are not defined at all but are passed - so if aws
and azure
were not included in the variables.tf but were passed by Radius, it will not fail. So users can very well just not include these fields in the their variable definition.
If it's an implementation detail of Radius
@rynowak This is not controlled by Radius.
In PR radius-project/radius#5999, we add optional keyword to azure and aws objects to address the problem.
I think there is room for confusion in how the problem and solutions are phrased. Adding optional field in this test file https://github.com/project-radius/radius/blob/a2d1ebe786aee71e2bf4215e0e3daf5671e6ae1d/pkg/recipes/terraform/testdata/.terraform/modules/test-module-recipe-context/variables.tf will not fix real user scenario. In real scenario this file is controlled by the user in the module/module wrapper they are using as a Recipe, so to support option 1, we are expecting that we can require users to always mark certain fields as optional which is not realistic.
Just to be clear - I am pretty new to TF work so my question could be irrelevant or already discussed.
I am still learning our TF design/implementation, so I might have the missing discussion or conversation. But I still don't understand why optional
is not realistic because optional
is officially supported keyword in TF. I see that real users use this keyword in their TF templates in GitHub and stackoverflow communities...
As I mentioned on the PR, Terraform does not throw an error if certain fields are not defined at all but are passed - so if
aws
andazure
were not included in the variables.tf but were passed by Radius, it will not fail. So users can very well just not include these fields in the their variable definition.If it's an implementation detail of Radius
@rynowak This is not controlled by Radius.
From what I understand from Ryan's comment, there are two personas:
context
variable in their module -- User will follow our docs or copy context variable sample definition to their own module. wrapper module
: Radius can populate recipe context parameters based on recipe context schema spec.We do not have the full-control for the first one even though they will follow how to write recipe module
and our sample, but we have our control for the second one. I thought that the second one was controllable in Ryan's comment.
Working on my first PR for TF work, my initial thought was that we would provide the sharable TF file which included recipe context schema, so recipe authors would just copy the sharable TF file to their TF recipe instead of authoring context variable by themselves.
@kachawla Based on your comment, is your expectation that we will guide TF recipe author that user would define only what they use in their module?
In bicep template, we don't define the entire recipe schema, which is free-form style object. But I thought I must define the exact schema for TF in the beginning. I learn today that TF also has any
type, which is free-form style object like bicep template. The only downside is that this is loosely typed variable. I think this option is also good to be consistent with bicep behavior. @rynowak @kachawla WDYT?
variable "context" {
description = "This variable contains Radius recipe context."
type = any
}
@youngbupark , @kachawla - did we have the information needed here on this issue , any action item to be taken on this issue?
:+1: We've reviewed this issue and have agreed to add it to our backlog. Please subscribe to this issue for notifications, we'll provide updates when we pick it up.
We also welcome community contributions! If you would like to pick this item up sooner and submit a pull request, please visit our contribution guidelines and assign this to yourself by commenting "/assign" on this issue.
For more information on our triage process please visit our triage overview
triage: need to have more discussion on this and decide on the priority.
But I still don't understand why optional is not realistic because optional is officially supported keyword in TF.
The reason why I mentioned it not being realistic is, because we don't control what users define in their variables.tf file, we can give them guidance to mark azure/aws attributes as optional since they will not be passed if not configured, but we can't enforce this since Radius doesn't have control over it.
TF also has any type, which is free-form style object like bicep template. The only downside is that this is loosely typed variable. I think this option is also good to be consistent with bicep behavior.
This is what we have documented in the recipe authoring page https://docs.radapp.io/guides/recipes/howto-author-recipes/
Problem
In PR radius-project/radius#5999, we support recipe context in Terraform module config. User's recipe TF module should contain the following
context
variable. We found that TF threw the error if Radius didn't provide one of the attributes in module json config.We found
attributes \"aws\" and \"azure\"\nare required.
error in the functional test because Radius didn't setaws
andazure
in module config. Terraform providesoptional
keyword for such optional attributes. In PR radius-project/radius#5999, we add optional keyword toazure
andaws
objects to address the problem.Functional test log - https://github.com/project-radius/radius/actions/runs/5774669455/job/15651905021
Possible solutions
Option 1
In PR radius-project/radius#5999 , I added
optional
keyword to makeazure
andaws
optional. So terraform will ignore the properties.Option 2
We don't add any optional keyword to attributes but instead, Radius can pass empty properties like below.
However, this will still cause the same problem when we introduce new attributes in recipe context.
Let's say there are User 1 and User 2.
User 1 uses Radius v0.30.0 and context v2 schema which includes new
gcp
attribute and publish it to the module repository. User 2 uses Radius v0.20.0 and context v1 schema which doesn't containgcp
attribute and try to use User 1's module. And then User 2 will face the same problem because Radius v0.20.0 would not populategcp
.Other questions
AB#8905
AB#9511
AB#10014