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

feat: initial draft implementation for string exclusions #645

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ramonvermeulen
Copy link

@ramonvermeulen ramonvermeulen commented Jan 27, 2025

Added a draft implementation for issue: #630

Adds two properties to the random_string as well as random_password resource:

  • exclusions Set of String
  • exclusions_case_sensitive Boolean (default false)

The goal is to provide a set of exclusions for the generated string (or password), if the generated string matches any of the exclusions it will regenerate the string until it has no match. If preferred I can change the exclusions to a configurable set of Regex patterns (might offer more flexibility).

Because it is my first time contributing to the terraform-provider-random, I'd love to first get a review on the following questions before I proceed with the implementation (e.g. going deep on testing and finalizing the implementation).

  1. Is this functionality that is desired in the terraform random provider? or would it be considered out-of-scope for the provider.
  2. Should exclusions be changed to a set of Regex instead of a plain string, I think it might offer more flexibility. Also because case sensitivity is already build-in to regex, we could work with only the exclusions argument instead of having a separate configuration option for case sensitivity.
  3. Would this implementation introduce a breaking change? As far I can see it is added functionality, e.g. adding configurable arguments and not modifying or removing existing arguments, so could be put into a minor release. However, I don't know the terraform-provider-random good enough yet to overview all the implications.

Copy link

hashicorp-cla-app bot commented Jan 27, 2025

CLA assistant check
All committers have signed the CLA.

@ramonvermeulen
Copy link
Author

ramonvermeulen commented Jan 28, 2025

Hi @austinvalle,

I’ve been following the activity in the repository and noticed that you've been one of the most active maintainers over the past year. Before going in deeper on the implementation and writing good tests, I was wondering if you might have some time to review my proposal above. I’d really appreciate your feedback.

@austinvalle
Copy link
Member

Hey @ramonvermeulen 👋🏻 , thanks for the PR!

I do have some things I'd like to hear more about with the use-case before proceeding with the PR, which I've added in a comment over on the issue if you'd like to add your thoughts: #630 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants