tfsdk: Allow Plan and State SetAttribute to create attribute paths #165

bflad · 2021-09-16T21:33:19Z

Closes #148

tfsdk/plan.go

paddycarver · 2021-09-20T18:53:18Z

tfsdk/plan.go

+	}
+
+	var parentTfVal tftypes.Value
+	parentPath := path.WithoutLastStep()


If this is the root, and has no parent, parentPath will be nil.

paddycarver · 2021-09-20T19:39:13Z

tfsdk/plan.go

+	if parentValue.Equal(tftypes.NewValue(tftypes.Object{}, nil)) {
+		// NewValue will panic if required attributes are missing in the
+		// tftypes.Object.
+		vals := map[string]tftypes.Value{}
+		for name, t := range parentTfType.(tftypes.Object).AttributeTypes {
+			vals[name] = tftypes.NewValue(t, nil)
+		}
+		parentValue = tftypes.NewValue(parentTfType, vals)
+	} else if parentValue.Equal(tftypes.Value{}) {
+		parentValue = tftypes.NewValue(parentTfType, nil)
+	}


Suggested change

if parentValue.Equal(tftypes.NewValue(tftypes.Object{}, nil)) {

// NewValue will panic if required attributes are missing in the

// tftypes.Object.

vals := map[string]tftypes.Value{}

for name, t := range parentTfType.(tftypes.Object).AttributeTypes {

vals[name] = tftypes.NewValue(t, nil)

}

parentValue = tftypes.NewValue(parentTfType, vals)

} else if parentValue.Equal(tftypes.Value{}) {

parentValue = tftypes.NewValue(parentTfType, nil)

}

if parentValue.IsNull() || !parentValue.IsKnown() {

var elementVal interface{}

if !parentValue.IsKnown() {

elementVal = tftypes.UnknownValue

}

switch {

case parentValue.Type().Is(tftypes.Object{}):

vals := map[string]tftypes.Value{}

for name, t := range parentTfType.(tftypes.Object).AttributeTypes {

vals[name] = tftypes.NewValue(t, elementVal)

}

parentValue = tftypes.NewValue(parentTfType, vals)

case parentValue.Type().Is(tftypes.List{}), parentValue.Type().Is(tftypes.Set{}):

parentValue = tftypes.NewValue(parentTfType, []tftypes.Value{})

case parentValue.Type().Is(tftypes.Map{}):

parentValue = tftypes.NewValue(parentTfType, map[string]tftypes.Value{})

case parentValue.Type().Is(tftypes.Tuple{}):

vals := []tftypes.Value{}

for _, t := range parentTfType.(tftypes.Tuple).ElementTypes {

vals = append(vals, tftypes.NewValue(t, elementVal)

}

parentValue = tftypes.NewValue(parentTfType, vals)

default:

panic("cry about it I guess?")

}

}

Also maybe we want this as a helper function? idk

Makes it more unit testable, at least.

bflad · 2021-09-21T18:54:04Z

Hopefully this is much more understandable now. Please let me know if you want anything else adjusted.

tfsdk/plan.go

tfsdk/schema.go

tfsdk/plan.go

paddycarver · 2021-09-23T22:52:58Z

tfsdk/plan.go

+		return false, diags
+	}
+
+	return len(remaining.Steps()) == 0, diags


Not a request for a change, just noting here that this shouldn't be necessary, it should always be true when there's no error. I think it's good to check it anyways, just wanted to surface that context on WalkAttributePath.

tfsdk/schema.go

paddycarver · 2021-09-23T23:11:34Z

tfsdk/tftypes_value.go

+// children. List, Map, and Set are created with empty elements.
+//
+// The parentType parameter ensures that the value will match the current
+// schema, not what is currently stored in the parentValue.


I think we would actually like this to actively not be the case; the schema represents type constraints (could be DynamicPseudoType, could be an object with optional attributes) but the value represents a concrete type (any non-DynamicPseudoType type, could be an object without optional attributes, respectively). So when modifying a value, I think we want to stick with the value's interpretation of what the type is supposed to be, not the schema's, so the value can stay internally consistent and match its type. Technically, it may even panic in situations where the value and type of the value start deviating.

paddycarver · 2021-09-23T23:14:03Z

tfsdk/tftypes_value.go

+// Lists can only have the next element added according to the current length.
+//
+// The parentType parameter ensures that the value will match the current
+// schema, not what is currently stored in the parentValue.


Same thing here.

paddycarver · 2021-09-23T23:18:39Z

tfsdk/tftypes_value.go

+			parentPath,
+			"Value Conversion Error",
+			"An unexpected error was encountered trying to create a value. This is always an error in the provider. Please report the following to the provider developer:\n\n"+
+				fmt.Sprintf("Unknown parent type %T to create value.", parentValue.Type()),


