-
Notifications
You must be signed in to change notification settings - Fork 115
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
Prevent panic on random_string
and random_password
when character set is empty
#551
Changes from all commits
be5e90d
e847d2d
99e5112
64ce4ae
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: BUG FIXES | ||
body: 'provider/random_password: Fix bug which causes panic when special, upper, lower | ||
and number/numeric are all false' | ||
time: 2024-04-10T17:54:23.219223+01:00 | ||
custom: | ||
Issue: "551" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
kind: BUG FIXES | ||
body: 'provider/random_string: Fix bug which causes panic when special, upper, lower | ||
and number/numeric are all false' | ||
time: 2024-04-10T17:55:36.979987+01:00 | ||
custom: | ||
Issue: "551" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,6 +24,7 @@ import ( | |
mapplanmodifiers "github.com/terraform-providers/terraform-provider-random/internal/planmodifiers/map" | ||
stringplanmodifiers "github.com/terraform-providers/terraform-provider-random/internal/planmodifiers/string" | ||
"github.com/terraform-providers/terraform-provider-random/internal/random" | ||
"github.com/terraform-providers/terraform-provider-random/internal/validators" | ||
) | ||
|
||
var ( | ||
|
@@ -424,6 +425,8 @@ func stringSchemaV3() schema.Schema { | |
|
||
"number": schema.BoolAttribute{ | ||
Description: "Include numeric characters in the result. Default value is `true`. " + | ||
"If `number`, `upper`, `lower`, and `special` are all configured, at least one " + | ||
"of them must be set to `true`. " + | ||
"**NOTE**: This is deprecated, use `numeric` instead.", | ||
Optional: true, | ||
Computed: true, | ||
|
@@ -432,16 +435,32 @@ func stringSchemaV3() schema.Schema { | |
boolplanmodifier.RequiresReplace(), | ||
}, | ||
DeprecationMessage: "**NOTE**: This is deprecated, use `numeric` instead.", | ||
Validators: []validator.Bool{ | ||
validators.AtLeastOneOfTrue( | ||
path.MatchRoot("special"), | ||
path.MatchRoot("upper"), | ||
path.MatchRoot("lower"), | ||
), | ||
}, | ||
}, | ||
|
||
"numeric": schema.BoolAttribute{ | ||
Description: "Include numeric characters in the result. Default value is `true`.", | ||
Optional: true, | ||
Computed: true, | ||
Description: "Include numeric characters in the result. Default value is `true`. " + | ||
"If `numeric`, `upper`, `lower`, and `special` are all configured, at least one " + | ||
"of them must be set to `true`.", | ||
Optional: true, | ||
Computed: true, | ||
PlanModifiers: []planmodifier.Bool{ | ||
boolplanmodifiers.NumberNumericAttributePlanModifier(), | ||
boolplanmodifier.RequiresReplace(), | ||
}, | ||
Validators: []validator.Bool{ | ||
validators.AtLeastOneOfTrue( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nota Bene: Definitely don't need to change this, but another consideration here might be to introduce this multiple attribute validation at the resource level as a ConfigValidator and specify all 4 attributes in there. Maybe its just me but I tend to get a little confused reading attribute-based validators (remembering "oh yeah, it includes this attribute too") and whether that attribute-based validation should be added to all the relevant attributes (confusingly, no, since it will generate multiple similar diagnostics). 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Happy to refactor along these lines, but if you're ok with the current implementation could you approve the PR if there's nothing blocking? |
||
path.MatchRoot("special"), | ||
path.MatchRoot("upper"), | ||
path.MatchRoot("lower"), | ||
), | ||
}, | ||
}, | ||
|
||
"min_numeric": schema.Int64Attribute{ | ||
|
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 we update the description of this attribute to indicate that it needs to be set with one of
special
,upper
, orlower
being true? Same with thenumber
attribute.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 shout. I've updated the description for
number
andnumeric
for bothresource_password
andresource_string
and regenerated the docs.