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

provider/aws: Add validation for aws_db_instance.identifier #2516

Merged

Conversation

radeksimko
Copy link
Member

http://docs.aws.amazon.com/cli/latest/reference/rds/create-db-instance.html

screen shot 2015-06-26 at 14 59 57

I did not bother with the character limit since it's engine-specific and it's not quite easy yet to do validation based on other fields.

@radeksimko radeksimko force-pushed the f-aws-db-instance-id-validation branch from 6ba201f to 3a98525 Compare June 26, 2015 14:06
@phinze
Copy link
Contributor

phinze commented Jun 26, 2015

LGTM

radeksimko added a commit that referenced this pull request Jun 26, 2015
provider/aws: Add validation for aws_db_instance.identifier
@radeksimko radeksimko merged commit 4230a52 into hashicorp:master Jun 26, 2015
@radeksimko radeksimko deleted the f-aws-db-instance-id-validation branch June 26, 2015 14:30
@@ -74,6 +74,26 @@ func resourceAwsDbInstance() *schema.Resource {
Type: schema.TypeString,
Required: true,
ForceNew: true,
ValidateFunc: func(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
if !regexp.MustCompile(`^[0-9a-z-]$`).MatchString(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be [0-9a-z-]$ instead of ^[0-9a-z-]$ ?

foobarbaz-test-terraform-test2 is failing:

http://rubular.com/r/ixQHBNXPiV vs http://rubular.com/r/difJw4AHVJ

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, this should actually be ^[0-9a-z-]+$ (i.e. quantifier + is missing, so it's checking a single character only), thanks for noticing, fix is on the way.

@radeksimko
Copy link
Member Author

@phinze Per discussion in 8fa96d2 it may be necessary to loosen the validation here too.

I admit I was not running acceptance tests for RDS since these take ~30mins+, but considering the impact (broken tests), I feel I should have done that.

@radeksimko
Copy link
Member Author

In fact there doesn't seem to be any logic for lowercasing the identifier, so it should be 🆗

@phinze
Copy link
Contributor

phinze commented Jun 30, 2015

@radeksimko yeah - the acctests are passing here so we're good to leave it

@ghost
Copy link

ghost commented May 1, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants