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

AttributePlanModifier for default value #285

Closed
tmccombs opened this issue Apr 6, 2022 · 13 comments
Closed

AttributePlanModifier for default value #285

tmccombs opened this issue Apr 6, 2022 · 13 comments
Labels
enhancement New feature or request
Milestone

Comments

@tmccombs
Copy link

tmccombs commented Apr 6, 2022

Module version

github.com/hashicorp/terraform-plugin-framework v0.6.1

Use-cases

It is pretty common use case to have a default value for an attribute. It seems the way to do that in the plugin framework is to define a AttributePlanModifier. However, doing so requires quite a bit of boilerplate, and it probably makes sense to include this in the pre-defined modifiers included in the framework.

Note that this is limited to a static default known at the time of creating the schema, not something that computes a default from the current plan or something like that.

Attempted Solutions

User's can define their own modifies to do this, but doing so requires quite a bit of boilerplate for a very common requirement.

Proposal

Include a AttributePlanModifier to support adding a default value. Something like:

type DefaultAttributePlanModifier struct {
	Default attr.Value
}

func DefaultAttribute(value attr.Value) DefaultAttributePlanModifier {
	return DefaultAttributePlanModifier{Default: value}
}

func (m DefaultAttributePlanModifier) Modify(ctx context.Context, req tfsdk.ModifyAttributePlanRequest, resp *tfsdk.ModifyAttributePlanResponse) {
	if  resp.AttributePlan == nil || req.AttributeConfig == nil {
		return
	}

	val, err := req.AttributeConfig.ToTerraformValue(ctx)
	if err != nil {
		resp.Diagnostics.AddAttributeError(req.AttributePath,
			"Error converting config value",
			fmt.Sprintf("An unexpected error was encountered converting a %s to its equivalent Terraform representionat. This is always a bug in the provilder.\n\nError: %s", req.AttributePlan.Type(ctx), err),
		)
	}

	// if configuration was provided, then don't use the default
	if !val.IsNull() {
		return
	}

	val, err = resp.AttributePlan.ToTerraformValue(ctx)
	if err != nil {
		resp.Diagnostics.AddAttributeError(req.AttributePath,
			"Error converting plan value",
			fmt.Sprintf("An unexpected error was encountered converting a %s to its equivalent Terraform representionat. This is always a bug in the provilder.\n\nError: %s", req.AttributePlan.Type(ctx), err),
		)
	}

	// If the plan is known and not null (for example due to another plan modifier),
	// don't set the default value
	if !val.IsNull() || val.IsKnown() {
		return
	}

	resp.AttributePlan = m.Default
}

func (m DefaultAttributePlanModifier) Description(ctx context.Context) string {
	return "Use a static default value for an attribute"
}

func (m DefaultAttributePlanModifier) MarkdownDescription(ctx context.Context) string {
	return m.Description(ctx)
}

References

Additional notes.

It's a little unfortunate, that there doesn't seem to be a public interface to convert an interface{} to an attr.Value, like the inverse of ValueAs, so that the default value has to be an attr.Value instead of a native go value. At least I couldn't find such an interface.

I also don't love that I have to define Description and MarkdownDescription methods even if it is just an internal provider. I'm not even really sure how those are used for validators and plan modifiers.

Edit:

Updated code to be more correct.

@tmccombs tmccombs added the enhancement New feature or request label Apr 6, 2022
@nsilve
Copy link

nsilve commented Apr 12, 2022

Moreover there is another problem appeared regarding default values complexity after 88f0847. The default value of the "complex" attributes (e.g. lists, maps etc) cannot be just an empty list/map but the element type should be define too.

To explain better, previously the default value could be something like types.Map{}, but now it is going to break here since ToTerraformValue() firstly accesses the ElemType of the variable and then checks whether it is null.

So the way to overcome that problem, is to define more complicated default values which define their element types too:

types.Map{
	ElemType: types.ObjectType{
		AttrTypes: map[string]attr.Type{
			"value": types.StringType,
		},
	},
}

@magodo
Copy link

magodo commented Apr 20, 2022

It seems adding a default plan modifier like proposed above seems extends the scope of plan modifier to non-computed attributes?

// When providing PlanModifiers, it's necessary to set Computed to true.

@tmccombs
Copy link
Author

How so? You would still need to have Computed set to true in order to use this plan modifier.

@magodo
Copy link

magodo commented Apr 20, 2022

@tmccombs In SDK v2, setting a default value for optional and non-computed attribute is allowed. It is weired to mark an optional attribute as computed only for being able to use the default plan modifier, which also breaks the assumption for O+C attributes, i.e. if it is unset, keeps using whatever value set remotely (but now it will always use the default value).

@magodo
Copy link

magodo commented Apr 20, 2022

@tmccombs BTW, should the check above be modified to, as otherwise, it will always be resolved to true?

	if val.IsKnown() && !val.IsNull() {
		return
	}

@tmccombs
Copy link
Author

In SDK v2, setting a default value for optional and non-computed attribute is allowed. It is weired to mark an optional attribute as computed only for being able to use the default plan modifier

