-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
resource/aws_lightsail_instance: Add validation for instance name #8667
Conversation
Type: schema.TypeString, | ||
Required: true, | ||
ForceNew: true, | ||
ValidateFunc: validation.StringMatch(regexp.MustCompile(`^[[:alnum:]]{2}[[:alnum:]_\-]*$`), "must be at least 2 characters long and contain only alphanumerics, hyphens, and dashes"), |
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.
Is a-123
a valid name? I believe this regex wouldn't match that.
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.
My personal preference for more complex validation functions nowadays is utilizing validation.All()
so multiple restrictions can be broken down into easier functions. Certainly not a requirement though, just thought I would mention!
ValidateFunc: validation.StringMatch(regexp.MustCompile(`^[[:alnum:]]{2}[[:alnum:]_\-]*$`), "must be at least 2 characters long and contain only alphanumerics, hyphens, and dashes"), | |
ValidateFunc: validation.All( | |
validation.StringLenBetween(3, 255), | |
validation.StringMatch(regexp.MustCompile(`^[a-zA-Z0-9_-]*$`), "must contain only alphanumeric characters, underscores, and hyphens"), | |
), |
My hope is that we'll eventually find emergent patterns with StringMatch
and create wrappers/constants to simplify this even further.
If you are curious, I was able to coerce the max length error with acctest.RandString()
😄
lightsailName := acctest.RandString(1025)
--- FAIL: TestAccAWSLightsailInstance_basic (7.34s)
testing.go:568: Step 0 error: errors during apply:
Error: InvalidInputException: 81gjehvhlkp8zb809x3bsb7zuoxnqtcxwu4hqdt8zlbhqyo8qockfumus0jgr6cyae4unytj3ms7okj2yo6z2ksnyw4ws9bv2n0tirk9prhfmuntcjr9z1eazufbtlttrxwd7owds2fwt3pxnxaanwwhhrfmf339apqh8dtci4zg99ocmzarxjbhhhymtndtof0u0101s3rfevonb0br1196srrpb82cir8fg8unxjgy8ntzflwq7xwkpbunr33vh76jj4voe3he36rz20v9iktvqh63ha2wj87pmndpp0uyvefb9j1s89q9zfv3aifnywc0el66yx4czfzzyc9xaq6rdebhczpe7bat6apsdmazhqt139ql7rhtlauld3q4df3d9cncf3fad7o8nrjlzy3ow6mxmuwgi8bbg1fwr27trf8imx9yroh1f82psmywon3bai1irb93krgb3lu73zuj0z1mnb0sofmq7yayid41rypqpkxwrbs3od64ia7j0s34z941f604wd606d6huvxnvgcp6nxpjqvw0mgyjw9ndrmnczkmfa06d0sb64yhm6hrk3wbako7prgarub1ycnlsfmrx1kas6xd69xu26p0va3o1eot7f0nkec2vjftycs713p8hztnnrfjwf29xlx7gomx6r80duoxbjkb1rgek8om8ex3xcg3wy2p6s0dwpuhg1oakyqrmjj0j7n6gqvs3wpsshbycq1rxr3dcjylhfbqgorzqt6ugz1b8map763v0cd0bqjigadwlemsf1gd9gvlu0ax7wbamlnshqxgowebhllopcjbhdcml76v6mhqld01gw231p8349nrr7fcd8aidwd0kn4xwl34f0jh0s4eerh7z1vr1bdwe0xpzctdjaugrhgy81xr3q7r6j2sd3ttvsh3jyj7xuty8nqd2vhxgcj1qke0s2cihdoxnkau4quwsx0a72w8odesbolcmhbtm3qmjday97t0y99s27f0sm2rt4i3mpjn7gmkp is not a valid resource name. Length exceeds maximum of 255
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.
@paddycarver good catch 🙇♂️
@bflad no one said anything about a limit 😜 Good catch as well. I'll address both cases and take a look at validation.All
. Although, I do enjoy looking at a complex validation every now and then.
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.
I actually like the validation.All
approach as it allows for separated messaging ✔️
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.
My concern is resolved, but I defer to @bflad.
0968203
to
efda7a1
Compare
@paddycarver @bflad if you could take another look that would be great. I opted to double up on the error message for each validation so that a user can quickly see what is considered a valid name as opposed to see the error in parts. |
@nywilken sorry for moving this one back -- I would like to show you some things here. 😄 |
👍 looking forward to it, thanks. |
``` --- PASS: TestAccAWSLightsailInstance_Name (99.68s) ```
efda7a1
to
c53bbd2
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.
Looks good! 🚀 Please note I'm going to merge this along with #9273 since they will likely have merge conflicts then rebase my #9080 afterwards.
--- PASS: TestAccAWSLightsailInstance_euRegion (52.79s)
--- PASS: TestAccAWSLightsailInstance_disapear (56.80s)
--- PASS: TestAccAWSLightsailInstance_basic (58.24s)
--- PASS: TestAccAWSLightsailInstance_Name (112.66s)
This has been released in version 2.19.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
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! |
related to #8647
Community Note
Fixes #0000
Release note for CHANGELOG:
Output from acceptance testing: