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

New Resource: aws_sns_application #1101

Merged
merged 1 commit into from
Feb 9, 2018
Merged

New Resource: aws_sns_application #1101

merged 1 commit into from
Feb 9, 2018

Conversation

iceycake
Copy link
Contributor

hashicorp/terraform#8294 has been opened for almost a year and we need further discussion on how to get this merge into the aws provider.

@megaqube @bigkraig @mikecook @etartakovsky please comment on this as we all really need this PR.

@iceycake
Copy link
Contributor Author

@stack72 Please review this PR again as you have mentioned back in Dec last year you will try to look into this with a test account.

@bigkraig
Copy link
Contributor

👍 lets do this!

@grubernaut grubernaut mentioned this pull request Jul 13, 2017
Closed
@grubernaut grubernaut self-assigned this Jul 13, 2017
@grubernaut grubernaut added enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. labels Jul 13, 2017
grubernaut
grubernaut suggested changes Jul 13, 2017
View reviewed changes
Copy link
Contributor

@grubernaut grubernaut left a comment

Choose a reason for hiding this comment

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

Left some comments, questions, and suggestions. Please take a look, thanks!

aws/resource_aws_sns_application.go
"log"

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

Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit: align imports here

aws/resource_aws_sns_application.go
)

var SupportedPlatforms = map[string]bool{
//"ADM": true, // (Amazon Device Messaging)
Copy link
Contributor

Choose a reason for hiding this comment

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

ADM is not currently supported, previously deprecated, or to be supported in the future?

aws/resource_aws_sns_application.go
},

