New Resource: aws_sns_sms_preferences #3858

lbrucher · 2018-03-21T10:32:08Z

There is currently no way to set the SMS preferences in SNS from Terraform.

This PR is an attempt at providing this feature.

lbrucher · 2018-03-28T07:20:12Z

Hello, I was wondering if anyone has had a chance to look at this PR? Any blocker or changes to be made? Any potential of it being merged soon? Thanks!

bcornils · 2018-04-04T14:05:28Z

Hello @lbrucher apologies for the delay. We are looking into several PR's and requests atm. Let me work on where we are and get back to you with something concrete. I'm the new PM for the project so I believe the slow down is my fault.

paultyng · 2018-04-09T16:06:09Z

aws/resource_aws_sns_sms_preferences.go

+
+const resourceId = "aws_sns_sms_id"
+
+var SMSAttributeMap = map[string]string{


no need to export this I don't think? (same with the var below as well)

Indeed, corrected

paultyng · 2018-04-09T16:09:34Z

aws/resource_aws_sns_sms_preferences.go

+	defaultSmsType := d.Get("default_sms_type").(string)
+	usageReportS3Bucket := d.Get("usage_report_s3_bucket").(string)
+
+	// Validation


Please move validation in to ValidateFunc in the schema above like this: https://github.com/terraform-providers/terraform-provider-aws/blob/3f9a830038f20f819ba947e90770c50d470d164b/aws/resource_aws_s3_bucket.go#L249

(for these other validations as well, more validations available in the validation package, int ranges, string in slice, etc)

paultyng · 2018-04-09T16:20:50Z

aws/resource_aws_sns_sms_preferences.go

+
+		Schema: map[string]*schema.Schema{
+			"monthly_spend_limit": {
+				Type:     schema.TypeString,


This probably should be a TypeInt string so TF can do the proper type validation

This is tricky because the AWS API treats all those attributes as Strings, including monthly spend limit and delivery sampling rate, and an empty string "" (not defined) is not the same as a "0" (explicit 0).
Using int values in TF won't allow us to distinguish between the 2 cases and will always set those values as "0" when retrieved as "", which would be incorrect.

paultyng · 2018-04-09T16:21:17Z

aws/resource_aws_sns_sms_preferences.go

+			},
+
+			"delivery_status_success_sampling_rate": {
+				Type:     schema.TypeString,


This probably should be a TypeInt string so TF can do the proper type validation

See above comment

paultyng · 2018-04-09T16:23:48Z

@lbrucher appreciate this PR, this is pretty close, just some minor Terraform specific stuff to clean up I think.

lbrucher · 2018-04-25T10:04:21Z

@paultyng thanks for the feedback, please see my comments above, thanks.

paultyng

LGTM, thanks!

bflad · 2018-05-02T20:52:16Z

I'm seeing a bunch of test failures but they are likely from our testing being parallelized:

=== RUN   TestAccAWSSNSSMSPreferences_almostAll
--- FAIL: TestAccAWSSNSSMSPreferences_almostAll (6.24s)
    testing.go:518: Step 0 error: After applying this step and refreshing, the plan was not empty:
        
        DIFF:
        
        UPDATE: aws_sns_sms_preferences.test_pref
          default_sms_type:                      "" => "Transactional"
          delivery_status_iam_role_arn:          "arn:aws:iam::*******:role/test_smsdelivery_role" => ""
          delivery_status_success_sampling_rate: "75" => ""
          monthly_spend_limit:                   "" => "1"
          usage_report_s3_bucket:                "" => "some-bucket"
=== RUN   TestAccAWSSNSSMSPreferences_defaultSMSType
--- FAIL: TestAccAWSSNSSMSPreferences_defaultSMSType (6.70s)
    testing.go:518: Step 0 error: After applying this step and refreshing, the plan was not empty:
        
        DIFF:
        
        UPDATE: aws_sns_sms_preferences.test_pref
          default_sms_type:                      "" => "Transactional"
          delivery_status_iam_role_arn:          "arn:aws:iam::*******:role/test_smsdelivery_role" => ""
          delivery_status_success_sampling_rate: "75" => ""

=== RUN   TestAccAWSSNSSMSPreferences_deliveryRole
--- FAIL: TestAccAWSSNSSMSPreferences_deliveryRole (9.55s)
    testing.go:579: Error destroying resource! WARNING: Dangling resources
        may exist. The full state and error is shown below.
        
        Error: Check failed: SNS SMS Preferences resource exists when it should be destroyed!
        
        State: <no state>

=== RUN   TestAccAWSSNSSMSPreferences_empty
--- FAIL: TestAccAWSSNSSMSPreferences_empty (6.10s)
    testing.go:518: Step 0 error: After applying this step and refreshing, the plan was not empty:
        
        DIFF:
        
        UPDATE: aws_sns_sms_preferences.test_pref
          delivery_status_iam_role_arn:          "arn:aws:iam::*******:role/test_smsdelivery_role" => ""
          delivery_status_success_sampling_rate: "75" => ""

This will need to be adjusted on merge

rahuljain1311 · 2018-05-04T06:17:59Z

@bflad @paultyng
Thanks for approving this PR. It will significantly help us while playing with SMSs. If you guys can tell possible time frame for releasing v1.18.0 it would be super helpful

bflad

Testing serialized and the destroy check function fixed in 0cbaba3

Passes acceptance testing now:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSSNSSMSPreferences'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSSNSSMSPreferences -timeout 120m
=== RUN   TestAccAWSSNSSMSPreferences
=== RUN   TestAccAWSSNSSMSPreferences/almostAll
=== RUN   TestAccAWSSNSSMSPreferences/defaultSMSType
=== RUN   TestAccAWSSNSSMSPreferences/deliveryRole
=== RUN   TestAccAWSSNSSMSPreferences/empty
--- PASS: TestAccAWSSNSSMSPreferences (40.16s)
    --- PASS: TestAccAWSSNSSMSPreferences/almostAll (10.71s)
    --- PASS: TestAccAWSSNSSMSPreferences/defaultSMSType (9.13s)
    --- PASS: TestAccAWSSNSSMSPreferences/deliveryRole (10.78s)
    --- PASS: TestAccAWSSNSSMSPreferences/empty (9.55s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	40.211s

bflad · 2018-05-17T21:38:49Z

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

ghost · 2020-04-06T17:12:05Z

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!

Add a resource to allow setting SNS SMS preferences

b50aec8

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 21, 2018

bflad added new-resource Introduces a new resource. service/sns Issues and PRs that pertain to the sns service. labels Mar 21, 2018

radeksimko changed the title ~~Add a resource to allow setting SNS SMS preferences~~ New Resource: aws_sns_sms_preferences Mar 23, 2018

paultyng suggested changes Apr 9, 2018

View reviewed changes

Laurent Brucher added 2 commits April 25, 2018 11:54

Validation performed using validation functions

4661df6

Not exporting smsAttriuteMap nor smsAttributeDefaultValues

c0eab44

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 25, 2018

paultyng approved these changes May 2, 2018

View reviewed changes

bflad added this to the v1.18.0 milestone May 2, 2018

bflad modified the milestones: v1.18.0, v1.19.0 May 11, 2018

bflad approved these changes May 11, 2018

View reviewed changes

bflad merged commit c0eab44 into hashicorp:master May 11, 2018

bflad added a commit that referenced this pull request May 11, 2018

Update CHANGELOG for #3858

0f420f0

ghost locked and limited conversation to collaborators Apr 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: aws_sns_sms_preferences #3858

New Resource: aws_sns_sms_preferences #3858

lbrucher commented Mar 21, 2018

lbrucher commented Mar 28, 2018

bcornils commented Apr 4, 2018

paultyng Apr 9, 2018

lbrucher Apr 25, 2018

paultyng Apr 9, 2018

paultyng Apr 9, 2018

lbrucher Apr 25, 2018

paultyng Apr 9, 2018

lbrucher Apr 25, 2018

paultyng commented Apr 9, 2018

lbrucher commented Apr 25, 2018

paultyng left a comment

bflad commented May 2, 2018

rahuljain1311 commented May 4, 2018

bflad left a comment

bflad commented May 17, 2018

ghost commented Apr 6, 2020


		const resourceId = "aws_sns_sms_id"

		var SMSAttributeMap = map[string]string{

New Resource: aws_sns_sms_preferences #3858

New Resource: aws_sns_sms_preferences #3858

Conversation

lbrucher commented Mar 21, 2018

lbrucher commented Mar 28, 2018

bcornils commented Apr 4, 2018

paultyng Apr 9, 2018

Choose a reason for hiding this comment

lbrucher Apr 25, 2018

Choose a reason for hiding this comment

paultyng Apr 9, 2018

Choose a reason for hiding this comment

paultyng Apr 9, 2018

Choose a reason for hiding this comment

lbrucher Apr 25, 2018

Choose a reason for hiding this comment

paultyng Apr 9, 2018

Choose a reason for hiding this comment

lbrucher Apr 25, 2018

Choose a reason for hiding this comment

paultyng commented Apr 9, 2018

lbrucher commented Apr 25, 2018

paultyng left a comment

Choose a reason for hiding this comment

bflad commented May 2, 2018

rahuljain1311 commented May 4, 2018

bflad left a comment

Choose a reason for hiding this comment

bflad commented May 17, 2018

ghost commented Apr 6, 2020