-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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: Added RDS event subscriptions #6367
provider/aws: Added RDS event subscriptions #6367
Conversation
return err | ||
} | ||
|
||
// Set tags, but theyre not in the response |
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.
Tags can be queried via separate API, see how we do it for DB instances:
https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_db_instance.go#L650-L675
The same API seems to be used for many resources, see http://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/USER_Tagging.html#USER_Tagging.ARN
Also keep in mind we always try to avoid extra API requests for account ID like you can see in https://github.com/hashicorp/terraform/blob/master/builtin/providers/aws/resource_aws_db_instance.go#L975 where it wasn't avoidable. It is avoidable for RDS Event Subscriptions though since you have sub.CustomerAwsId
available. 😺
@bigkraig I left you some comments there, hopefully it helps you in moving this from WIP to "ready for final review & merge" 😉 The most important ones are regarding tags & retry logic for updates + acceptance tests. Also in terms of convenience I think that user would expect the subscription to be |
@radeksimko thanks for all of the work! I'll get all of these looked at asap, a lot of helpful information in here. |
@radeksimko We use federated accounts here which causes iam.GetUser to fail and that means I am unable to generate ARNs for RDS resources. This becomes a problem for tag testing, which requires the ARN for making changes. |
I was able to fix the ARN problem by updating the aws-sdk-go library, but my branch was off an older HEAD so it looks like it has gone into merge hell. I may rebase/rework this branch if/when #6385 is merged |
Thanks for the other PRs, but I don't think any of them are blockers for this PR, specifically you don't need to be querying API for account ID for this resource, as I mentioned in one of my comments:
|
@radeksimko I don't think its avoidable in resourceAwsDbEventSubscriptionUpdate. I can get the account id from ModifyEventSubscription, but that only happens if something has changed. If only the tags are different I wouldn't have made that call. |
You can get it in |
cb9873b
to
afa380e
Compare
@radeksimko done! is this a wrap? |
a78b496
to
de4725e
Compare
_, err = stateConf.WaitForState() | ||
if err != nil { | ||
return fmt.Errorf("Modifying RDS Event Subscription %s failed: %s", d.Id(), err) | ||
} |
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 should also be calling setPartial
for all fields that have been successfully modified here - i.e. enabled
, event_categories
, sns_topic
and source_type
, otherwise if an error occurs when setting tags below, these changes might be lost, or rather treated as unsaved.
I left you some (hopefully last before merge) comments there, otherwise this is looking pretty good. Also I appreciate you squashed the changes into 1 commit 👍 |
de4725e
to
a57a573
Compare
@radeksimko Got everything addressed. I have had some problems running the update acceptance test, it deletes before it runs the update. I'm not sure what I am doing wrong here, I looked at TestAccAWSRDSCluster_backupsUpdate to compare and I am missing whatever is different. I was able to verify by running terraform against my own configurations, so its just a matter of fixing the test at this point. |
// set tags | ||
conn := meta.(*AWSClient).rdsconn | ||
arn := buildRDSEventSubscriptionARN(d.Get("customer_aws_id").(string), d.Id(), meta.(*AWSClient).region) | ||
d.Set("arn", arn) |
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.
There is no arn
field defined in the schema. I don't think it necessarily needs to be, but in such case we shouldn't be trying to save the ARN to state.
Except the ARN, there's just one last thing - probably misunderstanding of my previous comment: if d.HasChange("enabled") {
d.SetPartial("enabled") the Better use of that function would be something like this: if requestUpdate {
log.Printf("[DEBUG] RDS Event Subscription modification request: %#v", req)
...
// Wait, catching any errors
_, err = stateConf.WaitForState()
if err != nil {
return fmt.Errorf("Modifying RDS Event Subscription %s failed: %s", d.Id(), err)
}
d.SetPartial("event_categories")
d.SetPartial("enabled")
d.SetPartial("sns_topic")
d.SetPartial("...another field...")
}
... i.e. only call See how we do this in I will try to run the acceptance tests to see what the issue is there. |
So the acceptance tests need some tuning - possibly a copy-paste error: diff --git a/builtin/providers/aws/resource_aws_db_event_subscription_test.go b/builtin/providers/aws/resource_aws_db_event_subscription_test.go
index 5aa5f51..26de043 100644
--- a/builtin/providers/aws/resource_aws_db_event_subscription_test.go
+++ b/builtin/providers/aws/resource_aws_db_event_subscription_test.go
@@ -24,13 +24,13 @@ func TestAccAWSDBEventSubscription_basicUpdate(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBEventSubscriptionExists("aws_db_event_subscription.bar", &v),
resource.TestCheckResourceAttr(
- "aws_db_instance.bar", "enabled", "true"),
+ "aws_db_event_subscription.bar", "enabled", "true"),
resource.TestCheckResourceAttr(
- "aws_db_instance.bar", "source_type", "db-instance"),
+ "aws_db_event_subscription.bar", "source_type", "db-instance"),
resource.TestCheckResourceAttr(
- "aws_db_instance.bar", "name", "baz"),
+ "aws_db_event_subscription.bar", "name", "tf-acc-test-rds-event-subs"),
resource.TestCheckResourceAttr(
- "aws_db_instance.bar", "tags.Name", "name"),
+ "aws_db_event_subscription.bar", "tags.Name", "name"),
),
},
resource.TestStep{
@@ -38,11 +38,11 @@ func TestAccAWSDBEventSubscription_basicUpdate(t *testing.T) {
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSDBEventSubscriptionExists("aws_db_event_subscription.bar", &v),
resource.TestCheckResourceAttr(
- "aws_db_instance.bar", "enabled", "false"),
+ "aws_db_event_subscription.bar", "enabled", "false"),
resource.TestCheckResourceAttr(
- "aws_db_instance.bar", "source_type", "db-parameter-group"),
+ "aws_db_event_subscription.bar", "source_type", "db-parameter-group"),
resource.TestCheckResourceAttr(
- "aws_db_instance.bar", "tags.Name", "new-name"),
+ "aws_db_event_subscription.bar", "tags.Name", "new-name"),
),
},
}, then it will work as expected:
|
a57a573
to
96ff958
Compare
@radeksimko Done!!! Thanks for all of the help with this. |
Thanks @bigkraig for finishing this. I reran acceptance tests and re-checked the code and it LGTM. |
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. |
Addresses #5797, I still need to add more test cases and look further into the tags. You can definitely tag these subscriptions based on the AWS Console, you can see them in there, but I haven't seen how to retrieve them from the API.