add attr.ObjectType #44
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paddycarver
approved these changes
Jun 7, 2021
paddycarver
reviewed
Jun 8, 2021
|
Thanks for highlighting the additional types added in #41 - since these will be used in a number of open PRs I think it's best to have the conversation here. New proposal is to use naming consistent with the other types in The versions proposed in #41 have the advantage of brevity but I think a method name of |
paddycarver
approved these changes
Jun 8, 2021
paddycarver
added a commit
that referenced
this pull request
Jun 8, 2021
Reflection has A Problem: We want to support using `attr.Value`s inside of structs, slices, etc. and just having the reflection package Do The Right Thing. This means the reflection package needs to be able to create them. We originally solved that in #30 by just instantiating empty values of their type, and then calling a `SetTerraformValue` method on that value. This was super annoying, because then we needed two methods for getting an `attr.Value` set to the right values: `attr.Type.ValueFromTerraform` and `attr.Value.SetTerraformValue` were basically the same thing. But whatever, it worked. Except, no it didn't. Complex types like lists and maps store their element/attribute types as properties on their structs. It's important that these be set. Only the `attr.Type` has this information, it's not passed in as part of the `tftypes.Value`. So reflect couldn't set those values, and produced broken `attr.Value`s as a result. (Their `ToTerraformValue` methods would run into trouble, because they wouldn't know what `tftypes.Type` to use for elements or attributes). To solve this problem, we decided to supply the `attr.Type` from the schema to the reflect package, wiring it through so that we could instantiate new `attr.Value`s when the opportunity presented itself. This solves our problem, because we got rid of the `attr.Value.SetTerraformValue` method and used the `attr.Type.ValueFromTerraform` directly to just instantiate a new value, which made sure it was set up correctly. But now we have a new problem: what if the `attr.Type` in the schema doesn't produce the `attr.Value` they're trying to assign to? We decided to just throw an error on that one, because there's no reasonable way around it. Depends on #44.
paddycarver
added a commit
that referenced
this pull request
Jun 8, 2021
Reflection has A Problem: We want to support using `attr.Value`s inside of structs, slices, etc. and just having the reflection package Do The Right Thing. This means the reflection package needs to be able to create them. We originally solved that in #30 by just instantiating empty values of their type, and then calling a `SetTerraformValue` method on that value. This was super annoying, because then we needed two methods for getting an `attr.Value` set to the right values: `attr.Type.ValueFromTerraform` and `attr.Value.SetTerraformValue` were basically the same thing. But whatever, it worked. Except, no it didn't. Complex types like lists and maps store their element/attribute types as properties on their structs. It's important that these be set. Only the `attr.Type` has this information, it's not passed in as part of the `tftypes.Value`. So reflect couldn't set those values, and produced broken `attr.Value`s as a result. (Their `ToTerraformValue` methods would run into trouble, because they wouldn't know what `tftypes.Type` to use for elements or attributes). To solve this problem, we decided to supply the `attr.Type` from the schema to the reflect package, wiring it through so that we could instantiate new `attr.Value`s when the opportunity presented itself. This solves our problem, because we got rid of the `attr.Value.SetTerraformValue` method and used the `attr.Type.ValueFromTerraform` directly to just instantiate a new value, which made sure it was set up correctly. But now we have a new problem: what if the `attr.Type` in the schema doesn't produce the `attr.Value` they're trying to assign to? We decided to just throw an error on that one, because there's no reasonable way around it. Depends on #44.
kmoe
added a commit
that referenced
this pull request
Jun 11, 2021
paddycarver
pushed a commit
that referenced
this pull request
Jun 12, 2021
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds the
ObjectTypeinterface, needed for reflection work.The
AttributeTypes()function could also be calledGetAttributeTypes()- I've tried to make it consistent withType.TerraformType().Will add changelog entry when approved.