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

OptionalAttributes are not optional #83

Open
bschaeffer opened this issue Apr 30, 2021 · 4 comments
Open

OptionalAttributes are not optional #83

bschaeffer opened this issue Apr 30, 2021 · 4 comments
Labels
bug Something isn't working

Comments

@bschaeffer
Copy link

terraform-plugin-go version

v0.3.0

Relevant provider source code

package main

import (
	"github.com/hashicorp/terraform-plugin-go/tfprotov5"
	"github.com/hashicorp/terraform-plugin-go/tftypes"
)

var (
	objType = tftypes.Object{
		AttributeTypes: map[string]tftypes.Type{
			"id":   tftypes.Number,
			"name": tftypes.String,
		},
		OptionalAttributes: map[string]struct{}{
			"name": struct{}{},
		},
	}
)

func main() {
	value := tftypes.NewValue(objType, map[string]tftypes.Value{
		"id":   tftypes.NewValue(tftypes.Number, 1),
	})

	_, err := tfprotov5.NewDynamicValue(value.Type(), value)
	if err != nil {
		panic(err)
	}
}

Terraform Configuration Files

Unrelated

Expected Behavior

I am able to create a DyanmicValue without error.

Actual Behavior

panic: AttributeName("name"): no value set

Steps to Reproduce

Run the provided script.

References

@bschaeffer bschaeffer added the bug Something isn't working label Apr 30, 2021
bschaeffer added a commit to bschaeffer/terraform-plugin-go that referenced this issue Apr 30, 2021
@bschaeffer
Copy link
Author

bschaeffer commented Apr 30, 2021

You can overcome this by instead supplying a nil Value, but its not super clear that was the solution

tftypes.NewValue(tftypes.String, nil)

Though I am not sure if that's the same thing as making sure you don't store in state any attributes the user hasn't explicitly provided as config input. How to respect the users desire not to track an attribute in state is what brought me here.

@alexsomesan
Copy link
Member

Plus one on this. It seems we're hitting this in the Kubernetes alpha provider too.

Here's my version of a repro snippet:

package main

import (
	"log"

	"github.com/hashicorp/terraform-plugin-go/tfprotov5"
	"github.com/hashicorp/terraform-plugin-go/tftypes"
)

func main() {
	ot := tftypes.Object{
		AttributeTypes: map[string]tftypes.Type{
			"a": tftypes.String,
			"b": tftypes.String,
		},
		OptionalAttributes: map[string]struct{}{
			"b": {},
		},
	}

	val := tftypes.NewValue(ot, map[string]tftypes.Value{
		"a": tftypes.NewValue(tftypes.String, "foo"),
	})

	dval, err := tfprotov5.NewDynamicValue(ot, val)
	if err != nil {
		log.Fatalf("NewDynamicValue error: %s", err)
	}

	log.Printf("MsgPack: %x\n", dval.MsgPack)

	nval, err := dval.Unmarshal(ot)
	if err != nil {
		log.Fatalf("ValueFromMsgPack error: %s", err)
	}

	log.Printf("%+v\n", nval)
}

paddycarver added a commit that referenced this issue Aug 5, 2021
Update our Type interface to include UsableAs and Equal explicitly.

Add placeholders for UsableAs and Equal on all types, so tests can run.

Update String, Number, Bool, and DynamicPseudoType to have UsableAs
implementations, and tests for those implementations. DynamicPseudoType
will panic when UsableAs is called on it, because DynamicPseudoType is a
type constraint, it should never be associated with a Value.

This work will allow us to resolve #83.
@paddycarver
Copy link
Contributor

I think this is working as intended: optional attributes are about what the practitioner has to set in the config, they don't really influence what gets set in state. Is the request here to optionally not track things in state? Does setting things to null not fulfill this need? Or is it just about not needing to set things to null explicitly when creating the objects?

@bschaeffer
Copy link
Author

@paddycarver I think the request here is maybe for more documentation... I came to the solution of tftypes.NewValue(tftypes.String, nil) on my own (or maybe reading other provider code), not through docs or anything.

Having said that, the docs in this code base have been 💯 in all other cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants