Deprecating number in favour of numeric #258

bendbennett · 2022-05-24T11:52:57Z

In order to align attribute naming, number is being deprecated and numeric has been added.
This gives the following attribute pairs:

numeric (bool)
min_numeric (int)
lower (bool)
min_lower (int)
upper (bool)
min_upper (int)
..............

In order to allow both number and numeric to co-exist and to be able to keep them in-sync and have them default to true when they are absent from config, the attribute schema for number was updated to remove Default and add Computed.

The Default behaviour was then replicated by using CustomizeDiffFunc(s) on resourcePassword and resourceString and the values of number and numeric were kept in-sync also by using CustomizeDiffFunc(s).

detro

Looking great. I just left a few comments about... adding godoc comments :)

The problem you solved is pretty complex, so it's actually crucial that your use of CustomizeDiff is accompanied by godoc that will help the next figure out what problem you solved there.

detro · 2022-05-25T20:32:07Z

internal/provider/string.go

+
+		number, ok := rawState["number"].(bool)
+		if !ok {
+			return nil, fmt.Errorf("resource %s state upgrade failed, number could not be asserted as bool: %T", resourceName, rawState["number"])


Suggested change

return nil, fmt.Errorf("resource %s state upgrade failed, number could not be asserted as bool: %T", resourceName, rawState["number"])

return nil, fmt.Errorf("resource %s state upgrade failed, 'number' is not a boolean: %T", resourceName, rawState["number"])

Just thinking it could be easier to understand for the practitioner. Feel free to discard.

Agreed. Fixed.

detro · 2022-05-25T20:34:31Z

internal/provider/string.go

+	}
+}
+
+func customDiffIfValue(key string) func(context.Context, *schema.ResourceDiff, interface{}) error {


I think this function needs a bit of godoc to explain what's needed for. I remember a bit of what you explained verbally, but I will forget in a few hours.

Agreed. Added.

The naming and single parameter confused me a little bit here at first glance! I wonder if it is worth renaming and making this slightly more generic (not sure how useful it'd be outside this provider, but I think the naming and signature will make it more clear what the intent is), e.g.

func planDefaultIfAllNull(default interface{}, keys ...string) []schema.CustomizeDiffFunc { var result []schema.CustomizeDiffFunc for _, key := range keys { result = append(result, customdiff.IfValue(/*...*/)) } return result }

With the callers then being planDefaultIfAllNull(true, "number", "numeric")...

Have refactored as you suggested.

detro · 2022-05-25T20:36:15Z

internal/provider/string.go

+	)
+}
+
+func customDiffIfValueChangeNumber() func(context.Context, *schema.ResourceDiff, interface{}) error {


Same as per function above (and below).

Just something to explain the whole "if number has changed, we want to prograpage to numeric" and vice-versa.

Agreed. Added.

detro · 2022-05-25T20:36:46Z

internal/provider/string.go

@@ -257,3 +322,67 @@ func generateRandomBytes(charSet *string, length int) ([]byte, error) {
 func readNil(_ context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics {
 	return nil
 }
+
+func resourceStateUpgradeAddNumeric(_ context.Context, rawState map[string]interface{}, _ interface{}, resourceName string) func(_ context.Context, rawState map[string]interface{}, _ interface{}) (map[string]interface{}, error) {


I think the linter is complaining about the rawState in the signature.

detro

LGTM

detro · 2022-05-30T13:49:24Z

CHANGELOG.md

+## 3.3.0 (Unreleased)
+
+ENHANCEMENTS:
+
+* resource/random_password: `number` is now deprecated and `numeric` has been added to align attribute naming. `number` will be removed in the next major release ([#258](https://github.com/hashicorp/terraform-provider-random/pull/258)).
+* resource/random_string: `number` is now deprecated and `numeric` has been added to align attribute naming. `number` will be removed in the next major release ([#258](https://github.com/hashicorp/terraform-provider-random/pull/258)).
+


👩‍🍳 💋

detro · 2022-05-30T13:54:37Z

internal/provider/resource_password_test.go

+// TestAccResourcePassword_StateUpgraders covers the state upgrades from v0 to V2 and V1 to V2.
+// This includes the addition of bcrypt_hash and numeric attributes.


bendbennett · 2022-05-30T14:40:07Z

go.mod

 	github.com/hashicorp/go-uuid v1.0.3
 	github.com/hashicorp/terraform-plugin-docs v0.8.1
-	github.com/hashicorp/terraform-plugin-sdk/v2 v2.16.0
+	github.com/hashicorp/terraform-plugin-sdk/v2 v2.16.1-0.20220527155737-8a0031d0a59f


TODO: Needs to be updated with the appropriate tag from terraform-plugin-sdk/v2 once hashicorp/terraform-plugin-sdk#972 is merged.

bflad

All the new testing 😍

bflad · 2022-05-31T14:37:15Z

internal/provider/resource_string_test.go

+				resource.TestCheckResourceAttrSet("random_string.default", "number"),
+				resource.TestCheckResourceAttr("random_string.default", "number", "true"),


Nit: Checking the same attribute via AttrSet and Attr is redundant as the latter will fail accordingly if the state value is not present. 👍

Have removed redundant checks for TestCheckResourceAttrSet.

bflad · 2022-05-31T14:41:24Z

internal/provider/resource_string_test.go

+func TestAccResourceString_StateUpgraders(t *testing.T) {
+	t.Parallel()
+
+	v1Cases := []struct {


Meta Terraform Provider acceptance testing note: most providers tend to "write out" acceptance test cases rather than trying to create testing looping constructs, etc. This was previously a convention to help newer Gophers more easily run individual tests (usually just single tests with underscores in names) over learning that they can do parent/child syntax with the go test -run flag. This isn't such a big deal in providers like this one which don't have long-running CRUD methods, such as those that provision API-based resources, but just mentioning.

bflad · 2022-05-31T14:43:11Z

internal/provider/resource_string_test.go

+		shouldError     bool
+		errMsg          string


Nit: You can save on the separate bool by doing something like expectedErr error as error will be nil as its zero-value and you can still perform message checking by doing expectedErr.Error() to get the string out.

Good point. Fixed.

bflad · 2022-05-31T14:54:12Z

internal/provider/string.go

+		},
+		func(_ context.Context, d *schema.ResourceDiff, _ interface{}) error {
+			vm := d.GetRawConfig().AsValueMap()
+			if vm["number"].IsNull() && vm["numeric"].IsNull() {


Should these map accesses have panic -> error protection for missing map keys? e.g.

number, numberOk := vm["number"] numeric, numericOk := vm["numeric"] if !numberOk || !numericOk { return fmt.Errorf("yikes!") } if number.IsNull() && numeric.IsNull() { // ...

Have added checks for missing map keys.

bflad · 2022-05-31T17:55:30Z

internal/provider/string.go

+	}
+}
+
+func customDiffIfValue(key string) func(context.Context, *schema.ResourceDiff, interface{}) error {


The naming and single parameter confused me a little bit here at first glance! I wonder if it is worth renaming and making this slightly more generic (not sure how useful it'd be outside this provider, but I think the naming and signature will make it more clear what the intent is), e.g.

func planDefaultIfAllNull(default interface{}, keys ...string) []schema.CustomizeDiffFunc { var result []schema.CustomizeDiffFunc for _, key := range keys { result = append(result, customdiff.IfValue(/*...*/)) } return result }

With the callers then being planDefaultIfAllNull(true, "number", "numeric")...

…null in config

…for password and string resources

…n TestStep.

…ng test to verify behaviour

…or number and numeric

…tStep

bflad · 2022-06-03T12:41:36Z

internal/provider/string.go

+func resourceStateUpgradeAddNumeric(resourceName string) func(_ context.Context, rawState map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
+	return func(_ context.Context, rawState map[string]interface{}, _ interface{}) (map[string]interface{}, error) {
+		if rawState == nil {
+			return nil, fmt.Errorf("resource %s state upgrade failed, state is nil", resourceName)


Does Terraform CLI not already make it clear which resource address failed, which would inherently include the resource name? If not, maybe we should raise a feature request upstream so adding the resource name, which may not be the most helpful in large configurations, is not necessary in provider code.

I've just checked this. Terraform CLI does make it clear which resource address failed. For instance,

│ Error: state upgrade failed, state is nil │ │ with random_password.password, │ on resource.tf line 1, in resource "random_password" "password": │ 1: resource "random_password" "password" {

Consequently, I have removed usage of the resource name from the error messages in and refactored to replace resourceStateUpgradeAddNumeric func with resourcePasswordStringStateUpgradeV1.

bflad

Open question above ☝️ but approving to unblock you.

bflad

Looks good to me 🚀

github-actions bot added size/M size/L size/XL and removed size/M size/L labels May 24, 2022

detro reviewed May 25, 2022

View reviewed changes

github-actions bot added the documentation label May 26, 2022

bendbennett added this to the v3.3.0 milestone May 26, 2022

bendbennett marked this pull request as ready for review May 26, 2022 09:28

bendbennett requested a review from a team as a code owner May 26, 2022 09:28

github-actions bot added the dependencies label May 27, 2022

bendbennett requested a review from detro May 30, 2022 11:02

detro approved these changes May 30, 2022

View reviewed changes

bendbennett commented May 30, 2022

View reviewed changes

bendbennett requested a review from bflad May 31, 2022 14:56

bflad reviewed May 31, 2022

View reviewed changes

bendbennett added 14 commits June 3, 2022 12:37

Deprecating number in favour of numeric

b1d820b

Switching to using a state upgrader for deprecation of number attribute

15c6e3b

Using HasChange to detect whether attribute value has been altered

71b3e9d

Using CustomizeDiff to keep number and numeric in-sync

bccd2c7

Examine raw config to determine whether number and numeric have been set

b35d514

Extend CustomizeDiff to set number and numeric to true when both are …

194e8bd

…null in config

Renaming misspelled test file

33f8878

Adding test for state upgrade v1

dc2b121

Adding test for updating number and numeric

af7fa64

Adding CustomizeDiff and tests for state upgrade and customize diff

28ed88c

Fix import state func tests

318fc7c

DRY-up func for adding number to state during resource state upgrade …

3cc6aaa

…for password and string resources

DRY-up funcs for customize diff

8530696

Removing unnecessary func args for state upgrader

333d4d4

bendbennett added 18 commits June 3, 2022 12:37

Updating docs to deprecate number and add numeric

bc50035

Rewording error messages

dd28e5b

Adding docs to provide context to the usage of customize diff funcs

5edfc96

Updating CHANGELOG.md

d65b4d3

Setting the CHANGELOG as unreleased

8d66210

Temporarily using sdk/v2 sha to use ExternalProviders in TestStep

f45ca85

Adding test to verify state upgrades by using ExternalProviders withi…

2f596d5

…n TestStep.

Bumping SDK version to use ExternalProviders in TestStep fix and addi…

102133a

…ng test to verify behaviour

Adding test cases to verify random password state upgrade behaviour f…

80f5843

…or number and numeric

Consolidating V0 to V2 state upgrade tests

121e1d2

Update unit tests for state upgraders

10efe17

Consolidate testing of state upgraders

6095a62

Adding testing of state upgrade from v1 to v2 for random string

fd6f3de

Using v2.17.0 of terraform-plugin-sdk/v2 for ExternalProviders in Tes…

fccb472

…tStep

Remove redundant checks for TestCheckResourceAttrSet

59eb920

Remove shouldError bool from tests

0e0d7c5

Refactoring customize diff funcs

7a1c9be

Updating dependencies

40442ed

bendbennett force-pushed the bendbennett/rename-number branch from 24746bd to 40442ed Compare June 3, 2022 11:44

github-actions bot removed the dependencies label Jun 3, 2022

Adding terraform 1.2.* to testing matrix

f4bb2c3

bendbennett requested a review from bflad June 3, 2022 12:13

bflad reviewed Jun 3, 2022

View reviewed changes

bflad approved these changes Jun 3, 2022

View reviewed changes

Refactoring to remove usage of resource name in errors

3e61cf7

bflad approved these changes Jun 6, 2022

View reviewed changes

bendbennett merged commit 96bc479 into main Jun 6, 2022

bendbennett deleted the bendbennett/rename-number branch June 6, 2022 10:58

arthurbdiniz mentioned this pull request Oct 11, 2022

Fix random_string resource numeric input deprecation DNXLabs/terraform-aws-securityhub#9

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecating number in favour of numeric #258

Deprecating number in favour of numeric #258

bendbennett commented May 24, 2022 •

edited

detro left a comment

detro May 25, 2022

bendbennett May 26, 2022

detro May 25, 2022

bendbennett May 26, 2022

bflad May 31, 2022

bendbennett Jun 3, 2022

detro May 25, 2022

bendbennett May 26, 2022

detro May 25, 2022

detro left a comment

detro May 30, 2022

detro May 30, 2022

bendbennett May 30, 2022 •

edited

bflad left a comment

bflad May 31, 2022

bendbennett Jun 3, 2022

bflad May 31, 2022

bflad May 31, 2022

bendbennett Jun 3, 2022

bflad May 31, 2022

bendbennett Jun 3, 2022

bflad May 31, 2022

bflad Jun 3, 2022

bendbennett Jun 6, 2022

bflad left a comment

bflad left a comment

	return nil, fmt.Errorf("resource %s state upgrade failed, number could not be asserted as bool: %T", resourceName, rawState["number"])
	return nil, fmt.Errorf("resource %s state upgrade failed, 'number' is not a boolean: %T", resourceName, rawState["number"])

		// TestAccResourcePassword_StateUpgraders covers the state upgrades from v0 to V2 and V1 to V2.
		// This includes the addition of bcrypt_hash and numeric attributes.

		resource.TestCheckResourceAttrSet("random_string.default", "number"),
		resource.TestCheckResourceAttr("random_string.default", "number", "true"),

Deprecating number in favour of numeric #258

Deprecating number in favour of numeric #258

Conversation

bendbennett commented May 24, 2022 • edited

detro 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

detro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bendbennett May 30, 2022 • edited

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

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

bflad left a comment

Choose a reason for hiding this comment

bendbennett commented May 24, 2022 •

edited

bendbennett May 30, 2022 •

edited