-
Notifications
You must be signed in to change notification settings - Fork 2.7k
feat: add support for .tf.json #36341
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need all those new fixtures. please reduce to minimum to satisfy coverage
I also think think you shouldn't convert to tf and parse again. you should directly transform to the parsed hcl structure. isn't the hcl lib from terraform supporting those json files out of the box? |
you can check if there's an additional library to |
ok, in this case you should directly convert the Json to the parsed hcl format and don't generate the hcl source code. |
From the .tf.json spec you don't really know when something is an array vs an object. in the kubernetes resources there is the metadata block for example. This is seen as an array, but written as an object. There are many of these little nuances that make it harder to go directly from JSON to that parsed hcl format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you reduce the number of deps extracted in the tests? I can see many deps being extracted and most are similar. Best will be to only keeps deps which are unique somewhat.
If something similar can be done with the fixtures too, please consider doing it. We prefer adding fixtures only when its necessary.
Thanks!
@@ -0,0 +1,129 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reduce fixtures to minimum size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I initially resolved this by removing some coverage. But I do want to ask, why do we want to reduce the coverage which would make sure the behavior between the .tf
and the .tf.json
remains the same?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer to inline small fixtures wherever possible as it's better for performance. But it should not come at cost of coverage
I don't agree. we know we only support specific types with specific properties, so we can directly use zod schema to parse and transform the known supported objects and discard all other things. |
I changed it to use the zod schema. Tests pass so I think it does work correctly now. Might need to expand the test cases to cover all the supported terraform resources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cdktf/hcl2json
directly parses JSON files while converting.
From the .tf.json spec you don't really know when something is an array vs an object. in the kubernetes resources there is the metadata block for example. This is seen as an array, but written as an object. There are many of these little nuances that make it harder to go directly from JSON to that parsed hcl format
This is how the spec describes it in their syntax docs.
When a JSON object property is named after a nested block type, the value of this property represents one or more blocks of that type. The value of the property must be either a JSON object or a JSON array.
To implement this it should be also possible to do a simply normalization from objects to array starting from the second or third level of the JSON objects
const GenericResourceInstance = z.record(tfLiteral); | ||
const GenericResourceSchema = z.record(oneOrMany(GenericResourceInstance)); | ||
|
||
const KubernetesInstance = z.any().transform((orig) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this any
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, an oversight. Removed for a typed value
'selector', | ||
]); | ||
|
||
function normalise(node: any): any { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function normalise(node: any): any { | |
function normalise(node: unknown): unknown { |
Do not use any
.
If we are going this route we should do this for every block and not only the Kubernetes ones.
When a JSON object property is named after a nested block type, the value of this property represents one or more blocks of that type. The value of the property must be either a JSON object or a JSON array.
https://developer.hashicorp.com/terraform/language/syntax/json#nested-block-mapping
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going this route we should do this for every block and not only the Kubernetes ones.
Agreed, however renovate currently only supports the docker, helm and kubernetes resources for Terraform.
I have read it and meant this in context of this comment and it is feasible to me to adapt the extractor logic to expect an array here, if it simplifies the code. |
Confliced |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, we should add .tf.json
to the managerFilePatterns
@@ -0,0 +1,129 @@ | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer to inline small fixtures wherever possible as it's better for performance. But it should not come at cost of coverage
bf86ec7
to
dbb1419
Compare
ba95785
to
fc2f3bb
Compare
added the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good but still too many fixtures which will slow down tests too much
Is this latest change to inline fixtures better? |
Changes
Added support for the terraform json file format. Expanded the parseHCL function to determine if a given file is a .tf.json or regular .tf file and do a JSON parse into HCL before passing that output into the HCL parser, which format is further supported by the code base
Context
Documentation (please check one with an [x])
How I've tested my work (please select one)
I have verified these changes via: