provider/aws: Added RDS event subscriptions #6367

bigkraig · 2016-04-26T21:54:25Z

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.

radeksimko · 2016-04-27T14:29:01Z

builtin/providers/aws/resource_aws_db_event_subscription.go

+		return err
+	}
+
+	// Set tags, but theyre not in the response


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. 😺

radeksimko · 2016-04-27T15:23:08Z

@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.
It would be nice to have at least 1 more test which would be testing resource updates.

Also in terms of convenience I think that user would expect the subscription to be enabled = true by default, rather than disabled by default.

bigkraig · 2016-04-27T17:53:59Z

@radeksimko thanks for all of the work! I'll get all of these looked at asap, a lot of helpful information in here.

bigkraig · 2016-04-27T23:11:25Z

@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.

bigkraig · 2016-04-28T00:25:01Z

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

radeksimko · 2016-04-28T05:47:50Z

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:

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 · 2016-04-28T19:22:11Z

@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.

radeksimko · 2016-04-29T04:58:34Z

I can get the account id from ModifyEventSubscription

You can get it in resourceAwsDbEventSubscriptionRead and save it to the schema, just create a new field in the schema, e.g. customer_aws_id and make it Computed: true. Then you can leverage that account ID anywhere else, in other methods.

bigkraig · 2016-04-29T21:08:14Z

@radeksimko done! is this a wrap?

radeksimko · 2016-05-01T10:44:27Z

builtin/providers/aws/resource_aws_db_event_subscription.go

+		_, err = stateConf.WaitForState()
+		if err != nil {
+			return fmt.Errorf("Modifying RDS Event Subscription %s failed: %s", d.Id(), err)
+		}


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.

radeksimko · 2016-05-01T11:49:56Z

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 👍

bigkraig · 2016-05-02T21:19:19Z

@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.

radeksimko · 2016-05-03T12:16:35Z

builtin/providers/aws/resource_aws_db_event_subscription.go

+	// set tags
+	conn := meta.(*AWSClient).rdsconn
+	arn := buildRDSEventSubscriptionARN(d.Get("customer_aws_id").(string), d.Id(), meta.(*AWSClient).region)
+	d.Set("arn", arn)


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.

radeksimko · 2016-05-03T12:24:33Z

Except the ARN, there's just one last thing - probably misunderstanding of my previous comment:

if d.HasChange("enabled") {
    d.SetPartial("enabled")

the SetPartial function is (in the current version if this PR) confirming the value should be changed at a stage where you cannot be sure whether the update operation is actually going to be successful - i.e. too early.

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 SetPartial when you're sure that the field was actually updated successfully.

See how we do this in aws_elb for example.

I will try to run the acceptance tests to see what the issue is there.

radeksimko · 2016-05-04T09:17:10Z

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:

TF_ACC=1 go test ./builtin/providers/aws -v -run=AWSDBEventSubscription -timeout 120m
=== RUN   TestAccAWSDBEventSubscription_basicUpdate
--- PASS: TestAccAWSDBEventSubscription_basicUpdate (1455.29s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    1455.318s

bigkraig · 2016-05-05T00:56:53Z

@radeksimko Done!!! Thanks for all of the help with this.

radeksimko · 2016-05-05T10:14:21Z

Thanks @bigkraig for finishing this. I reran acceptance tests and re-checked the code and it LGTM.

ghost · 2020-04-25T02:35:37Z

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.

radeksimko added enhancement provider/aws new-resource labels Apr 27, 2016

radeksimko reviewed Apr 27, 2016
View reviewed changes

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Apr 27, 2016

bigkraig force-pushed the rds-event-subscription branch from cb9873b to afa380e Compare April 29, 2016 21:07

bigkraig changed the title ~~WIP: Adding RDS event subscriptions~~ Added RDS event subscriptions Apr 29, 2016

bigkraig force-pushed the rds-event-subscription branch 2 times, most recently from a78b496 to de4725e Compare April 29, 2016 23:13

radeksimko reviewed May 1, 2016
View reviewed changes

radeksimko self-assigned this May 1, 2016

bigkraig changed the title ~~Added RDS event subscriptions~~ provider/aws: Added RDS event subscriptions May 2, 2016

bigkraig force-pushed the rds-event-subscription branch from de4725e to a57a573 Compare May 2, 2016 21:17

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label May 3, 2016

radeksimko reviewed May 3, 2016
View reviewed changes

radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label May 4, 2016

Added RDS event subscriptions

96ff958

bigkraig force-pushed the rds-event-subscription branch from a57a573 to 96ff958 Compare May 5, 2016 00:40

radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label May 5, 2016

radeksimko merged commit 1f80ec4 into hashicorp:master May 5, 2016

bigkraig deleted the rds-event-subscription branch May 5, 2016 13:59

bigkraig added a commit to ticketmaster/terraform that referenced this pull request May 5, 2016

Added RDS event subscriptions (hashicorp#6367)

cd482a2

stack72 mentioned this pull request May 12, 2016

Missing RDS Event Subscription #5797

Closed

cristicalin pushed a commit to cristicalin/terraform that referenced this pull request May 24, 2016

Added RDS event subscriptions (hashicorp#6367)

d643772

johanneswuerbach mentioned this pull request Jul 26, 2016

provider/aws: db event subscription re-created when changing source_ids #7809

Closed

ghost locked and limited conversation to collaborators Apr 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/aws: Added RDS event subscriptions #6367

provider/aws: Added RDS event subscriptions #6367

bigkraig commented Apr 26, 2016

radeksimko Apr 27, 2016 •

edited

Loading

radeksimko commented Apr 27, 2016

bigkraig commented Apr 27, 2016

bigkraig commented Apr 27, 2016

bigkraig commented Apr 28, 2016 •

edited

Loading

radeksimko commented Apr 28, 2016

bigkraig commented Apr 28, 2016

radeksimko commented Apr 29, 2016

bigkraig commented Apr 29, 2016

radeksimko May 1, 2016

radeksimko commented May 1, 2016 •

edited

Loading

bigkraig commented May 2, 2016

radeksimko May 3, 2016

radeksimko commented May 3, 2016

radeksimko commented May 4, 2016

bigkraig commented May 5, 2016

radeksimko commented May 5, 2016 •

edited

Loading

ghost commented Apr 25, 2020

provider/aws: Added RDS event subscriptions #6367

provider/aws: Added RDS event subscriptions #6367

Conversation

bigkraig commented Apr 26, 2016

radeksimko Apr 27, 2016 • edited Loading

Choose a reason for hiding this comment

radeksimko commented Apr 27, 2016

bigkraig commented Apr 27, 2016

bigkraig commented Apr 27, 2016

bigkraig commented Apr 28, 2016 • edited Loading

radeksimko commented Apr 28, 2016

bigkraig commented Apr 28, 2016

radeksimko commented Apr 29, 2016

bigkraig commented Apr 29, 2016

radeksimko May 1, 2016

Choose a reason for hiding this comment

radeksimko commented May 1, 2016 • edited Loading

bigkraig commented May 2, 2016

radeksimko May 3, 2016

Choose a reason for hiding this comment

radeksimko commented May 3, 2016

radeksimko commented May 4, 2016

bigkraig commented May 5, 2016

radeksimko commented May 5, 2016 • edited Loading

ghost commented Apr 25, 2020

radeksimko Apr 27, 2016 •

edited

Loading

bigkraig commented Apr 28, 2016 •

edited

Loading

radeksimko commented May 1, 2016 •

edited

Loading

radeksimko commented May 5, 2016 •

edited

Loading