-
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 migration and new schema version for resource_string #29
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.
Thanks for working on this follow-up fix, @phekmat!
This looks good to me. I have one bit of feedback inline, just to handle this cautiously for anyone who is already using this feature in v1.3.0.
random/resource_string_migration.go
Outdated
is.Attributes["min_numeric"] = "0" | ||
is.Attributes["min_upper"] = "0" | ||
is.Attributes["min_lower"] = "0" | ||
is.Attributes["min_special"] = "0" |
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.
Because we've now shipped a version where these did exist in schema version 0, I think for safety we need to handle these conditionally so that we don't inadvertently cause further problems for people who have already applied changes with the v1.3.0 release.
I think it should be sufficient to just apply this change conditionally only if the attribute is not already set:
if v := is.Attributes["min_special"]; v == "" {
is.Attributes["min_special"] = "0"
}
This would ensure that we don't overwrite any nonzero values that might have been inserted by users already trying to use the new feature with v1.3.0
, which would then cause another forced replacement for those users.
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 catch. Done
83cbcb7
to
287d49a
Compare
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.
👍 thanks!
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. |
This should resolve the issues with forcing a new resource described in the comments of #22