Schema: map[string]*schema.Schema{
"name": &schema.Schema{
Copy link
Contributor

Choose a reason for hiding this comment

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

super minor nit: can remove all of the duplicate struct field declarations here:

"name": { 
  // ....
}

aws/resource_aws_sns_application.go
"credential": &schema.Schema{
Type: schema.TypeString,
Required: true,
ForceNew: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

ForceNew defaults to false, can remove

aws/resource_aws_sns_application.go
Type: schema.TypeString,
Required: true,
ForceNew: false,
StateFunc: hashSum,
Copy link
Contributor

Choose a reason for hiding this comment

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

Privacy the primary reasoning for using a hashSum here, and for principal?

aws/resource_aws_sns_application_test.go

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

Copy link
Contributor

Choose a reason for hiding this comment

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

minor nit: align imports

aws/resource_aws_sns_application_test.go
resource.TestCheckResourceAttr(
"aws_sns_application.gcm_test", "success_sample_rate", "100"),
resource.TestCheckResourceAttr(
"aws_sns_application.gcm_test", "created_topic", "arn:aws:sns:us-east-1:638386993804:endpoint-created-topic"),
Copy link
Contributor

Choose a reason for hiding this comment

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

The arn values here will change based on which user runs the acceptance test. Can use TestCheckResourceAttrSet() to verify that the attribute was set to a value. For explicit attribute checking, I believe we have a helper function in the aws provider to build an arn regardless of user-id or govcloud, etc.

aws/resource_aws_sns_application_test.go
resource.TestCheckResourceAttr(
"aws_sns_application.gcm_test", "success_sample_rate", "99"),
resource.TestCheckResourceAttr(
"aws_sns_application.gcm_test", "created_topic", "arn:aws:sns:us-east-1:638386993804:endpoint-created-topic-update"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

aws/resource_aws_sns_application_test.go
resource "aws_sns_application" "gcm_test" {
name = "aws_sns_application_test"
platform = "GCM"
created_topic = "arn:aws:sns:us-east-1:638386993804:endpoint-created-topic"
Copy link
Contributor

Choose a reason for hiding this comment

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

These arn's should be created during the terraform run of the acceptance test, for access reasons.

aws/resource_aws_sns_application_test.go
return nil
}

var testAccAWSSNSApplicationGCMConfig = `
Copy link
Contributor

Choose a reason for hiding this comment

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

For future updates to the resource, it helps to use a function to return a string for configuration values. This also allows you to get all the necessary environment variables for a configuration prior to writing the string.

func testAccAWSSNSApplicationGCMConfig(gcm_key string) string {
  return fmt.Sprintf(`foo bar baz`)
}

@grubernaut
Copy link
Contributor

Initial review looks good @iceycake + @dalehamel + @mgaqube.

However, the main blocker to being able to merge this PR, is that we need testing variables for all of the extra environment variables that are now required to test this resource in the AWS provider. We run nightly acceptance tests, and will need to be able to test this resource prior to merging.
Feel free to email me at either my personal email, or employee email (jake@hashicorp.com), with any tokens required to test this resource.
We might be able to create these tokens internally, but it will definitely take longer to get approval than it might take to get from the author(s).

Thanks again for your patience!

@iceycake
Copy link
Contributor Author

@megaqube do you still have any testing token that can provide to them?

@megaqube
Copy link

@iceycake @grubernaut Would this be for longterm testing use or just to test with before accepting the PR? Unfortunately the API tokens for Android and IOS that I used to test are tied to our company's accounts, so I cannot share those. I do not have a personal Apple Developer account, but if you need a short term use GCM API token to test, I can probably create one with my personal account, but I will need to disable after you've tested it. Let me know if that would suffice.

@grubernaut
Copy link
Contributor

@iceycake + @megaqube, the tokens will need to be used for long-term testing use. We will likely be able to create an ADM key to use for the resource, but that may take a while, if it incurs extra costs associated.

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 19, 2017
@ImMatureTony
Copy link

Any word on this? Looking forward to it. 👍

@kwent
Copy link
Contributor

kwent commented Oct 20, 2017

I'm looking forward to this feature as well ! Where are we at ? Any ETA ? Thanks for the great work.

@radeksimko radeksimko added the size/XL Managed by automation to categorize the size of a PR. label Nov 15, 2017
@vladimir-malyarevich-syncplicity
Copy link

Dear community,
What's a big deal to merge this pull request?
We suffer of not having this.

@radeksimko radeksimko added this to the v1.4.0 milestone Nov 15, 2017
@Ninir
Copy link
Contributor

Ninir commented Nov 16, 2017

Hi @megaqube @iceycake

Would you be able to provide such tokens & fix @grubernaut 's review?

Thanks!

@iceycake
Copy link
Contributor Author

@Ninir As @grubernaut mentioned 4 months ago, this test is associating additional cost for the token. To answer your question, sorry i can’t gjve out my company token as again there is $$$ associate to that.

We are building our internal aws provider along with a few other unmerged PRs to get around with this issue.

@Ninir
Copy link
Contributor

Ninir commented Nov 20, 2017

@iceycake Totally understandable :)

Regarding the unmerged PRs, could you provide me the links to them by PM on Gitter so that we review them?

Regarding the tokens, we will need to figure out a way to do that :)

Thanks!

@radeksimko radeksimko modified the milestones: v1.4.0, v1.6.0 Nov 29, 2017
@radeksimko radeksimko added the service/sns Issues and PRs that pertain to the sns service. label Jan 16, 2018
@radeksimko radeksimko changed the title Added implementation and documentation for SNS Application Platform New Resource: aws_sns_application Jan 16, 2018
@radeksimko radeksimko modified the milestones: v1.6.0, v1.9.0 Jan 16, 2018
@kwent
Copy link
Contributor

kwent commented Jan 25, 2018

Up! Still needed in 2018. How can we help ?

@bflad bflad self-assigned this Feb 1, 2018
@bflad
Copy link
Contributor

bflad commented Feb 1, 2018

How can we help?

I'll be looking at this PR shortly to take it across the finish line, but if anyone could fix up the items from the initial review (as they look like items I would say/ask as well), comment on them, or at least let us know that no more development from the original authors can occur, that would be a help to getting the ball rolling here again.

Thanks!

bflad added a commit that referenced this pull request Feb 7, 2018
@bflad
resource/aws_sns_application: Incorporate resource feedback from #1101 …
86fc7a5 
…and rename attributes to closer match SNS attributes
@bflad bflad mentioned this pull request Feb 7, 2018
Merged
@bflad
Copy link
Contributor

bflad commented Feb 7, 2018

I have submitted a followup PR: #3283

@bflad bflad merged commit d778b67 into hashicorp:master Feb 9, 2018
bflad added a commit that referenced this pull request Feb 9, 2018
@bflad
Update CHANGELOG for #1101 and #3283
65181e8
@bflad
Copy link
Contributor

bflad commented Feb 9, 2018

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.

jocgir added a commit to coveooss/terraform-provider-aws that referenced this pull request Feb 12, 2018
@jocgir
Merge commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9'
e27844b 
* 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
jocgir added a commit to coveooss/terraform-provider-aws that referenced this pull request Feb 12, 2018
@jocgir
Merge commit '5293a0e3b1366ee16d8742b9b2354781a79bfbd9' into new-ssm-…
b96bc39 
…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
@ghost
Copy link

ghost commented Apr 8, 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 8, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@grubernaut grubernaut grubernaut requested changes

Assignees

@bflad bflad

Labels
enhancement Requests to existing resources that expand the functionality or scope. new-resource Introduces a new resource. service/sns Issues and PRs that pertain to the sns service. size/XL Managed by automation to categorize the size of a PR.
Projects
None yet
Milestone
v1.9.0
Development

Successfully merging this pull request may close these issues.