Skip to content
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

redshift event subscription resource #6146

Merged
merged 2 commits into from
Oct 15, 2018
Merged

redshift event subscription resource #6146

merged 2 commits into from
Oct 15, 2018

Conversation

f0rk
Copy link
Contributor

@f0rk f0rk commented Oct 13, 2018

Changes proposed in this pull request:

  • Implements the aws_redshift_event_subscription resource.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSRedshiftEventSubscription'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSRedshiftEventSubscription -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSRedshiftEventSubscription_importBasic
--- PASS: TestAccAWSRedshiftEventSubscription_importBasic (37.98s)
=== RUN   TestAccAWSRedshiftEventSubscription_basicUpdate
--- PASS: TestAccAWSRedshiftEventSubscription_basicUpdate (38.91s)
=== RUN   TestAccAWSRedshiftEventSubscription_withPrefix
--- PASS: TestAccAWSRedshiftEventSubscription_withPrefix (19.53s)
=== RUN   TestAccAWSRedshiftEventSubscription_withSourceIds
--- PASS: TestAccAWSRedshiftEventSubscription_withPrefix (18.80s)
=== RUN   TestAccAWSRedshiftEventSubscription_categoryUpdate
--- PASS: TestAccAWSRedshiftEventSubscription_categoryUpdate (31.77s)

Sorry about the large diff in aws/provider.go.

@ghost ghost added size/XXL Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/redshift Issues and PRs that pertain to the redshift service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 13, 2018
@bflad bflad added the new-resource Introduces a new resource. label Oct 14, 2018
bflad
bflad reviewed Oct 14, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @f0rk 👋 Thanks for submitting this! Initial feedback below. Please let us know if you have any questions or do not have time to implement it.

aws/resource_aws_redshift_event_subscription.go Outdated
Update: resourceAwsRedshiftEventSubscriptionUpdate,
Delete: resourceAwsRedshiftEventSubscriptionDelete,
Importer: &schema.ResourceImporter{
State: resourceAwsRedshiftEventSubscriptionImport,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the Read function already uses d.Id(), we can remove the custom import function here and use schema.ImportStatePassThrough instead. 👍

aws/resource_aws_redshift_event_subscription.go Outdated
SubscriptionName: aws.String(name),
SnsTopicArn: aws.String(d.Get("sns_topic").(string)),
Enabled: aws.Bool(d.Get("enabled").(bool)),
SourceIds: sourceIds,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic above can all be replaced with SourceIds: expandStringSet(d.Get("source_ids").(*schema.Set)), 😄

aws/resource_aws_redshift_event_subscription.go Outdated
SourceIds: sourceIds,
SourceType: aws.String(d.Get("source_type").(string)),
Severity: aws.String(d.Get("severity").(string)),
EventCategories: eventCategories,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here -- the logic above can all be replaced with EventCategories: expandStringSet(d.Get("event_categories").(*schema.Set)), 😄

aws/resource_aws_redshift_event_subscription.go Outdated
name = resource.UniqueId()
}

tags := tagsFromMapRedshift(d.Get("tags").(map[string]interface{}))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Since the tags variable isn't used anywhere else in this function, this can be moved inside CreateEventSubscriptionInput, e.g. Tags: tagsFromMapRedshift(d.Get("tags").(map[string]interface{}))

aws/resource_aws_redshift_event_subscription.go Outdated
Required: true,
ForceNew: true,
},
"sns_topic": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here:

  • We should rename this sns_topic_arn to match the Redshift API as well as be more explicit about what the argument is looking for.
  • We can add plan-time validation for this via: ValidateFunc: validateArn,

website/docs/r/redshift_event_subscription.html.markdown Outdated
master_username = "default"
master_password = "default"

...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We generally prefer to omit the configuration of other referenced resources in the example configurations due to their maintenance burden, but if this is being left here, we should ensure its valid HCL by making sure this is a comment, e.g. # ... other configuration ...

website/docs/r/redshift_event_subscription.html.markdown Outdated

The following arguments are supported:

* `name` - (rEquired) The name of the Redshift event subscription.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: rEquired should be Required

website/docs/r/redshift_event_subscription.html.markdown Outdated
The following arguments are supported:

* `name` - (rEquired) The name of the Redshift event subscription.
* `sns_topic` - (Required) The SNS topic to send events to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should explicitly mention it is the ARN of the SNS topic (after updating attribute name). 👍

website/docs/r/redshift_event_subscription.html.markdown Outdated
* `name` - (rEquired) The name of the Redshift event subscription.
* `sns_topic` - (Required) The SNS topic to send events to.
* `source_ids` - (Optional) A list of identifiers of the event sources for which events will be returned. If not specified, then all sources are included in the response. If specified, a source_type must also be specified.
* `source_type` - (Optional) The type of source that will be generating the events. Valid options are `cluster`, `cluster-parameter-group`, ``, `cluster-security-group`, or` cluster-snapshot`. If not set, all sources will be subscribed to.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The formatting here is a little off:

  • Extraneous
``,
  • Last item should be
or `cluster-snapshot`

website/docs/r/redshift_event_subscription.html.markdown Show resolved Hide resolved
@bflad
Copy link
Contributor

bflad commented Oct 14, 2018

Also, I'm guessing that the linting failure is coming from running Go 1.10 or earlier. If you update to Go 1.11 and go fmt aws/provider.go again it should stop that failure.

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 14, 2018
@f0rk
Copy link
Contributor Author

f0rk commented Oct 15, 2018

@bflad thanks for the feedback! I should be able to get to these enhancements and fixes soon

@ghost ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Oct 15, 2018
@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 15, 2018
bflad
bflad approved these changes Oct 15, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks @f0rk! 🚀

--- PASS: TestAccAWSRedshiftEventSubscription_withPrefix (4.70s)
--- PASS: TestAccAWSRedshiftEventSubscription_categoryUpdate (7.31s)
--- PASS: TestAccAWSRedshiftEventSubscription_basicUpdate (7.58s)
--- PASS: TestAccAWSRedshiftEventSubscription_withSourceIds (7.90s)

@bflad bflad added this to the v1.41.0 milestone Oct 15, 2018
@bflad bflad merged commit 2051ba0 into hashicorp:master Oct 15, 2018
bflad added a commit that referenced this pull request Oct 15, 2018
@bflad
Update CHANGELOG for #6146
81be831
@bflad
Copy link
Contributor

bflad commented Oct 18, 2018

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

@ghost
Copy link

ghost commented Apr 2, 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 ghost locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
documentation Introduces or discusses updates to documentation. new-resource Introduces a new resource. service/redshift Issues and PRs that pertain to the redshift service. size/XL Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v1.41.0
Development

Successfully merging this pull request may close these issues.
2 participants
@f0rk @bflad