Suggested change

fmt.Sprintf("Unknown parent type %T to create value.", parentValue.Type()),

fmt.Sprintf("Unknown parent type %s to create value.", parentValue.Type()),

This will give us a friendlier output of what the type actually is semantically, not what the Go implementation of it is.

paddycarver · 2021-09-23T23:26:55Z

tfsdk/plan_test.go

+				}),
+				Schema: Schema{
+					Attributes: map[string]Attribute{
+						"test": {


This should appear in Raw as nil, I believe?

paddycarver · 2021-09-23T23:27:28Z

tfsdk/plan_test.go

+				}),
+				Schema: Schema{
+					Attributes: map[string]Attribute{
+						"tags": {


Same, these attributes would be in the Plan's Raw, set to null, not omitted from it.

Reference: #148

…eturned in SetAttribute handling Previously: ``` --- FAIL: TestPlanSetAttribute (0.01s) --- FAIL: TestPlanSetAttribute/write-List-AttrTypeWithValidateWarning-Element (0.00s) plan_test.go:2949: unexpected diagnostics (+wanted, -got): diag.Diagnostics( - nil, + { + diag.AttributeWarningDiagnostic{ + WarningDiagnostic: diag.WarningDiagnostic{detail: "This is a warning.", summary: "Warning Diagnostic"}, + path: s`AttributeName("test")`, + }, + }, ) --- FAIL: TestPlanSetAttribute/write-Map-AttrTypeWithValidateWarning-Element (0.00s) plan_test.go:2949: unexpected diagnostics (+wanted, -got): diag.Diagnostics( - nil, + { + diag.AttributeWarningDiagnostic{ + WarningDiagnostic: diag.WarningDiagnostic{detail: "This is a warning.", summary: "Warning Diagnostic"}, + path: s`AttributeName("test")`, + }, + }, ) --- FAIL: TestPlanSetAttribute/write-Set-AttrTypeWithValidateWarning-Element (0.00s) plan_test.go:2949: unexpected diagnostics (+wanted, -got): diag.Diagnostics( - nil, + { + diag.AttributeWarningDiagnostic{ + WarningDiagnostic: diag.WarningDiagnostic{detail: "This is a warning.", summary: "Warning Diagnostic"}, + path: s`AttributeName("test")`, + }, + }, ) --- FAIL: TestStateSetAttribute (0.01s) --- FAIL: TestStateSetAttribute/write-Map-AttrTypeWithValidateWarning-Element (0.00s) state_test.go:4349: unexpected diagnostics (+wanted, -got): diag.Diagnostics( - nil, + { + diag.AttributeWarningDiagnostic{ + WarningDiagnostic: diag.WarningDiagnostic{detail: "This is a warning.", summary: "Warning Diagnostic"}, + path: s`AttributeName("test")`, + }, + }, ) --- FAIL: TestStateSetAttribute/write-List-AttrTypeWithValidateWarning-Element (0.00s) state_test.go:4349: unexpected diagnostics (+wanted, -got): diag.Diagnostics( - nil, + { + diag.AttributeWarningDiagnostic{ + WarningDiagnostic: diag.WarningDiagnostic{detail: "This is a warning.", summary: "Warning Diagnostic"}, + path: s`AttributeName("test")`, + }, + }, ) --- FAIL: TestStateSetAttribute/write-Set-AttrTypeWithValidateWarning-Element (0.00s) state_test.go:4349: unexpected diagnostics (+wanted, -got): diag.Diagnostics( - nil, + { + diag.AttributeWarningDiagnostic{ + WarningDiagnostic: diag.WarningDiagnostic{detail: "This is a warning.", summary: "Warning Diagnostic"}, + path: s`AttributeName("test")`, + }, + }, ) FAIL ```

…e root path

…ctionality

paddycarver

LGTM, let's ship it, great work 👍 🚀

github-actions · 2021-10-24T02:08:33Z

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.

bflad added the bug Something isn't working label Sep 16, 2021

bflad added this to the v0.4.0 milestone Sep 16, 2021

bflad requested a review from a team September 16, 2021 21:33

bflad added a commit that referenced this pull request Sep 16, 2021

Update CHANGELOG for #165

44ee07c

paddycarver reviewed Sep 20, 2021

View reviewed changes

bflad mentioned this pull request Sep 21, 2021

Consider Adding Type Alias for tftypes.Transform Functions hashicorp/terraform-plugin-go#108

Closed

paddycarver assigned bflad Sep 21, 2021

anGie44 mentioned this pull request Sep 21, 2021

Resource Import Support hashicorp/terraform-provider-awscc#151

Merged

bflad removed their assignment Sep 21, 2021

bflad requested a review from paddycarver September 21, 2021 18:54

paddycarver reviewed Sep 22, 2021

View reviewed changes

tfsdk/plan.go Show resolved Hide resolved

tfsdk/schema.go Show resolved Hide resolved

tfsdk/plan.go Show resolved Hide resolved

paddycarver suggested changes Sep 23, 2021

View reviewed changes

bflad added 10 commits September 23, 2021 21:09

tfsdk: Allow Plan and State SetAttribute to create attribute paths

5e962cb

Reference: #148

Update CHANGELOG for #165

5e022d5

tfsdk: Update TestServerImportResourceState to match fixed SetAttribute

9b9efe6

tfsdk: Rearrange SetAttribute tail recursion for clarity

c753938

tfsdk: Add Plan and State pathExists methods

50e3ddf

tfsdk: Ensure some form of error is thrown trying to incorrectly writ…

e21869a

…e root path

tfsdk: Refactor setAttributeTransformFunc into more unit testable fun…

e2355fe

…ctionality

tfsdk: Make SetAttribute testing appropriate

3835b4d

tfsdk: All the fixes

daf8676

bflad force-pushed the bflad-b-SetAttribute-writes branch from 39520c6 to daf8676 Compare September 24, 2021 01:10

paddycarver approved these changes Sep 24, 2021

View reviewed changes

bflad merged commit afbf734 into main Sep 24, 2021

bflad deleted the bflad-b-SetAttribute-writes branch September 24, 2021 01:12

bflad mentioned this pull request Sep 27, 2021

tftypes: Type should implement AttributePathStepper hashicorp/terraform-plugin-go#110

Closed

github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tfsdk: Allow Plan and State SetAttribute to create attribute paths #165

tfsdk: Allow Plan and State SetAttribute to create attribute paths #165

bflad commented Sep 16, 2021

paddycarver Sep 20, 2021

paddycarver Sep 20, 2021

bflad commented Sep 21, 2021

paddycarver Sep 23, 2021

paddycarver Sep 23, 2021

paddycarver Sep 23, 2021

paddycarver Sep 23, 2021

paddycarver Sep 23, 2021

paddycarver Sep 23, 2021

paddycarver left a comment

github-actions bot commented Oct 24, 2021

-	if parentValue.Equal(tftypes.NewValue(tftypes.Object{}, nil)) {
-		// NewValue will panic if required attributes are missing in the
-		// tftypes.Object.
-		vals := map[string]tftypes.Value{}
-		for name, t := range parentTfType.(tftypes.Object).AttributeTypes {
-			vals[name] = tftypes.NewValue(t, nil)
-		}
-		parentValue = tftypes.NewValue(parentTfType, vals)
-	} else if parentValue.Equal(tftypes.Value{}) {
-		parentValue = tftypes.NewValue(parentTfType, nil)
-	}
+	if parentValue.IsNull() || !parentValue.IsKnown() {
+		var elementVal interface{}
+		if !parentValue.IsKnown() {
+			elementVal = tftypes.UnknownValue
+		}
+		switch {
+		case parentValue.Type().Is(tftypes.Object{}):
+			vals := map[string]tftypes.Value{}
+			for name, t := range parentTfType.(tftypes.Object).AttributeTypes {
+				vals[name] = tftypes.NewValue(t, elementVal)
+			}
+			parentValue = tftypes.NewValue(parentTfType, vals)
+		case parentValue.Type().Is(tftypes.List{}), parentValue.Type().Is(tftypes.Set{}):
+			parentValue = tftypes.NewValue(parentTfType, []tftypes.Value{})
+		case parentValue.Type().Is(tftypes.Map{}):
+			parentValue = tftypes.NewValue(parentTfType, map[string]tftypes.Value{})
+		case parentValue.Type().Is(tftypes.Tuple{}):
+			vals := []tftypes.Value{}
+			for _, t := range parentTfType.(tftypes.Tuple).ElementTypes {
+				vals = append(vals, tftypes.NewValue(t, elementVal)
+			}
+			parentValue = tftypes.NewValue(parentTfType, vals)
+		default:
+			panic("cry about it I guess?")
+		}
+	}

	fmt.Sprintf("Unknown parent type %T to create value.", parentValue.Type()),
	fmt.Sprintf("Unknown parent type %s to create value.", parentValue.Type()),

tfsdk: Allow Plan and State SetAttribute to create attribute paths #165

tfsdk: Allow Plan and State SetAttribute to create attribute paths #165

Conversation

bflad commented Sep 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad commented Sep 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paddycarver left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 24, 2021