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

r/sns_topic_subscription - recreate on topic deletion + validations #14101

Merged
merged 2 commits into from
Mar 25, 2021
Merged

r/sns_topic_subscription - recreate on topic deletion + validations #14101

merged 2 commits into from
Mar 25, 2021

Conversation

DrFaust92
Copy link
Collaborator

@DrFaust92 DrFaust92 commented Jul 8, 2020 •
edited by gdavison
Loading

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #9645

Release note for CHANGELOG:

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSSNSTopicSubscription_'
--- PASS: TestAccAWSSNSTopicSubscription_basic (77.90s)
--- PASS: TestAccAWSSNSTopicSubscription_filterPolicy (175.19s)
--- PASS: TestAccAWSSNSTopicSubscription_deliveryPolicy (134.98s)
--- PASS: TestAccAWSSNSTopicSubscription_rawMessageDelivery (130.24s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (104.22s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (166.53s)
--- PASS: TestAccAWSSNSTopicSubscription_disappears (156.98s)
--- PASS: TestAccAWSSNSTopicSubscription_disappears_topic (83.49s)

@DrFaust92 DrFaust92 requested a review from a team July 8, 2020 21:18
@ghost ghost added size/XL Managed by automation to categorize the size of a PR. service/sns Issues and PRs that pertain to the sns service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Jul 8, 2020
@DrFaust92 DrFaust92 changed the title r/sns_topic_subscription - read after create + disappears tests [WIP]r/sns_topic_subscription - read after create + disappears tests Jul 8, 2020
@DrFaust92 DrFaust92 changed the title [WIP]r/sns_topic_subscription - read after create + disappears tests r/sns_topic_subscription - read after create + disappears tests Jul 9, 2020
@DrFaust92
Copy link
Collaborator Author

just noticed #10496 exists.

let me know if i should close mine or rebase after the aforementioned PR is merged

@ghost ghost added the documentation Introduces or discusses updates to documentation. label Jul 9, 2020
@bflad
Copy link
Contributor

bflad commented Oct 2, 2020

#10496 has been merged, so this is ready for rebase

@bflad bflad added enhancement Requests to existing resources that expand the functionality or scope. technical-debt Addresses areas of the codebase that need refactoring or redesign. waiting-response Maintainers are waiting on response from community or contributor. labels Oct 2, 2020
@DrFaust92 DrFaust92 marked this pull request as draft October 2, 2020 21:10
@DrFaust92 DrFaust92 marked this pull request as ready for review October 3, 2020 09:39
@DrFaust92
Copy link
Collaborator Author

rebased + reran tests

ewbankkit
ewbankkit reviewed Oct 26, 2020
View reviewed changes
aws/resource_aws_sns_topic_subscription.go
@@ -152,11 +158,11 @@ func resourceAwsSnsTopicSubscriptionUpdate(d *schema.ResourceData, meta interfac
}

func resourceAwsSnsTopicSubscriptionRead(d *schema.ResourceData, meta interface{}) error {
snsconn := meta.(*AWSClient).snsconn
conn := meta.(*AWSClient).snsconn

log.Printf("[DEBUG] Loading subscription %s", d.Id())

Copy link
Contributor

Choose a reason for hiding this comment

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

Call ListSubscriptionsByTopic first?
See #9645 (comment).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank, Added.

@DrFaust92 DrFaust92 force-pushed the r/sns_subscription_refactor branch 2 times, most recently from 152d0c4 to 0b80c80 Compare October 29, 2020 20:18
@DrFaust92
Copy link
Collaborator Author

rerun tests:

--- PASS: TestAccAWSSNSTopicSubscription_disappears (111.31s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (121.52s)
--- PASS: TestAccAWSSNSTopicSubscription_deliveryPolicy (127.22s)
--- PASS: TestAccAWSSNSTopicSubscription_filterPolicy (162.21s)
--- PASS: TestAccAWSSNSTopicSubscription_rawMessageDelivery (171.12s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (201.53s)

@DrFaust92 DrFaust92 removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 29, 2020
@DrFaust92 DrFaust92 requested a review from a team as a code owner November 9, 2020 19:32
Base automatically changed from master to main January 23, 2021 00:58
@DrFaust92
Copy link
Collaborator Author 

--- PASS: TestAccAWSSNSTopicSubscription_disappears_topic (37.01s)
--- PASS: TestAccAWSSNSTopicSubscription_basic (87.44s)
--- PASS: TestAccAWSSNSTopicSubscription_disappears (93.81s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (103.16s)
--- PASS: TestAccAWSSNSTopicSubscription_redrivePolicy (120.52s)
--- PASS: TestAccAWSSNSTopicSubscription_rawMessageDelivery (121.38s)
--- PASS: TestAccAWSSNSTopicSubscription_filterPolicy (124.27s)
--- PASS: TestAccAWSSNSTopicSubscription_deliveryPolicy (126.26s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (134.03s)

@YakDriver
Copy link
Member

@DrFaust92 Thank you for this pull request! The aws_sns_topic_subscription recently underwent substantial modernization in #14923. We apologize for the inconvenience that will cause with this PR. However, we look forward to reviewing these improvements rebased on the modernization.

@ghost ghost added size/L Managed by automation to categorize the size of a PR. and removed size/XL Managed by automation to categorize the size of a PR. labels Feb 27, 2021
@DrFaust92 DrFaust92 changed the title r/sns_topic_subscription - read after create + disappears tests r/sns_topic_subscription - recreate on topic deletion + validations Feb 27, 2021
@DrFaust92 DrFaust92 added bug Addresses a defect in current functionality. and removed technical-debt Addresses areas of the codebase that need refactoring or redesign. labels Feb 27, 2021
@gdavison gdavison self-assigned this Mar 25, 2021
gdavison
gdavison approved these changes Mar 25, 2021
View reviewed changes
Copy link
Contributor

@gdavison gdavison left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

--- PASS: TestAccAWSSNSTopicSubscription_email (25.45s)
--- PASS: TestAccAWSSNSTopicSubscription_disappears_topic (27.48s)
--- PASS: TestAccAWSSNSTopicSubscription_filterPolicy (65.17s)
--- PASS: TestAccAWSSNSTopicSubscription_rawMessageDelivery (67.47s)
--- PASS: TestAccAWSSNSTopicSubscription_redrivePolicy (67.72s)
--- PASS: TestAccAWSSNSTopicSubscription_deliveryPolicy (67.79s)
--- PASS: TestAccAWSSNSTopicSubscription_basic (71.10s)
--- PASS: TestAccAWSSNSTopicSubscription_disappears (84.14s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingEndpoint (87.73s)
--- PASS: TestAccAWSSNSTopicSubscription_autoConfirmingSecuredEndpoint (124.39s)
--- PASS: TestAccAWSSNSTopicSubscription_firehose (144.02s)

@gdavison gdavison merged commit 37b0efc into hashicorp:main Mar 25, 2021
@github-actions github-actions bot added this to the v3.34.0 milestone Mar 25, 2021
@ghost
Copy link

ghost commented Mar 26, 2021

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@DrFaust92 DrFaust92 deleted the r/sns_subscription_refactor branch April 15, 2021 10:43
@ghost
Copy link

ghost commented Apr 25, 2021

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 as resolved and limited conversation to collaborators Apr 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ewbankkit ewbankkit ewbankkit left review comments

@gdavison gdavison gdavison approved these changes

Assignees

@gdavison gdavison

Labels
bug Addresses a defect in current functionality. documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/sns Issues and PRs that pertain to the sns service. size/L 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
v3.34.0
Development

Successfully merging this pull request may close these issues.

SNS topic subscription is not recreated after SNS topic delete/recreate
5 participants
@DrFaust92 @bflad @YakDriver @gdavison @ewbankkit