Bug - Fix regex for aws_transfer_user #8304

jukie · 2019-04-12T16:42:45Z

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Changes proposed in this pull request:

Allow uppercase, hyphens, and underscores in validation regex
Add condition for user_name beginning with a hyphen
Split user_name length validation to separate conditionals

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSTransferUserName_validation'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSTransferUserName_validation -timeout 120m
=== RUN   TestAccAWSTransferUserName_validation
=== PAUSE TestAccAWSTransferUserName_validation
=== CONT  TestAccAWSTransferUserName_validation
--- PASS: TestAccAWSTransferUserName_validation (4.36s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	4.466s

...

charlyx · 2019-04-14T20:35:45Z

aws/validators.go

+	if regexp.MustCompile(`[^0-9a-zA-Z_-]`).MatchString(value) {
+		errors = append(errors, fmt.Errorf("%q can only contain alphanumeric characters, underscores, and hyphens", k))
+	}
+	if len(value) < 3 {


I would start by checking the length first.
Then the patterns: maybe "cannot begin with a hyphen" first?

I'll update it now, does this order sound good?

Start with hyphen

Less than 3 characters

More than 32 characters

Regex match

I believe that if the length is not good there is no point checking for characters so I'd start with it.
Otherwise, you might just use strings.HasPrefix for the "start with hypen" rule.

Would you rather have a single conditional? I was only concerned about the longer output but if a single is preferred I'll keep that in.

charlyx · 2019-04-14T20:44:55Z

aws/resource_aws_transfer_user_test.go

+			{
+				Config:      testAccAWSTransferUserName_validation("-abcdef"),
+				ExpectError: regexp.MustCompile(`"user_name" cannot begin with a hyphen`),
+			},


That's cool! You never test that your validation function itself "passes" though.
You might want to add that step.

Not sure I understand, is that not what I had already done via below?

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSTransferUserName_validation' ==> Checking that code complies with gofmt requirements... TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSTransferUserName_validation -timeout 120m === RUN TestAccAWSTransferUserName_validation === PAUSE TestAccAWSTransferUserName_validation === CONT TestAccAWSTransferUserName_validation --- PASS: TestAccAWSTransferUserName_validation (1.64s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 1.681s ...

I'll be sure to add whatever else is required.

What I meant is that you've tested all the errors but you have no test saying that the name is correct.
They're all expecting errors.

Understood, I'll take care of that now.

@charlyx could you take another look at this?

bflad

LGTM, thanks @jukie 🚀 Will adjust the new acceptance test name on merge for consistency.

Output from acceptance testing:

--- PASS: TestAccAWSTransferUser_UserName_Validation (3.98s)
--- PASS: TestAccAWSTransferUser_disappears (9.89s)
--- PASS: TestAccAWSTransferUser_basic (11.63s)
--- PASS: TestAccAWSTransferUser_modifyWithOptions (26.39s)

bflad · 2019-04-24T13:42:04Z

aws/resource_aws_transfer_user_test.go

@@ -125,6 +125,37 @@ func TestAccAWSTransferUser_disappears(t *testing.T) {
 	})
 }

