-
Notifications
You must be signed in to change notification settings - Fork 114
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
Deprecating number in favour of numeric #258
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
internal/provider/string.go
Outdated
|
||
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"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Fixed.
internal/provider/string.go
Outdated
} | ||
} | ||
|
||
func customDiffIfValue(key string) func(context.Context, *schema.ResourceDiff, interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have refactored as you suggested.
internal/provider/string.go
Outdated
) | ||
} | ||
|
||
func customDiffIfValueChangeNumber() func(context.Context, *schema.ResourceDiff, interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Added.
internal/provider/string.go
Outdated
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the linter is complaining about the rawState
in the signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
## 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)). | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👩🍳 💋
// TestAccResourcePassword_StateUpgraders covers the state upgrades from v0 to V2 and V1 to V2. | ||
// This includes the addition of bcrypt_hash and numeric attributes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️
go.mod
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Needs to be updated with the appropriate tag from terraform-plugin-sdk/v2
once hashicorp/terraform-plugin-sdk#972 is merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the new testing 😍
resource.TestCheckResourceAttrSet("random_string.default", "number"), | ||
resource.TestCheckResourceAttr("random_string.default", "number", "true"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Checking the same attribute via AttrSet and Attr is redundant as the latter will fail accordingly if the state value is not present. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have removed redundant checks for TestCheckResourceAttrSet
.
func TestAccResourceString_StateUpgraders(t *testing.T) { | ||
t.Parallel() | ||
|
||
v1Cases := []struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
shouldError bool | ||
errMsg string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Fixed.
internal/provider/string.go
Outdated
}, | ||
func(_ context.Context, d *schema.ResourceDiff, _ interface{}) error { | ||
vm := d.GetRawConfig().AsValueMap() | ||
if vm["number"].IsNull() && vm["numeric"].IsNull() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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() { // ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have added checks for missing map keys.
internal/provider/string.go
Outdated
} | ||
} | ||
|
||
func customDiffIfValue(key string) func(context.Context, *schema.ResourceDiff, interface{}) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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")...
…for password and string resources
…ng test to verify behaviour
…or number and numeric
24746bd
to
40442ed
Compare
internal/provider/string.go
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open question above ☝️ but approving to unblock you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 🚀
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. |
In order to align attribute naming,
number
is being deprecated andnumeric
has been added.This gives the following attribute pairs:
In order to allow both
number
andnumeric
to co-exist and to be able to keep them in-sync and have them default totrue
when they are absent from config, the attribute schema for number was updated to removeDefault
and addComputed
.The
Default
behaviour was then replicated by usingCustomizeDiffFunc(s)
onresourcePassword
andresourceString
and the values ofnumber
andnumeric
were kept in-sync also by usingCustomizeDiffFunc(s)
.