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.

Sign up for GitHub

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

Jump to bottom

Refactor Out randomString() #10040

Closed
9 of 13 tasks
bflad opened this issue Sep 8, 2019 · 4 comments · Fixed by #10478
Closed
9 of 13 tasks

Refactor Out randomString() #10040

bflad opened this issue Sep 8, 2019 · 4 comments · Fixed by #10478
Labels
service/elb Issues and PRs that pertain to the elb service. service/iam Issues and PRs that pertain to the iam service. service/rds Issues and PRs that pertain to the rds service. service/redshift Issues and PRs that pertain to the redshift service. service/worklink Issues and PRs that pertain to the worklink service. technical-debt Addresses areas of the codebase that need refactoring or redesign. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Milestone
v2.33.0

Comments

@bflad
Copy link
Contributor

bflad commented Sep 8, 2019 •
edited
Loading

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Currently there is a function named randomString(), which generates a random lowercase alphabetical string:

https://github.com/terraform-providers/terraform-provider-aws/blob/b4efb0be0ac4315f2f4ac52955e58fc32ffe05d5/aws/resource_aws_db_parameter_group_test.go#L624-L632

Most usage of this function should be replaced with acctest.RandStringFromCharSet(len, acctest.CharSetAlpha), which is functionally equivalent and part of the Terraform Provider SDK.

Affected Resource(s)

  • aws/resource_aws_db_parameter_group_test.go
  • aws/resource_aws_elb_test.go
  • aws/resource_aws_iam_role_policy_test.go
  • aws/resource_aws_iam_role_test.go
  • aws/resource_aws_iam_user_policy_test.go
  • aws/resource_aws_iam_user_test.go
  • aws/resource_aws_redshift_cluster_test.go
  • aws/resource_aws_redshift_parameter_group_test.go
  • aws/resource_aws_redshift_security_group_test.go
  • aws/resource_aws_redshift_subnet_group_test.go
  • aws/resource_aws_worklink_fleet_test.go
  • aws/resource_aws_worklink_website_certificate_authority_association_test.go
  • aws/validators_test.go (see also Simplify Validation Functions in validators.go with Terraform helper/validation Functions #8424)
@bflad bflad added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. service/iam Issues and PRs that pertain to the iam service. service/rds Issues and PRs that pertain to the rds service. service/elb Issues and PRs that pertain to the elb service. service/redshift Issues and PRs that pertain to the redshift service. technical-debt Addresses areas of the codebase that need refactoring or redesign. service/worklink Issues and PRs that pertain to the worklink service. labels Sep 8, 2019
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Sep 8, 2019
@bflad bflad removed the needs-triage Waiting for first response or review from a maintainer. label Sep 8, 2019
This was referenced Sep 18, 2019
Merged
Merged
Merged
Merged
This was referenced Oct 1, 2019
Closed
Merged
Merged
This was referenced Oct 2, 2019
Closed
Closed
@jukie
Copy link
Contributor

jukie commented Oct 10, 2019 •
edited
Loading

Are we ready to make changes here now @bflad? https://github.com/terraform-providers/terraform-provider-aws/blob/b4efb0be0ac4315f2f4ac52955e58fc32ffe05d5/aws/resource_aws_db_parameter_group_test.go#L624-L632

@jukie
Copy link
Contributor

jukie commented Oct 10, 2019

Scratch that, looks like aws/validators_test.go is the last one. Considering there doesn't seem to be a consensus for #8424 would you prefer to refactor aws/validators_test.go so we can close this issue in the meantime?

@bflad
Copy link
Contributor Author

bflad commented Oct 11, 2019

@jukie let's refactor aws/validators_test.go for now and feel free to remove randomString() itself then -- thanks so much for helping out with this issue!

@jukie jukie mentioned this issue Oct 11, 2019
Merged
@bflad bflad added this to the v2.33.0 milestone Oct 11, 2019
@ghost
Copy link

ghost commented Mar 29, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 29, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
service/elb Issues and PRs that pertain to the elb service. service/iam Issues and PRs that pertain to the iam service. service/rds Issues and PRs that pertain to the rds service. service/redshift Issues and PRs that pertain to the redshift service. service/worklink Issues and PRs that pertain to the worklink service. technical-debt Addresses areas of the codebase that need refactoring or redesign. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v2.33.0
2 participants
@bflad @jukie