New Resource: aws_sns_platform_application #3283
Conversation
…inor wording changes
…and rename attributes to closer match SNS attributes
…and to set name and platform attributes in state
…orms requiring platform principal and validate during plan
* Support APNS and/or GCM platform testing * Test all attributes individually with updates * Test IAM role attributes with actual IAM roles * Test SNS topic attributes with actual SNS topics
…sistency with API
bflad
added
new-resource
| "success_feedback_sample_rate": { | ||
| Type: schema.TypeString, | ||
| Optional: true, | ||
| }, |
There was a problem hiding this comment.
I'm 👌 with the extra abstraction we built by turning all available attributes to lower_case format, but I still feel that we should stick to the API structure and nest all the attributes under attributes, what do you think?
There was a problem hiding this comment.
I'm not sure, honestly, 🙁 . The precedent here is that the other SNS resources (topic/topic_subscription) don't nest them. In this case, I think we're okay because the AWS API nesting is really just an implementation detail and its slightly easier Terraform code and user experience to keep these as top level attributes. Of course that's not valid if we were planning on supporting something like a aws_sns_platform_application_attribute resource (and do the same to the others).
If you feel strongly I can definitely change it. 👍
There was a problem hiding this comment.
It really depends how is the API going to evolve going forward - i.e. whether there is going to be more arguments on the root level (which could potentially clash with any nested ones).
I think it's a guessing game at this point and we have quite good deprecation mechanisms available in the schema, so it's fine to keep it as is for now.
|
|
||
| attributes := make(map[string]*string) | ||
|
|
||
| for k, _ := range resourceAwsSnsPlatformApplication().Schema { |
There was a problem hiding this comment.
Not sure this is a good idea, sometimes more verbose code is more than "clever abstraction" 😃
There was a problem hiding this comment.
Same as my other comment, this was probably just copypasta of the SNS topic resource in its original implementation. It was working correctly (currently anyways) so I didn't touch it.
| "was provided %q and received error: %s", platformApplicationArn.String(), err) | ||
| } | ||
|
|
||
| platformApplicationArnResourceParts := strings.Split(platformApplicationArn.Resource, "/") |
There was a problem hiding this comment.
Nitpick: It would be nice to move this into a function so it's more readable, e.g.
platform, name, err := splitSnsPlatformApplicationArn(platformApplicationArn.Resource)|
|
||
| if attributeOutput.Attributes != nil && len(attributeOutput.Attributes) > 0 { | ||
| attrmap := attributeOutput.Attributes | ||
| resource := *resourceAwsSnsPlatformApplication() |
There was a problem hiding this comment.
🤔 This feels little strange as no other resources I know of ever access the schema like this.
There was a problem hiding this comment.
Precedent (and likely where it was copied from): https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/resource_aws_sns_topic.go#L227
I'm not saying its valid/better though. It is harder to grok. If you want me to change the original PR implementation to loop through each attribute specifically by name, I can certainly do that.
|
|
||
| func hashSum(contents interface{}) string { | ||
| return fmt.Sprintf("%x", sha256.Sum256([]byte(contents.(string)))) | ||
| } |
There was a problem hiding this comment.
Nitpick: Do you mind moving this function into the test file as it's only used there?
There was a problem hiding this comment.
See StateFunc: hashSum, above in the attributes. If you look in #1101 there are some comments asking about this. We don't do this anywhere else that I know off the top of my head for storing obfuscated attribute values (to keep secrets out of the state). I don't have a strong opinion either which way on keeping it or not. It does make the acceptance testing harder when working with actual files rather than strings.
There was a problem hiding this comment.
Ah, doh 🤦♂️ , not sure why I didn't notice that.
Regarding hashing sensitive data - I don't want to set precedent either way, but I'm slightly more inclined to keeping it plain-text and adding a warning we have in docs of other similarly affected resources, like these:
https://www.terraform.io/docs/providers/aws/r/rds_cluster.html
instead of giving people false hope or setting expectation that it's ok to keep plain-text state file anywhere.
| PrincipalHash string | ||
| } | ||
|
|
||
| func testAccAwsSnsPlatformApplicationPlatformFromEnv() ([]*testAccAwsSnsPlatformApplicationPlatform, error) { |
There was a problem hiding this comment.
Nitpick: This function could theoretically take *testing.T as argument and do the skipping which would save us ~3 lines in each test and more importantly allow us to t.Fatal() on actual errors.
There was a problem hiding this comment.
That is a fantastic idea!
| t.Run(platform.Name, func(*testing.T) { | ||
| for _, tc := range testCases { | ||
| t.Run(fmt.Sprintf("%s/%s", platform.Name, tc), func(*testing.T) { | ||
| iamRoleName1 := fmt.Sprintf("tf-acc-%d", acctest.RandInt()) |
There was a problem hiding this comment.
Nitpick: Have you considered extracting the actual test logic into a function so it's slightly easier to read this?
… function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv
|
@radeksimko did the ID and test environment variables refactoring mentioned above. Everything still passes. If there are further items I should adjust here, I think we should probably chat in person since as you've noted, this is quite a complicated resource with the abstractions. 👍 |
…redential and platform_principal hashing
|
This has been released in terraform-provider-aws version 1.9.0. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
* commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9': (224 commits) v1.9.0 Update CHANGELOG for hashicorp#1101 and hashicorp#3283 docs/resource/aws_sns_platform_application: Add note about platform_credential and platform_principal hashing resource/aws_sns_platform_application: Refactor ID parsing to its own function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv Add lambda example (hashicorp#3168) Update CHANGELOG for hashicorp#3157 docs/data-source/aws_region: Remove now deprecated current argument data-source/aws_region: Refactor logic into findRegionByEc2Endpoint and findRegionByName functions Update CHANGELOG for hashicorp#3301 Update CHANGELOG for hashicorp#2559 and hashicorp#3240 Update CHANGELOG.md resource/aws_kinesis_stream: Retry deletion on LimitExceededException (hashicorp#3108) Update CHANGELOG.md resource/aws_dynamodb_table_item: Cleanup + add missing bits Added dynamodb_table_item resource hashicorp#517 Update CHANGELOG.md New Resource: aws_cloud9_environment_ec2 Update CHANGELOG.md Fixed markdown typo in docs resource/aws_kinesis_firehose_delivery_stream: Prevent crashes on empty CloudWatchLoggingOptions and fix extended_s3_configuration kms_key_arn ... # Conflicts: # aws/validators.go
…parameters-features * commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9': (752 commits) v1.9.0 Update CHANGELOG for hashicorp#1101 and hashicorp#3283 docs/resource/aws_sns_platform_application: Add note about platform_credential and platform_principal hashing resource/aws_sns_platform_application: Refactor ID parsing to its own function, use testing.T in testAccAwsSnsPlatformApplicationPlatformFromEnv Add lambda example (hashicorp#3168) Update CHANGELOG for hashicorp#3157 docs/data-source/aws_region: Remove now deprecated current argument data-source/aws_region: Refactor logic into findRegionByEc2Endpoint and findRegionByName functions Update CHANGELOG for hashicorp#3301 Update CHANGELOG for hashicorp#2559 and hashicorp#3240 Update CHANGELOG.md resource/aws_kinesis_stream: Retry deletion on LimitExceededException (hashicorp#3108) Update CHANGELOG.md resource/aws_dynamodb_table_item: Cleanup + add missing bits Added dynamodb_table_item resource hashicorp#517 Update CHANGELOG.md New Resource: aws_cloud9_environment_ec2 Update CHANGELOG.md Fixed markdown typo in docs resource/aws_kinesis_firehose_delivery_stream: Prevent crashes on empty CloudWatchLoggingOptions and fix extended_s3_configuration kms_key_arn ... # Conflicts: # aws/resource_aws_ssm_parameter_test.go
|
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
This PR is a continuation of #1101