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

Adding GetRecommendedRdsInstanceType to RDS module #799

Merged
merged 8 commits into from
Mar 15, 2021

Conversation

alpacamybags118
Copy link
Contributor

PR to address #783

This PR adds functionality to the RDS module to get a recommended instance type based on region, database engine, and a given list of instances. It will pick the first available one from the list or throw an error if none exist.

It uses the AWS SDK function DescribeOrderableDBInstanceOptions to achieve this.

I modeled it a lot off the EC2 get recommended instance type functionality, but it is somewhat simplified since the underlying AWS method used has less options.

@alpacamybags118
Copy link
Contributor Author

Something that DescribeOrderableDBInstanceOptions exposes that I didn't think to add as an option is the DB engine-version. I believe there are cases where the version could make a difference on availability of an instance type, so i'll go ahead and add that. This code should still be reviewable as is, and I can make any feedback changes while I add that parameter.

@alpacamybags118
Copy link
Contributor Author

Updated the code to also add engine version to the check

Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

This is great, thank you! Just a couple minor NITs in the comments, and I can kick off tests.

modules/aws/rds.go Outdated Show resolved Hide resolved
modules/aws/rds.go Outdated Show resolved Hide resolved
alpacamybags118 and others added 2 commits March 10, 2021 22:01
Co-authored-by: Yevgeniy Brikman <[email protected]>
Co-authored-by: Yevgeniy Brikman <[email protected]>
@alpacamybags118
Copy link
Contributor Author

Done!

@brikis98
Copy link
Member

Thanks! Kicking off tests now.

@brikis98
Copy link
Member

Ha! Got a test failure related to the instance type for EC2 instances:

TestTerraformRemoteExecExample 2021-03-11T10:22:13Z logger.go:66: �[1m�[31mError: �[0m�[0m�[1mError launching source instance: Unsupported: The requested configuration is currently not supported. Please check the documentation for supported configurations.
TestTerraformRemoteExecExample 2021-03-11T10:22:13Z logger.go:66: 	status code: 400, request id: 4e020e26-7c45-4ce8-a161-290cd71622d1�[0m
TestTerraformRemoteExecExample 2021-03-11T10:22:13Z logger.go:66: 
TestTerraformRemoteExecExample 2021-03-11T10:22:13Z logger.go:66: �[0m  on main.tf line 26, in resource "aws_instance" "example_public":
TestTerraformRemoteExecExample 2021-03-11T10:22:13Z logger.go:66:   26: resource "aws_instance" "example_public" �[4m{�[0m
TestTerraformRemoteExecExample 2021-03-11T10:22:13Z logger.go:66: �[0m
TestTerraformRemoteExecExample 2021-03-11T10:22:13Z logger.go:66: �[0m�[0m
TestTerraformRemoteExecExample 2021-03-11T10:22:13Z retry.go:99: Returning due to fatal error: FatalError{Underlying: error while running command: exit status 1; �[31m

I think this is exactly the issue you're fixing for RDS here, but we need to use the GetRecommendedInstanceType for EC2 to solve it. Any chance you'd want to update that example/test accordingly?

The other test failure is unrelated; our GCP test account is hitting some quota issues that we're cleaning up separately.

@alpacamybags118
Copy link
Contributor Author

Yup I can update that test

@alpacamybags118
Copy link
Contributor Author

updated!

@brikis98
Copy link
Member

Thank you!

One note: I think you may have accidentally checked in a .tfvars file in the most recent commit. Could you remove that?

@alpacamybags118
Copy link
Contributor Author

Whoops, removed!

Copy link
Member

@brikis98 brikis98 left a comment

Choose a reason for hiding this comment

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

OK, great, thank you. I'll kick off tests again.

@brikis98
Copy link
Member

Tests passed. Merging now.

@brikis98 brikis98 merged commit ad51245 into gruntwork-io:master Mar 15, 2021
@brikis98
Copy link
Member

This was referenced Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants