terraform-plugin-sdk icon indicating copy to clipboard operation
terraform-plugin-sdk copied to clipboard

Show warnings when fixing names via StateFunc?

Open radeksimko opened this issue 8 years ago • 6 comments

This is related to https://github.com/hashicorp/terraform/pull/4020 but also many other PRs which have been mimicking the AWS API behaviour in similar ways.

I understand the convenience for the user having existing resources and I understand that is the motivation for @stack72 and others to solve these issues that way. However I think that Terraform shouldn't be so silent about these fixes.

I think we should be more transparent and at least show warnings if the converted names differ from the original ones.


I'd personally like to use warnings in a way that it would allow us to turn on strict validation in the future (after a few versions, when we're sure enough users have been warned), but I understand the convenience for existing users has probably priority and that others may not like this (correct me if I'm wrong).

radeksimko avatar Nov 24 '15 07:11 radeksimko

@radeksimko I really like this idea - we should definitely be notifying the user that we are changing their variables so that they work with the APIs we are using

stack72 avatar Nov 24 '15 11:11 stack72

I'm not sure I entirely agree with you. For an API that is case-insensitive, all of the different-cased versions of a value are semantically equivalent, and I don't really see the point in drawing attention to byte-wise diffs that don't represent a difference in meaning.

I would agree that we should never cross the line into making a change that results in a difference of meaning/behavior, but as long as we're just mimicking the same normalization that the underlying API was going to do I see this as a benefit (improved UX) rather than a drawback.

Have you encountered a real-world drawback to having these things silently normalized, from an end-user perspective?

(I know it has a very real developer drawback in that we have to keep adding all of these silly StateFuncs everywhere, but I think we've lowercased enough of these now that we could just have a reusable "make this lowercase" function that just wraps strings.ToLower to make its argument be interface{}, so we can write StateFunc: something.ToLowerCase instead of the same wrapper function each time.)

apparentlymart avatar Nov 24 '15 17:11 apparentlymart

@radeksimko / @apparentlymart was any discussion given to this outside of this issue? I feel this has sort of dropped off the face of the planet atm :)

stack72 avatar Dec 11 '15 15:12 stack72

Easiest to accomplish this when we support diagnostics in protocol 5

paultyng avatar Oct 31 '19 19:10 paultyng

This may also be related to https://github.com/hashicorp/terraform-plugin-sdk/issues/19

radeksimko avatar Nov 04 '19 13:11 radeksimko

#74 is the tracking issue for diagnostics.

paultyng avatar Feb 05 '20 21:02 paultyng