Automatically run go-cty.convert after applying defaults #560
Conversation
There was a problem hiding this comment.
Assuming the associated cty change is merged, this looks like a good change to me! Some suggestions inline.
I also think we should review and update the top-level documentation comments for Defaults and Apply, which both still imply that type conversion is a separate step, to be applied after defaults are applied. My understand is that after this change type conversion may still be necessary (depending on how deeply defaults are specified), but we should be clear that Apply now does at least some type conversion of the input values.
| @@ -317,7 +320,11 @@ func TestDefaults_Apply(t *testing.T) { | |||
| }), | |||
| cty.ObjectVal(map[string]cty.Value{ | |||
| // No default value for "c" specified, so none applied. The | |||
| // convert stage will fill in a null. | |||
| // convert stage will have filled in a null. | |||
There was a problem hiding this comment.
A little more distinct from the previous logic:
| // convert stage will have filled in a null. | |
| // apply process converts to the target type, filling in a null. |
| @@ -433,6 +442,98 @@ func TestDefaults_Apply(t *testing.T) { | |||
| }), | |||
| }), | |||
| }, | |||
| "my_test_case": { | |||
There was a problem hiding this comment.
A guess at a slightly better test case name:
| "my_test_case": { | |
| "nested set of objects with optional attributes, with maps as inputs": { |
| return v, nil | ||
| } | ||
|
|
||
| func (d *Defaults) traverseType(path cty.Path) cty.Type { |
There was a problem hiding this comment.
This function and its collaborator traverseChildType are so similar to traverse and traverseChild that I think we should consider merging them. Could we extend traverse and traverseChild to return (cty.Value, cty.Type) so that the recursion logic is shared?
|
This PR is not needed anymore, with #564 we change the documentation to state people should manually convert before setting the defaults. |
Implemented a suggestion by @alisdair and looking for thoughts/feedback.
important: Requires zclconf/go-cty#140, as one of the tests requires this change. This also means the tests in this PR are currently failing.
By running the conversion function after every nested apply of the defaults we can fix an upstream issue in Terraform where the overall conversion of the entire object fails because nested objects have incompatible types, but if we convert as we go the inner types can be converted successfully.
Relates to hashicorp/terraform#31978
This PR also means that we wouldn't have to explicitly call convert in Terraform after the defaults as we now do it automatically here.