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.

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

Allow setting the notification topic ARN for ElastiCache clusters. #2836

Closed
wants to merge 1 commit into from
Closed

Allow setting the notification topic ARN for ElastiCache clusters. #2836

wants to merge 1 commit into from

Conversation

thegedge
Copy link
Contributor

I learned that setting an empty string for the ARN on modify is a no-op, so you have to set the status to "inactive". This was the one oddity in this change.

I manually tested various creation / update scenarios, but let me know if there's somewhere I can add some tests.

@apparentlymart
Copy link
Contributor

I'm not too familiar with this ElastiCache feature, but this change looks reasonable to me having guessed what these fields probably mean.

Terraform plugins typically use acceptance tests rather than unit tests, since mocking away the target service wouldn't leave much to test. You can see the current set of acceptance tests in the resource_aws_elasticache_cluster_test.go file in the same directory, and run them using:

make testacc TEST=./builtin/providers/aws TESTARGS='-run=ElasticacheCluster'

This requires the usual AWS environment variables to be set for some AWS credentials where it can create resources for testing. Of course running these tests could cost you money, so the main Terraform README states that contributions are not required to run or write acceptance tests. They are certainly appreciated if you're willing, though.

@thegedge
Copy link
Contributor Author

Given that the existing tests aren't comprehensive and also that we've been running this code for our own stacks (we do a custom build of terraform that merges in a bunch of feature branches we'd like) without any problems, I'd love to be able to merge this as-is.

@catsby
Copy link
Contributor

catsby commented Oct 28, 2015

Thanks @thegedge – I've opened #3674 to include a test for the new notification_topic_arn, so I'm going to close this one.

Thanks!

@catsby catsby closed this Oct 28, 2015
catsby added a commit that referenced this pull request Oct 28, 2015
provider/aws: Add notification topic ARN for ElastiCache clusters (continuation from #2836)
@thegedge
Copy link
Contributor Author

Thanks for picking that up, @catsby! Your PR gives me a better idea of how to approach the acceptance tests in future PRs I may send your way 😄

@catsby
Copy link
Contributor

catsby commented Oct 29, 2015

Eagerly awaiting more PRs 😄

@ghost
Copy link

ghost commented Apr 30, 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 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.

@ghost ghost locked and limited conversation to collaborators Apr 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants