-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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 AWS DMS (data migration service) resources #11122
Conversation
OK. This PR is ready for review! Last test case run below. Thanks.
|
OMG brilliant! Thank you so much for this PR it rocks big time! (we were about to start to write some scripts but thanks to you we dont have to) - cant wait for the release. |
Hi @jzbruno Thanks so much for the work here - please can you rebase this against master? This will help us to get on the review Thanks Paul |
@stack72 Sure. Fixed the merge conflicts. |
Just a FYI, while testing this, I got the following results:
|
Sorry, I forgot that the role is only created if you add your first DMS resource from the AWS console. I will add the role to the test tomorrow morning. |
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.
Hi @jzbruno
Thanks for the work here - as i noted last night, this will need the role added to the tests. Generally, I think there will need to be a few changes.
-
We shouldn't have empty strings in our test setup. I believe this is because we are setting these values as part of the request code rather than checking to see if it exists before passing it
-
Some of the comments suggest we should have ValidateFuncs - these are not vital for the merge but we should, if possible, follow up and add them
Hope this is all ok
Thanks for the work again
Paul
"engine_name": { | ||
Type: schema.TypeString, | ||
Required: true, | ||
// Valid Values: mysql, oracle, postgres, mariadb, aurora, redshift, sybase, sqlserver |
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 think we can add a validate func here - not crucial for merging :)
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 added the validate function. This probably won't change very often.
For something like replication_instance_type where new instance types may be added often over the course of the year, should we validate those too?
Port: aws.Int64(int64(d.Get("port").(int))), | ||
ServerName: aws.String(d.Get("server_name").(string)), | ||
Tags: dmsTagsFromMap(d.Get("tags").(map[string]interface{})), | ||
Username: aws.String(d.Get("username").(string)), |
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.
This is optional - therefore, what happens if we don't set this - AWS will cope with the empty string?
EndpointIdentifier: aws.String(d.Get("endpoint_id").(string)), | ||
EndpointType: aws.String(d.Get("endpoint_type").(string)), | ||
EngineName: aws.String(d.Get("engine_name").(string)), | ||
Password: aws.String(d.Get("password").(string)), |
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.
Optional attribute - what happens if we don't pass this?
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.
The API allows database_name, username, and password to be empty strings. I changed these to be required since it doesn't make much sense to set these to an empty string.
"github.com/hashicorp/terraform/helper/validation" | ||
) | ||
|
||
func resourceAwsDmsEndpoint() *schema.Resource { |
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.
Do we need to add wait for create or update? Is this eventually consistent?
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.
The endpoint create and update are eventually consistent. I didn't put a wait on these because the update is very fast and it is hard to properly tell difference between the resource going from status of available to modifying to available vs. never leaving available.
Any suggestions on the proper handling of this?
func dmsEndpointConfig(randId string) string { | ||
return fmt.Sprintf(` | ||
resource "aws_dms_endpoint" "dms_endpoint" { | ||
database_name = "" |
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.
why would we pass an empty database name?
}, | ||
"replication_instance_id": { | ||
Type: schema.TypeString, | ||
Required: true, |
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 think we should turn these constraints into a validate func
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 have added validate functions to all the DMS resource IDs.
replication_subnet_group_id = "${aws_dms_replication_subnet_group.dms_replication_subnet_group.replication_subnet_group_id}" | ||
tags { | ||
Name = "tf-test-dms-replication-instance-%[1]s" | ||
Update = "" |
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.
We shouldn't pass these as empty strings - empty strings mean we should omit them
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.
Fixed.
func resourceAwsDmsReplicationSubnetGroupUpdate(d *schema.ResourceData, meta interface{}) error { | ||
conn := meta.(*AWSClient).dmsconn | ||
|
||
request := &dms.ModifyReplicationSubnetGroupInput{ |
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.
Do we have to pass everything as part of the Update request or should we pass only what we need to change?
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.
Everything but the ReplicationSubnetGroupDescription is required. I will update.
"cdc_start_time": { | ||
Type: schema.TypeInt, | ||
Optional: true, | ||
// Unix timestamp 1484346880 |
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.
Comment?
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.
Do you just want me to add context? The API documentation was unclear what type of timestamp this should be.
func resourceAwsDmsReplicationTaskCreate(d *schema.ResourceData, meta interface{}) error { | ||
conn := meta.(*AWSClient).dmsconn | ||
|
||
request := &dms.CreateReplicationTaskInput{ |
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 like we still have an issue of passing empty strings - is this ok on the AWS side? We usually follow the pattern of if v_, ok := d.GetOk....
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 have looked these situations over for each resource. The problem is that the AWS API does allow not only empty strings for a lot of it's inputs but also they are optional. Since GetOk sets ok to false when an input is the zero value of it's type, we cannot know if the input was omitted from the tf config or set to the valid zero value.
Any suggestions are welcome here, but since the API accepts the input I think we are ok.
…endpoint engine_name.
The acceptance tests take a long time to run. Is there a way to create the common resources and use them in multiple test steps? This would help a lot with the aws_dms_replication_task test which requires all of the other resources to exist. And is possible to combine the import tests with these? |
Hi @jzbruno Sorry! Any chance of a rebase on this? :) P. |
Ok. I fixed the merge conflicts. Sorry, I missed that the certificate resource could be managed through the API. I have added that too. This should be ready for review again. |
Thanks @jzbruno :) Looking at this again now! P. |
Hi @jzbruno This is looking much - I am getting 1 error when I run the tests:
I am going to bump that timeout - to 20 minutes just to make sure that all is well :) As soon as I do that and get a successful test, we are good to merge Thanks again Paul |
Hey @jzbruno so I am having real issues getting these tests to work - I am getting timeouts when the VPC that is being spun up is trying to be destroyed. Did you manage to get them all working successfully? Paul |
Certificate tests pass:
|
Subnet tests pass:
|
Endpoint tests pass:
|
Replication Instance tests do not work for me...
|
Ok. I will take a look later today. |
nps! :) |
… Combine import tests with basic tests to cut down runtime.
@stack72 I think I have the tests fixed. They needed explicit dependencies set to ensure the subnets were deleted in the correct order. Also, I combined the import tests with the basic tests. This saves about 30 mins on the runtime.
|
Thanks so much for all the work here - you have worked through all the issues and made everything work :)
|
* Add aws dms vendoring * Add aws dms endpoint resource * Add aws dms replication instance resource * Add aws dms replication subnet group resource * Add aws dms replication task resource * Fix aws dms resource go vet errors * Review fixes: Add id validators for all resources. Add validator for endpoint engine_name. * Add aws dms resources to importability list * Review fixes: Add aws dms iam role dependencies to test cases * Review fixes: Adjustments for handling input values * Add aws dms replication subnet group tagging * Fix aws dms subnet group doesn't use standard error for resource not found * Missed update of aws dms vendored version * Add aws dms certificate resource * Update aws dms resources to force new for immutable attributes * Fix tests failing on subnet deletion by adding explicit dependencies. Combine import tests with basic tests to cut down runtime.
Thanks for the review and help! |
…rp#11122) * Add aws dms vendoring * Add aws dms endpoint resource * Add aws dms replication instance resource * Add aws dms replication subnet group resource * Add aws dms replication task resource * Fix aws dms resource go vet errors * Review fixes: Add id validators for all resources. Add validator for endpoint engine_name. * Add aws dms resources to importability list * Review fixes: Add aws dms iam role dependencies to test cases * Review fixes: Adjustments for handling input values * Add aws dms replication subnet group tagging * Fix aws dms subnet group doesn't use standard error for resource not found * Missed update of aws dms vendored version * Add aws dms certificate resource * Update aws dms resources to force new for immutable attributes * Fix tests failing on subnet deletion by adding explicit dependencies. Combine import tests with basic tests to cut down runtime.
…rp#11122) * Add aws dms vendoring * Add aws dms endpoint resource * Add aws dms replication instance resource * Add aws dms replication subnet group resource * Add aws dms replication task resource * Fix aws dms resource go vet errors * Review fixes: Add id validators for all resources. Add validator for endpoint engine_name. * Add aws dms resources to importability list * Review fixes: Add aws dms iam role dependencies to test cases * Review fixes: Adjustments for handling input values * Add aws dms replication subnet group tagging * Fix aws dms subnet group doesn't use standard error for resource not found * Missed update of aws dms vendored version * Add aws dms certificate resource * Update aws dms resources to force new for immutable attributes * Fix tests failing on subnet deletion by adding explicit dependencies. Combine import tests with basic tests to cut down runtime.
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. |
This closes #8973.
This is WIP for adding AWS DMS resource support. A few people were asking for it so I am putting this up to allow; tracking progress, giving early feedback, or collaborating.
I will be working on finishing this week.