-
Notifications
You must be signed in to change notification settings - Fork 118
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
Add random_bytes
resource
#494
Conversation
…intended to be used as key or secret
…uiresReplaceIfValuesNotNull
…erChange that was not failing when it should be
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 🚀
@@ -0,0 +1,2 @@ | |||
# Random bytes can be imported by specifying the value as base64 string. | |||
terraform import random_bytes.basic 8/fu3q+2DcgSJ19i0jZ5Cw== |
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: Not sure if we should quote the example value since some shells may try to treat the base64 character set as special. 👍
internal/provider/resource_bytes.go
Outdated
// mapplanmodifiers.RequiresReplaceIfValuesNotNull() has been used for consistency with other | ||
// resources but mapplanmodifier.RequiresReplace() could have been used as there shouldn't be any | ||
// prior state storage from a terraform-plugin-sdk based resource which would've collapsed a map | ||
// of null values into a null map. | ||
mapplanmodifiers.RequiresReplaceIfValuesNotNull(), |
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: I wonder if we should try to save the technical debt in this new implementation by using the regular validator here since there was never any prior state from terraform-plugin-sdk? I'm personally not worried about code-level consistency for this situation in new implementations. We can always "downgrade" to the special validator (or consider updating upstream) if there are real behavior issues with the regular validator.
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.
That makes sense to me. I've replaced mapplanmodifiers.RequiresReplaceIfValuesNotNull()
with mapplanmodifier.RequiresReplace()
, and removed the redundant comment.
…RequiresReplaceIfValuesNotNull() * There should be no instances of prior state for random_bytes as it was never present in the SDKv2 version of the random provider
Thanks for this @bendbennett! (I've been following #272 for quite some time.) Will you be releasing the provider (https://registry.terraform.io/providers/hashicorp/random/3.5.1) soon to get this new functionality available? |
@dhermes we should be releasing this early next week since we try to not release late in the week. I would imagine 3.6.0 will land Monday or Tuesday. |
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. |
Closes: #63
Closes: #272
This PR incorporates all of the commits from Add new resource random_bytes to generate an array of random bytes.
Additional changes include:
result_base64
=>base64
, andresult_hex
=>hex
id
attribute from the schema