+func TestAccAWSTransferUserName_validation(t *testing.T) {


Nit: The acceptance test naming format should match: TestAccAWSTransferUser_XXX so all testing for the resource can be run with the same prefix and underscore, e.g. TestAccAWSTransferUser_UserName_Validation

Otherwise we have this situation (note lack of this new test being run):

$ TF_ACC=1 go test ./aws -v -timeout 120m -parallel 20 -run='TestAccAWSTransferUser_' === RUN TestAccAWSTransferUser_basic === PAUSE TestAccAWSTransferUser_basic === RUN TestAccAWSTransferUser_modifyWithOptions === PAUSE TestAccAWSTransferUser_modifyWithOptions === RUN TestAccAWSTransferUser_disappears === PAUSE TestAccAWSTransferUser_disappears === CONT TestAccAWSTransferUser_basic === CONT TestAccAWSTransferUser_modifyWithOptions === CONT TestAccAWSTransferUser_disappears --- PASS: TestAccAWSTransferUser_disappears (11.24s) --- PASS: TestAccAWSTransferUser_basic (12.36s) --- PASS: TestAccAWSTransferUser_modifyWithOptions (26.95s) PASS ok github.com/terraform-providers/terraform-provider-aws/aws 27.993s

Okay, I'll update and make another PR. Thanks for the review!

bflad · 2019-04-24T13:47:05Z

aws/validators.go

@@ -117,15 +117,10 @@ func validateTransferServerID(v interface{}, k string) (ws []string, errors []er

 func validateTransferUserName(v interface{}, k string) (ws []string, errors []error) {


Nit: In the future this can be simplified to use Terraform Provider SDK validation functions to show multiple validation errors easier:

ValidateFunc: validation.All( validation.StringLenBetween(3, 32), validation.StringMatch(regexp.MustCompile(`^[a-zA-Z0-9_]`), "must begin with alphanumeric or underscore character"), validation.StringMatch(regexp.MustCompile(`^[a-zA-Z0-9-_]+$`), "must contain only alphanumeric, hyphen, and underscore characters"), ),

Thanks, I wasn't aware of this. Is there a tech-debt effort to change other validation functions as well in validators.go? I'd be happy to help with that.

I thought I had created the technical debt issue for that, but apparently not. I'll try to get that created later today.

For now, this is a good start on a similar effort: #7906

Here's an initial proposal issue for fixing up validators.go: #8424 We should wait until other maintainers submit input to it before beginning any implementation.

…nce test name for consistency Reference: #8304 (review)

nywilken · 2019-04-27T01:41:48Z

This has been released in version 2.8.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-03-30T17:32:09Z

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!

jukie added 2 commits April 12, 2019 12:34

Change regex to allow hyphens, underscores, and uppercase characters

06325c4

Add test function for user_name validation

0261323

ghost added size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Apr 12, 2019

jukie mentioned this pull request Apr 12, 2019

Wrong aws_transfer_user user_name validation #8065

Closed

jukie changed the title ~~Bugfix aws transfer user~~ Bug - Fix regex for aws_transfer_user Apr 13, 2019

charlyx reviewed Apr 14, 2019

View reviewed changes

jukie added 2 commits April 14, 2019 18:45

add test case for valid username

0714b28

Use single regex and add valid resource configs in validation tests

7345bd8

ghost added size/M Managed by automation to categorize the size of a PR. and removed size/S Managed by automation to categorize the size of a PR. labels Apr 15, 2019

bflad added enhancement Requests to existing resources that expand the functionality or scope. service/transfer Issues and PRs that pertain to the transfer service. labels Apr 24, 2019

bflad added this to the v2.8.0 milestone Apr 24, 2019

bflad approved these changes Apr 24, 2019

View reviewed changes

bflad merged commit 7345bd8 into hashicorp:master Apr 24, 2019

bflad added a commit that referenced this pull request Apr 24, 2019

tests/resource/aws_transfer_user: Update user_name validation accepta…

79c7cd9

…nce test name for consistency Reference: #8304 (review)

bflad added a commit that referenced this pull request Apr 24, 2019

Update CHANGELOG for #8304

2f73d78

bflad added a commit that referenced this pull request Apr 24, 2019

Update CHANGELOG for #8304

fb8d81d

ghost locked and limited conversation to collaborators Mar 30, 2020

justinretzolk added the partner Contribution from a partner. label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug - Fix regex for aws_transfer_user #8304

Bug - Fix regex for aws_transfer_user #8304

jukie commented Apr 12, 2019 •

edited

Loading

charlyx Apr 14, 2019

jukie Apr 14, 2019 •

edited

Loading

charlyx Apr 14, 2019

jukie Apr 14, 2019

charlyx Apr 14, 2019

jukie Apr 14, 2019

charlyx Apr 14, 2019

jukie Apr 14, 2019

jukie Apr 15, 2019

bflad left a comment

bflad Apr 24, 2019

jukie Apr 24, 2019

bflad Apr 24, 2019

jukie Apr 24, 2019

bflad Apr 24, 2019

bflad Apr 24, 2019

nywilken commented Apr 27, 2019

ghost commented Mar 30, 2020

		@@ -117,15 +117,10 @@ func validateTransferServerID(v interface{}, k string) (ws []string, errors []er

		func validateTransferUserName(v interface{}, k string) (ws []string, errors []error) {

Bug - Fix regex for aws_transfer_user #8304

Bug - Fix regex for aws_transfer_user #8304

Conversation

jukie commented Apr 12, 2019 • edited Loading

Community Note

Choose a reason for hiding this comment

jukie Apr 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken commented Apr 27, 2019

ghost commented Mar 30, 2020

jukie commented Apr 12, 2019 •

edited

Loading

jukie Apr 14, 2019 •

edited

Loading