Add resource schema-based support for static default values #674

bendbennett · 2023-02-16T12:27:31Z

Closes: #668

…#668)

…ormDefaults (#668)

… values (#668)

bflad

Drive-by comments before we chat. 😄 Good stuff so far.

internal/fwschema/attribute_default.go

tfsdk/state.go

internal/fwserver/server_planresourcechange_test.go

resource/schema/bool_attribute.go

…in the public API (#668)

…et, single nested attributes (#668)

…d single nested attributes (#668)

austinvalle

This looks awesome! 🎉

One general question, are we planning on adding a new section to the plugin framework docs about default values and the relevant interfaces? Similar to the plan modification docs

bendbennett · 2023-03-08T17:22:33Z

One general question, are we planning on adding a new section to the plugin framework docs about default values and the relevant interfaces? Similar to the plan modification docs

Excellent question. Yep, we definitely should add to the docs. I'll attend to this. Thanks for flagging.

…tributes (#668)

bflad

Overall I think this is pretty close! Some considerations and we'll likely want to figure out the best naming/strategy for the built-in default functions since those details would immediately be protected by compatibility promises.

bflad · 2023-03-13T14:38:04Z

resource/schema/numberdefault/static_value.go

+// StaticValue returns a static number value default handler.
+//
+// Use StaticValue if a static default value for a number should be set.
+func StaticValue(defaultVal types.Number) defaults.Number {


To prevent provider developer errors and keep this similar to the other primitive type functions, it may be good to use *big.Float here to represent a known value, instead of types.Number which could be set to null/unknown. We could consider different naming for these if we want to ensure there's room to support a types type variant -- e.g. calling this StaticBigFloat(), calling the int64 one StaticInt64(), etc, then leaving "StaticValue" for the types ones.

That said, we may want to consider adding error diagnostics from the default handling methods if the given default value is null or unknown. I'm not sure its valid to ever have either -- for null it should have Computed and Default removed, for unknown it could result permanent plan differences for practitioners.

I've switched to using *big.Float and updated the primitives to use StaticBigFloat(), StaticInt() etc.

I've not added the error diagnostics for the handling methods if the default is null or unknown. Happy to do this, but I was wondering if we might want to discuss this first?

resource/schema/schema.go

bflad · 2023-03-13T14:52:44Z

resource/schema/schema.go

+	return diag.NewAttributeErrorDiagnostic(
+		path,
+		"Schema Using Attribute Default For Non-Computed Attribute",
+		fmt.Sprintf("attribute %q must be computed when using default", path.String()),


Nit: It'd be good to include our typical verbiage about "This is an issue with the provider and should be reported to the provider developers" in case a practitioner happens to receive it.

website/data/plugin-framework-nav-data.json

bflad · 2023-03-13T15:16:26Z

website/docs/plugin/framework/resources/default.mdx

+// DefaultString runs the logic of the default. Access to the path is available in `req`, while
+// `resp` contains fields for updating the planned value, and returning diagnostics.
+func (m timeNowDefaultValue) DefaultString(_ context.Context, req defaults.StringRequest, resp *defaults.StringResponse)
+    resp.PlanValue = types.StringValue(time.Now().String())


How would you feel about showing a generic time.Time default similar to our "StaticValue" functions instead of hardcoding time.Now()?

bflad · 2023-03-13T15:17:38Z

website/docs/plugin/framework/resources/plan-modification.mdx

+<Note>
+    Any Defaults that have been set are processed before any plan modifiers.
+</Note>
+


Should this just be added to the list below instead?

bflad · 2023-03-13T15:24:06Z

website/docs/plugin/framework/resources/plan-modification.mdx

    // If the value is unknown or known, do not set default value.
    if !req.PlanValue.IsNull() {
        return
    }

-    resp.PlanValue = types.StringValue(m.Default)
+    resp.PlanValue = types.StringValue(time.Now().String())


How do you feel about adjusting this to be a plan modifier that normalizes JSON-encoded strings (keeping the prior state if they match when normalized) so its separate from the default value use case?

As discussed, have used one of the predefined UseStateForUnknown plan modifiers as an example.

bflad · 2023-03-13T15:27:29Z

internal/fwschemadata/data_default.go

+			defaultValue := a.BoolDefaultValue()
+			if defaultValue != nil {
+				resp := defaults.BoolResponse{}
+				defaultValue.DefaultBool(ctx, defaults.BoolRequest{}, &resp)


It'd be good to have trace/debug logging for each of these that we're calling provider-defined logic.

Perfect, thanks!

bflad · 2023-03-13T15:29:46Z

internal/fwschemadata/data_default_test.go

+						},
+					},
+					map[string]tftypes.Value{
+						"bool_attribute": tftypes.NewValue(tftypes.Bool, false), // value in state


Should the Description be updated to reflect this instead a code comment?

Co-authored-by: Brian Flad <bflad417@gmail.com>

bflad

Excellent! Nice work. 🚀

website/docs/plugin/framework/resources/plan-modification.mdx

website/docs/plugin/framework/resources/default.mdx

website/docs/plugin/framework/migrating/attributes-blocks/default-values.mdx

bflad · 2023-03-16T23:16:45Z

resource/schema/numberdefault/static_value_test.go

+	"github.com/hashicorp/terraform-plugin-framework/types"
+)
+
+func TestStaticValueDefaultNumber(t *testing.T) {


Nit: Should this be TestStaticBigFloat to match the function name?

I've updated the naming of the /resource/schema static default test functions for primitives to make them consistent.

bflad · 2023-03-16T23:17:53Z

internal/fwschemadata/data_default.go

+			defaultValue := a.BoolDefaultValue()
+			if defaultValue != nil {
+				resp := defaults.BoolResponse{}
+				defaultValue.DefaultBool(ctx, defaults.BoolRequest{}, &resp)


Perfect, thanks!

Co-authored-by: Brian Flad <bflad417@gmail.com>

github-actions · 2023-04-20T02:09:55Z

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.

bendbennett added 9 commits February 10, 2023 17:00

Adding Default Value interfaces (#668)

8c30700

Fixing docs (#668)

7da348d

Adding framework-defined default value interfaces (#668)

a02c48f

Add framework schema interface (#668)

7a344fd

Add resource attribute interface (#668)

38c6858

Adding TransformPlanDefaults for BoolAttribute (#668)

c24881c

Modifying TransformDefaults and incorporating into PlanResourceChange (…

2ec22b0

…#668)

Add handling for float64, int64, number and string defaults in Transf…

2f47555

…ormDefaults (#668)

Add handling to prevent MarkComputedNilsAsUnknown overwriting default…

0c93366

… values (#668)

bendbennett added the enhancement New feature or request label Feb 16, 2023

bendbennett added this to the v1.2.0 milestone Feb 16, 2023

bflad reviewed Feb 16, 2023

View reviewed changes

internal/fwschema/attribute_default.go Outdated Show resolved Hide resolved

tfsdk/state.go Outdated Show resolved Hide resolved

internal/fwserver/server_planresourcechange_test.go Outdated Show resolved Hide resolved

resource/schema/bool_attribute.go Outdated Show resolved Hide resolved

bendbennett added 12 commits February 17, 2023 07:53

Add handling for list, map, object and set (#668)

40f0bb3

Add handling for list, map, object and set in PlanResourceChange (#668)

75ca3bf

Adding test coverage for list nested attribute (#668)

cc02f0f

Merge branch 'main' into bendbennett/issues-668

06d0a28

Merge branch 'main' into bendbennett/issues-668

bdd3313

Refactoring func name to {TYPE}DefaultValue() (#668)

0438d70

Removing TransformDefaults() from tfsdk.State so as not to expose it …

d2ae08e

…in the public API (#668)

Updating documentation for Default on resource schema attributes (#668)

23c6921

Adding Default to remaining resource schema attributes (#668)

54caecb

Adding further test coverage for TransformDefaults() for list, map, s…

d68d931

…et, single nested attributes (#668)

Adding further test coverage for PlanResourceChange() for map, set an…

cc78ea4

…d single nested attributes (#668)

Removing erroneous character (#668)

adb7a27

bendbennett marked this pull request as ready for review March 7, 2023 14:52

bendbennett requested a review from a team as a code owner March 7, 2023 14:52

austinvalle approved these changes Mar 8, 2023

View reviewed changes

Updating tests to use Computed everywhere (#668)

edf38a2

Adding schema validation to only permit default values on computed at…

c181416

…tributes (#668)

bendbennett added 3 commits March 10, 2023 15:37

Adding docs for default values (#668)

50e5db3

Merge branch 'main' into bendbennett/issues-668

e9b9c82

Further updates for docs (#668)

9cf0318

bflad reviewed Mar 13, 2023

View reviewed changes

bendbennett and others added 3 commits March 16, 2023 12:48

Merge branch 'main' into bendbennett/issues-668

c50f2d3

Apply suggestions from code review

4939833

Co-authored-by: Brian Flad <bflad417@gmail.com>

Updates following code review (#668)

3463d7c

bflad approved these changes Mar 16, 2023

View reviewed changes

bendbennett and others added 4 commits March 20, 2023 07:58

Merge branch 'main' into bendbennett/issues-668

32c517b

Apply suggestions from code review

1a16778

Co-authored-by: Brian Flad <bflad417@gmail.com>

Renaming test functions for consistency (#668)

bfad646

Fixing link (#668)

4849990

bendbennett merged commit cbd1cfe into main Mar 20, 2023

bendbennett deleted the bendbennett/issues-668 branch March 20, 2023 08:36

bflad added a commit that referenced this pull request Mar 20, 2023

Update CHANGELOG for #674

1f505c7

bflad added a commit that referenced this pull request Mar 21, 2023

Update CHANGELOG for #674 (#696)

918e8d1

ewbankkit mentioned this pull request Mar 24, 2023

Tech debt: Use Terraform Plugin Framework v1.2.0 default value capability hashicorp/terraform-provider-aws#30246

Closed

pksunkara mentioned this pull request Apr 12, 2023

Support null values in defaults #727

Closed

github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add resource schema-based support for static default values #674

Add resource schema-based support for static default values #674

bendbennett commented Feb 16, 2023

bflad left a comment

austinvalle left a comment

bendbennett commented Mar 8, 2023

bflad left a comment

bflad Mar 13, 2023

bendbennett Mar 16, 2023

bflad Mar 13, 2023

bflad Mar 13, 2023

bflad Mar 13, 2023

bflad Mar 13, 2023

bendbennett Mar 16, 2023

bflad Mar 13, 2023

bflad Mar 16, 2023

bflad Mar 13, 2023

bflad left a comment

bflad Mar 16, 2023

bendbennett Mar 20, 2023

bflad Mar 16, 2023

github-actions bot commented Apr 20, 2023

Add resource schema-based support for static default values #674

Add resource schema-based support for static default values #674

Conversation

bendbennett commented Feb 16, 2023

bflad left a comment

Choose a reason for hiding this comment

austinvalle left a comment

Choose a reason for hiding this comment

bendbennett commented Mar 8, 2023

bflad left a 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

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

bflad left a 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

github-actions bot commented Apr 20, 2023