But the framework doesn't have a native way to set a default value. And iirc that has been discussed on a couple of other issues. The current recommendation is to use a plan modifier to set a default value, this issue is just about making it easier to do that by providing a plan modifier implementation for setting a default value, so that every provider doesn't have to re-invent the wheel.

which also breaks the assumption for O+C attributes, i.e. if it is unset, keeps using whatever value set remotely (but now it will always use the default value).

I'm unfamiliar with that assumption. But if that assukmption is something that should be upheld, then maybe having native support for default values in the framework should be revisited.

BTW, should the check above be modified to, as otherwise, it will always be resolved to true?
yes, I think so

@bflad
Copy link
Member

bflad commented Apr 22, 2022

Drive-by note: Just to provide some context here, the framework should/will supply some standard plan modifiers that can handle at least primitive type default values. It'll be helpful for the maintainers to understand use cases, because as mentioned above, sometimes choosing a desired behavior may require choosing a separate implementation:

  • Where an attribute will preserve any existing state value (e.g. a configured value different than the default) when a configuration is removed rather than reverting to a default
  • Where an attribute will always revert to a default value when a configuration is removed

terraform-plugin-sdk was allowed to do things that are not allowed in other provider implementations and also performed some very opinionated plan logic. There is no longer forced opinions based on Computed-ness. Computed should always be required for any framework or provider-defined default value implementation outside the old SDK, since it signals to Terraform that a value not in a configuration may come from the provider. There may be consideration in #31 to just always set Computed with Optional, to simplify provider implementations.

@tpaul1611
Copy link

Just implemented this thing myself, really unintuitive to not have a default value modifier in the framework, since its implementation is so generic. Looking forward to see more modifiers 👍

@lambdalisue
Copy link

lambdalisue commented Jul 22, 2022

It seems the following simple code is enough with v0.10.0. It's just for note. I'm really welcome to see the official version 👍

package modifier

import (
	"context"

	"github.com/hashicorp/terraform-plugin-framework/attr"
	"github.com/hashicorp/terraform-plugin-framework/tfsdk"
)

// DefaultAttributePlanModifier is to set default value for an attribute
// https://github.com/hashicorp/terraform-plugin-framework/issues/285
type DefaultAttributePlanModifier struct {
	value attr.Value
}

func DefaultAttribute(value attr.Value) DefaultAttributePlanModifier {
	return DefaultAttributePlanModifier{value: value}
}

func (m DefaultAttributePlanModifier) Modify(
	ctx context.Context,
	req tfsdk.ModifyAttributePlanRequest,
	resp *tfsdk.ModifyAttributePlanResponse,
) {
	if req.AttributeConfig == nil || resp.AttributePlan == nil {
		return
	}

	// if configuration was provided, then don't use the default
	if !req.AttributeConfig.IsNull() {
		return
	}

	// If the plan is known and not null (for example due to another plan modifier),
	// don't set the default value
	if !resp.AttributePlan.IsUnknown() && !resp.AttributePlan.IsNull() {
		return
	}

	resp.AttributePlan = m.value
}

func (m DefaultAttributePlanModifier) Description(ctx context.Context) string {
	return "Use a static default value for an attribute"
}

func (m DefaultAttributePlanModifier) MarkdownDescription(ctx context.Context) string {
	return m.Description(ctx)
}

And usage is

// ...
"kind": {
	Optional:            true,
	Computed:            true,
	Type:                types.StringType,
	PlanModifiers: tfsdk.AttributePlanModifiers{
		modifier.DefaultAttribute(types.String{Value: "foo"}),
	},
},

Note that the terraform-provider-scaffolding-framework use req.Config.Get() in Create but I needed to replace it to req.Plan.Get() to use this modifier. Related issue is hashicorp/terraform-provider-scaffolding-framework#32

@pksunkara
Copy link

Where an attribute will always revert to a default value when a configuration is removed

FWIW I looked into all the scenarios a field generally behaves like and implemented modifiers for them at https://github.com/terraform-community-providers/terraform-plugin-framework-utils#modifiers.

Where an attribute will preserve any existing state value (e.g. a configured value different than the default) when a configuration is removed rather than reverting to a default

Turns out that we don't need anything else other than UseStateForUnknown for this scenario.


A test provider is implemented at https://github.com/terraform-community-providers/terraform-plugin-framework-utils/tree/master/test/terraform-provider-test along with tests at https://github.com/terraform-community-providers/terraform-plugin-framework-utils/tree/master/test/infra-test/specs.

I am also going to use this in https://github.com/terraform-community-providers/terraform-provider-linear.

@magodo
Copy link

magodo commented Dec 5, 2022

@bflad Is there a plan to make an official repo for plan modifiers, just like github.com/hashicorp/terraform-plugin-framework-validators? I raise this ask is because some customized plan modifiers broke due to #565, so it would be nice if there is an official one for this part.

@bflad
Copy link
Member

bflad commented Mar 20, 2023

This should be covered by #668 which will release with terraform-plugin-framework version 1.2.0 🔜 . If there are further bug reports, feature requests, or documentation suggestions with resource default value handling, please raise a new issue. 👍

@bflad bflad closed this as completed Mar 20, 2023
@github-actions
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, 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 Apr 20, 2023
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

No branches or pull requests

7 participants