Skip to content
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

Add internal/reflect package #30

Merged
merged 1 commit into from
Jun 1, 2021
Merged

Add internal/reflect package #30

merged 1 commit into from
Jun 1, 2021

Conversation

paddycarver
Copy link
Contributor

Add a package that will let us build reflect.Values from any Go type and
populate them with the data from any tftypes.Value. This culminates in
the reflect.Into function, which lets us have json.Unmarshal-esque
behavior but for tftypes.Values.

This will be used later for making aggregate types easier to work with.

@paddycarver paddycarver added the enhancement New feature or request label Jun 1, 2021
@paddycarver paddycarver added this to the v0.1.0 milestone Jun 1, 2021
@paddycarver paddycarver requested review from bflad and kmoe June 1, 2021 03:09
@paddycarver
Copy link
Contributor Author

This is based on #29 and dependent on that merging first.

@paddycarver
Copy link
Contributor Author

I am generally unhappy with the test situation here; things are tested and we have good coverage, and I'm happy with e.g. primitives, helpers, and numbers. But for larger things like structs, slices, maps, etc. I'm not sure how to test things while keeping the number of tests reasonable, because it's conceivable that e.g. setting a slice directly would work, but setting a slice as a property of an object wouldn't. Or vice-versa.

I think we may want to improve on that over time, though; I don't know that it's a problem we're able to fully solve in the moment. We can do better; I have a TODO for standalone testing of pointers and interfaces, which I think can reasonably be achieved and will add those tests, but I think the whole thing is a bit messier than I'd like.

Add a package that will let us build reflect.Values from any Go type and
populate them with the data from any tftypes.Value. This culminates in
the reflect.Into function, which lets us have json.Unmarshal-esque
behavior but for tftypes.Values.
@paddycarver
Copy link
Contributor Author

OK, I think this is ready for review again.

@paddycarver paddycarver merged commit 5905319 into main Jun 1, 2021
@paddycarver paddycarver deleted the paddy_reflection branch June 1, 2021 17:44
@kmoe kmoe mentioned this pull request Jun 2, 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.
@github-actions
Copy link

github-actions bot commented Jul 2, 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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants