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.

Sign up for GitHub

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

Jump to bottom

Add dms_event_subscription resource #7170

Merged

bflad merged 1 commit into hashicorp:master from hawknewton:dms_event_subscription

Apr 15, 2020

Contributor

hawknewton commented Jan 17, 2019 •

edited by bflad

Loading

Closes #3057

Changes proposed in this pull request:

Add dms_event_subscription resource

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSDmsEventSubscriptionBasic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSDmsEventSubscriptionBasic -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]


=== RUN   TestAccAWSDmsEventSubscriptionBasic
=== PAUSE TestAccAWSDmsEventSubscriptionBasic
=== CONT  TestAccAWSDmsEventSubscriptionBasic
--- PASS: TestAccAWSDmsEventSubscriptionBasic (797.93s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       797.967s
$


          Add dms_event_subscription resource

eda23c7

ghost added size/XL documentation provider service/databasemigrationservice tests labels

aeschright requested a review from a team

June 25, 2019 22:09

aeschright added the new-resource label

bflad suggested changes

View reviewed changes

Contributor

bflad left a comment

Hi @hawknewton 👋 Thank you for submitting this and apologies for the delayed review. Please see the below initial feedback and reach out if you have any questions or do not have time to implement the items. 👍

aws/resource_aws_dms_event_subscription.go

Comment on lines +10 to +11

		"github.com/hashicorp/terraform/helper/resource"
		"github.com/hashicorp/terraform/helper/schema"

Contributor

bflad Mar 17, 2020

Since this pull request has been introduced, we have moved to the standalone Terraform Plugin SDK repository (this will need to be fixed in all files here):

Suggested change

      
            	"github.com/hashicorp/terraform/helper/resource"
          
            	"github.com/hashicorp/terraform/helper/schema"
          
            	"github.com/hashicorp/terraform-plugin-sdk/helper/resource"
          
            	"github.com/hashicorp/terraform-plugin-sdk/helper/schema"

aws/resource_aws_dms_event_subscription.go

+              				Set:      schema.HashString,
+              				Required: true,
+              			},
+              			"name": {

Contributor

bflad Mar 17, 2020

Nit: We can provide simple plan-time validation for this attribute via ValidateFunc: validation.StringLenBetween(1, 255) 👍

aws/resource_aws_dms_event_subscription.go

+              				Required: true,
+              				ForceNew: true,
+              			},
+              			"sns_topic_arn": {

Contributor

bflad Mar 17, 2020

Nit: We can provide simple plan-time validation for this attribute via ValidateFunc: validateArn 👍

aws/resource_aws_dms_event_subscription.go

+              				ForceNew: true,
+              				Optional: true,
+              			},
+              			"source_type": {

Contributor

bflad Mar 17, 2020

Nit: We can provide simple plan-time validation for this attribute via the below 👍

ValidateFunc: validation.StringInSlice([]string{
  "replication-instance",
  "replication-task",
}, false),

aws/resource_aws_dms_event_subscription.go

+              				// The API suppors modification but doing so loses all source_ids
+              				ForceNew: true,
+              			},
+              			"tags": {

Contributor

bflad Mar 17, 2020

Since the introduction of this pull request, tagging support in the provider codebase has moved to a shared package, keyvaluetags: #10688

The issue description highlights all the steps to refactor this functionality, although notably it looks like the Read and Update functions below are missing tags handling, so that will need to be added. The Contributing Guide section on Adding Resource Tagging Support shows how to add acceptance testing for this.

website/docs/r/dms_event_subscription.html.markdown

		@@ -0,0 +1,59 @@
		---

Contributor

bflad Mar 17, 2020

CI will now catch this, but this is missing a sidebar link in website/aws.erb

website/docs/r/dms_event_subscription.html.markdown

+                Provides a DMS (Data Migration Service) event subscription resource.
+              ---
+              # aws_dms_event_subscription

Contributor

bflad Mar 17, 2020

Documentation now includes a prefix in the title for additional clarity

Suggested change

      
            # aws_dms_event_subscription
          
            # Resource: aws_dms_event_subscription

website/docs/r/dms_event_subscription.html.markdown

+              * `name` - (Required) Name of event subscription.
+              * `enabled` - (Optional, Default: true) Whether the event subscription should be enabled.
+              * `event_categories` - (Optional) List of event categories to listen for, see `DescribeEventCategories` for a canonical list.
+              * `source_type` - (Optional, Default: all events) If specificed may be either `replication-instance` or `migration-task`

Contributor

bflad Mar 17, 2020

Typos 😎

Suggested change

      
            * `source_type` - (Optional, Default: all events) If specificed may be either `replication-instance` or `migration-task`
          
            * `source_type` - (Optional, Default: all events) Type of source for events. Valid values: `replication-instance` or `replication-task`

website/docs/r/dms_event_subscription.html.markdown

+              * `source_ids` - (Required) Ids of sources to listen to.
+              * `sns_topic_arn` - (Required) SNS topic arn to send events on.
+              <a id="timeouts"></a>

Contributor

bflad Mar 17, 2020

Suggested change

<a id="timeouts"></a>

website/docs/r/dms_event_subscription.html.markdown

Comment on lines +49 to +51

+              - `create` - (Default `30 minutes`) Used for creating event subscriptions.
+              - `update` - (Default `30 minutes`) Used for event subscription modifications.
+              - `delete` - (Default `30 minutes`) Used for destroying event descriptions.

Contributor

bflad Mar 17, 2020

These values should match the resource code:

Suggested change

      
            - `create` - (Default `30 minutes`) Used for creating event subscriptions.
          
            - `update` - (Default `30 minutes`) Used for event subscription modifications.
          
            - `delete` - (Default `30 minutes`) Used for destroying event descriptions.
          
            - `create` - (Default `10m`) Used for creating event subscriptions.
          
            - `update` - (Default `10m`) Used for event subscription modifications.
          
            - `delete` - (Default `10m`) Used for destroying event descriptions.

bflad self-assigned this

bflad added the waiting-response label

bflad added this to the v2.58.0 milestone

bflad reviewed

View reviewed changes

Contributor

bflad left a comment

Hi again @hawknewton 👋 Since we haven't heard back from you, we have implemented the review feedback in a followup commit. Thank you for working on this.

Output from acceptance testing:

--- PASS: TestAccAWSDmsEventSubscription_basic (481.92s)
--- PASS: TestAccAWSDmsEventSubscription_disappears (500.31s)
--- PASS: TestAccAWSDmsEventSubscription_Enabled (584.73s)
--- PASS: TestAccAWSDmsEventSubscription_EventCategories (601.53s)
--- PASS: TestAccAWSDmsEventSubscription_Tags (540.87s)

bflad added a commit that referenced this pull request


          resource/aws_dms_event_subscription: Finish initial implementation

92200ba

Reference: #7170 (review)

Output from acceptance testing:

```
--- PASS: TestAccAWSDmsEventSubscription_basic (481.92s)
--- PASS: TestAccAWSDmsEventSubscription_disappears (500.31s)
--- PASS: TestAccAWSDmsEventSubscription_Enabled (584.73s)
--- PASS: TestAccAWSDmsEventSubscription_EventCategories (601.53s)
--- PASS: TestAccAWSDmsEventSubscription_Tags (540.87s)
```

bflad merged commit e9447a9 into hashicorp:master

bflad added a commit that referenced this pull request


          Update CHANGELOG for #7170

5b3560f

ghost commented Apr 17, 2020

This has been released in version 2.58.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost commented May 15, 2020

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!

ghost locked and limited conversation to collaborators

breathingdust removed the waiting-response label

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation new-resource provider size/XL tests