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

Bug - Fix regex for aws_transfer_user #8304

Merged
merged 4 commits into from
Apr 24, 2019

Conversation

jukie
Copy link
Contributor

@jukie jukie commented Apr 12, 2019

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

Fixes #8065

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

...

@ghost 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 jukie changed the title Bugfix aws transfer user Bug - Fix regex for aws_transfer_user Apr 13, 2019
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@jukie jukie Apr 14, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Start with hyphen
  2. Less than 3 characters
  3. More than 32 characters
  4. Regex match

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood, I'll take care of that now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@charlyx could you take another look at this?

@ghost 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 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 bflad added this to the v2.8.0 milestone Apr 24, 2019
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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

func TestAccAWSTransferUserName_validation(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"),
),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bflad bflad merged commit 7345bd8 into hashicorp:master Apr 24, 2019
bflad added a commit that referenced this pull request Apr 24, 2019
bflad added a commit that referenced this pull request Apr 24, 2019
bflad added a commit that referenced this pull request Apr 24, 2019
@nywilken
Copy link
Contributor

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
Copy link

ghost commented Mar 30, 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 30, 2020
@justinretzolk justinretzolk added the partner Contribution from a partner. label May 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. partner Contribution from a partner. service/transfer Issues and PRs that pertain to the transfer service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong aws_transfer_user user_name validation
5 participants