Skip to content
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 minimum character constraints to random_string #22

Merged
merged 1 commit into from
May 21, 2018

Conversation

phekmat
Copy link
Contributor

@phekmat phekmat commented Apr 3, 2018

The motivation behind this is to more reliably create passwords that will pass complexity requirements

@phekmat phekmat force-pushed the feature/min_characters branch from bc19e0b to e535c3c Compare April 3, 2018 17:00
@phekmat phekmat force-pushed the feature/min_characters branch from e535c3c to dfabf07 Compare April 3, 2018 17:10
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@mildwonkey mildwonkey merged commit 8eb87a8 into hashicorp:master May 21, 2018
@mildwonkey
Copy link
Contributor

Thanks for adding this feature :)

@kitsunde
Copy link

This change caused all random_string created resourced to need to be recreated, which in turn was cascading through the rest of the infrastructure. If anyone else don't want to rebuild their infra you can set:

"min_lower": "0",
"min_numeric": "0",
"min_special": "0",
"min_upper": "0"

on all the random_string resources in the state file.

@mildwonkey It was surprising to me that the generate-once value forced new changes when not specifically tainted, and when it's used for resource names and such pretty disproportional to recreate the infra.

@mildwonkey
Copy link
Contributor

@kitsunde I'm sorry to hear about that and thanks for sharing the workaround. I'm certain it wasn't the intent of this PR - I will take a look at it.

@martinb3
Copy link

martinb3 commented May 22, 2018

This also bit us this morning -- huge amounts of changes due to new explicit defaults. It's also a shame since we're pinning to ~> 1.0 which means we shouldn't be seeing a dangerous change from a new feature. I'd classify it as backwards-incompatible, for semver purposes, due to the destructive nature of it.

@phekmat
Copy link
Contributor Author

phekmat commented May 22, 2018

Apologies all. @mildwonkey I'm still relatively new with terraform providers, but I think the correct approach is to use a state migration to add the defaults to the instance state. I've opened a PR with that change (#29)

@mildwonkey
Copy link
Contributor

Thank you @phekmat - I opened #28 just a few minutes before you with basically the same function. (I'm also new to this!) I'll close my PR; it was a good learning experience for me but we should let you own the fix.

@ChrisAnn
Copy link

This hit us too and we had a short production outage as a result 😭

Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants