-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
resource/db_instance: set SourceRegion based on source_db ARN #3795
Conversation
@catsby I believe you have done most of the |
Requires aws/aws-sdk-go#1847 to land. |
The AWS SDK for Go's aws/aws-sdk-go#1847 PR has been replaced with aws/aws-sdk-go#2631 adding the outstanding unit tests and will be merged into master once reviewed. |
aws/aws-sdk-go#2631 just landed, so this is good to go. |
Any chance this could be merged in? maintaining a fork because of pending bug fixes is rather painful. |
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 @omeid 👋 Thank you for submitting this. For starters here, the maintainers will likely be looking for:
- Output showing that all existing replica acceptance testing is passing
- A covering acceptance test that fails before the change and passes after the change. There's a section in the Contribution Guide for how to add cross-region testing: https://github.com/terraform-providers/terraform-provider-aws/blob/master/.github/CONTRIBUTING.md#writing-and-running-cross-region-acceptance-tests
We will want to ensure that Terraform shows no difference if a replica_source_db
ARN is provided and that no regressions are introduced with this change. Please reach out if you have any questions or do not have time to implement the testing.
if arnParts := strings.Split(v.(string), ":"); len(arnParts) >= 4 { | ||
opts.SourceRegion = aws.String(arnParts[3]) | ||
} |
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 check would be safer and easier to understand using the AWS Go SDK provided ARN handlers arn.IsARN()
and arn.Parse()
, e.g.
if arn.IsARN(v.(string)) {
replicaSourceDbArn, err := arn.Parse(v.(string))
if err != nil {
return fmt.Errorf("error parsing replica_source_db as ARN: %s", err)
}
opts.SourceRegion = aws.String(replicaSourceDbArn.Region)
}
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.
Okay, I can make that change. Just shows how long this pr has been sitting here. (;
Hi again 👋 Since we haven't heard back, we are going to close this pull request for now. If anyone is still interested in this change, please submit a new pull request and we will take a fresh look. Thanks. |
I am still interested in fixing this. However, since we have been waiting for this for almost two years before we heard anything from Terrraform, we can't prioritize putting time into this. Also, I personally find it a bit distasteful that you leave a contributor waiting for an answer for two years but then promptly close the issue once you don't get a reply in a month or so over holiday season. |
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! |
As Per AmazonRDS/API/CreateDBInstanceReadReplica docs
This commit sets the Read Replica Source Region based on the
replicate_source_db arn.
kms_key_id is managed as usual.
Fixes: #518
Related: #3337, #865