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

Define State type #20

Closed
wants to merge 5 commits into from
Closed

Define State type #20

wants to merge 5 commits into from

Conversation

kmoe
Copy link
Member

@kmoe kmoe commented May 10, 2021

WIP, also contains req/resp types for now

extends from #11

Summary:

type State struct {
  Raw tfprotov6.DynamicValue
  Schema Schema
}

func (s *State) Get(ctx context.Context, attributePath *tftypes.AttributePath, attrType attribute.AttributeType) (attribute.AttributeValue, error) {}

func (s *State) Set(ctx context.Context, attributePath *tftypes.AttributePath, val interface{}) error {}

There's also an example of a type-specific Get helper, GetString, which returns a types.String. We could have one of these for each primitive type, but it's not possible for structural types due to the need to specify ElemType.

Puzzle:

It would be nice if users didn't have to make type assertions against values they get from state. I couldn't figure out how to make a version of Get which took a target *attribute.AttributeValue or target interface{} (pointer) and just marshalled the value into that, since AttributeValue is an interface.

Still thinking:

  • AttributePath helpers
    • I expect the vast majority of .Get() and .Set() calls will concern top-level attributes, so we should either have:
      • a quick helper Attr(attributeName string) tftypes.AttributePath to plug into state methods, or
      • versions of the state methods which take a string instead of tftypes.AttributePath, e.g. Get(context.Context, string, *tftypes.AttributePath) (attribute.AttributeValue, error)
  • Nested attributes
    • doesn't work yet

paddycarver and others added 5 commits May 5, 2021 08:08
Start building out the schema type, leaving some TODOs for things I'm
still mulling over and thinking about.

This also brings in the StringKind type, which is a pattern I'm not
thrilled about and we should probably think about it some more.

I also included the AttributeType and AttributeValue types, which I'll
add a package with some builtins for in a future commit, to show off how
they're used.
Add implementations of lists and strings, so we can see what an actual
type may look like under this system, and to make sure that complex
types still work under this system.
@kmoe
Copy link
Member Author

kmoe commented May 12, 2021

This is not in any way ready to merge, but I'm setting it to ready to review now because I don't want it to block anything while I'm away. Feel free to add/change commits on this branch.

I think the best API for Get() would involve the provider initialising an AttributeValue variable, passing the pointer, and having the framework set its value.

We should also have a State function that returns a map[string]AttributeValue using the types from the schema.

With these two in place, we probably don't need GetString and friends. Type assertions in provider code are still needed, but I think this is the best we can do.

@kmoe kmoe marked this pull request as ready for review May 12, 2021 18:03
@paddycarver paddycarver added this to the v0.1.0 milestone May 18, 2021
@paddycarver paddycarver added the enhancement New feature or request label May 18, 2021
request.go Show resolved Hide resolved
server.go Show resolved Hide resolved
if err != nil {
return ret, fmt.Errorf("error converting from tftypes.Value: %s", err)
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this all be replaced with a call to Get?

Suggested change
attrValue, err := s.Get(ctx, attributePath, types.StringType{})
if err != nil {
return ret, err
}

return attrValue, nil
}

func (s *State) Set(ctx context.Context, attributePath *tftypes.AttributePath, val interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we expect an AttributeValue here?

Suggested change
func (s *State) Set(ctx context.Context, attributePath *tftypes.AttributePath, val interface{}) error {
func (s *State) Set(ctx context.Context, attributePath *tftypes.AttributePath, val AttributeValue) error {

@@ -0,0 +1,98 @@
package tf
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use tf_test as the package name to avoid import loops, I think.

}, tftypes.NewValue(tftypes.Object{
AttributeTypes: map[string]tftypes.Type{
"my_string": tftypes.String,
"id": tftypes.String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe id would need to be part of the object type, too.

Value: "katy",
}

if actualStr != expectedStr {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think you want

Suggested change
if actualStr != expectedStr {
if !actualStr.Equal(expectedStr) {

@paddycarver
Copy link
Contributor

I think the best API for Get() would involve the provider initialising an AttributeValue variable, passing the pointer, and having the framework set its value.

I also think that's an interesting direction. #25 has the beginnings of a stab at that approach, built on #12. Specifically commit 2085e26 implements the approach in get.go. It's very rough and was basically a prototype to see how that might work. I think a Set (maybe differently named?) could be used to wholesale replace the state or plan when writing for the response. I don't know.

We should also have a State function that returns a map[string]AttributeValue using the types from the schema.

I'm not sure I disagree, but just know that we can't have the map values be types from the schema, they would need to be AttributeValues, and the user would need to type assert them. But I think that may be a valid approach, too. I'm wondering if we just give users all of these approaches and see what they use, a cull the ones that don't make sense at a later date...

In theory, we could have something like....

type State struct {
  // ....
}

// Get populates the struct passed as `target` with the entire state. No type assertion necessary.
func (s *State) Get(ctx context.Context, target interface{}) error {}

// GetAttribute retrieves the attribute found at `path` and returns it as an attr.Value,
// which provider developers need to assert the type of
func (s *State) GetAttribute(ctx context.Context, path attr.Path) (attr.Value, error) {}

// Set replaces the entire state with the values of the struct passed as `value`.
func (s *State) Set(ctx context.Context, value attr.Value) error {}

// SetAttribute sets the attribute found at `path` with the value passed as `value`
func (s *State) SetAttribute(ctx context.Context, path attr.Path, value attr.Value) error {}

And then if users want to translate the state to provider-defined Go types at the boundaries and work with only compiler-checked types internally and not have any chance of a typo not getting caught and causing a bug, they can use Get and Set. If they don't want to define the extra types are want to be more surgical about what they're retrieving or setting, they can use GetAttribute and SetAttribute. We could even use a GetAttributes and SetAttributes that returns/accepts a map[string]attr.Value for use cases where people just want to get a map of the data and not have to care about what all the keys are. (Though I still think those use cases are usually.... misguided.)

@kmoe
Copy link
Member Author

kmoe commented May 25, 2021

State work is superseded by #25/#26 (work ongoing), and request/response work is split out into #27.

@kmoe kmoe closed this May 25, 2021
@github-actions
Copy link

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 Jun 25, 2021
@bflad bflad deleted the katystate branch January 11, 2023 17:32
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