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

Random string generator #5

Merged
merged 2 commits into from
Sep 7, 2017

Conversation

ryan-dyer-sp
Copy link

Random string generator which can be used in places where other random_ resources do not suffice. ie for passwords where due to compliance reasons, alphanumeric and special characters are required.

@apparentlymart
Copy link
Contributor

Hi @ryan-dyer-sp! Thanks for working on this.

This looks good to me from an implementation standpoint.

I have a question about the use-case, though: if generating random passwords is the goal, shouldn't it be able to guarantee at least one of each requested class of character in order to comply with password requirements? Otherwise this resource would not necessarily generate policy-compatible passwords, I think.

Looking at this I also notice that the tests in this provider are all using resource.Test rather than resource.UnitTest, which means they will only work when in acceptance test mode. I'd like to fix that in a separate PR since these tests don't interact with anything outside of Terraform anyway; perhaps you'd like to update just your new test here to use UnitTest and then I can take care of the rest separately. (No worries if not... I can just wait until this is merged and do it.)

Thanks again!

@ryan-dyer-sp
Copy link
Author

In regards to the use case, you're right. I'm just looking to get something that's close and generic. Given a high enough length, its extremely unlikely you would encounter a string which wouldn't contain at least one lower, upper, num, and special. The use case of passwords and exact compliance would be a significantly more difficult undertaking imo.

And in regards to the test, I just mimiced what the random_id was doing for tests. I cant say I'm familiar with the proper way of doing this.

ryan-dyer-sp and others added 2 commits September 7, 2017 13:23
This can be used, for example, for generating passwords. Other random_ resources
may not suffice due to policy reasons, such as requiring special characters.
Since this provider does not interact with external services, it's safe
to run its tests as part of a normal unit testing pass, rather than
requiring acceptance testing mode.
@apparentlymart apparentlymart merged commit fbd03f6 into hashicorp:master Sep 7, 2017
@apparentlymart
Copy link
Contributor

Thanks again for this, @ryan-dyer-sp!

Before merging I made a couple tweaks:

  • I renamed the computed attribute to result for consistency with some of the other resources.
  • The linter complained about the variable names with underscores so I just renamed them to use camelcase to conform with usual Go style.

I also added the commit I made to make the tests run as unit tests, which means that they get included in the travis runs we do here for PRs. That's not directly related to your change, but it made it easier for me to verify it before merging.

@ryan-dyer-sp
Copy link
Author

@apparentlymart Awesome. When/How is this made available? I've built it locally and using that for now, but others on team use macs (ubuntu here) and so we're dealing with how to distribute multiple compiled versions.

@ryan-dyer-sp ryan-dyer-sp deleted the random_string branch November 30, 2018 17:11
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants