fix(sns): race condition exists between sqs queue policy and sns subscription #21797

jonnekaunisto · 2022-08-29T03:35:26Z

Fixes #20665 by adding a dependency to sqs policy for sns subscriptions.

All Submissions:

Have you followed the guidelines in our Contributing guide?

Adding new Unconventional Dependencies:

This PR adds new unconventional dependencies following the process described here

New Features

Have you added the new feature to an integration test?
- Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

This is a follow up to #21259, which got closed for failing for too long

gitpod-io · 2022-08-29T03:35:33Z

TheRealAmazonKendra

This cannot get merged because we can't edit the branch. I am also unable to verify that the tests run locally since I can't download the PR.

Additionally, the follow items weren't confirmed in the PR body:

Have you added the new feature to an integration test?

Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

jonnekaunisto · 2022-09-01T05:41:19Z

@TheRealAmazonKendra

This cannot get merged because we can't edit the branch. I am also unable to verify that the tests run locally since I can't download the PR.

I'm not sure I understand the problem, how do I fix this?

Have you added the new feature to an integration test?

I marked it as completed now, I think the existing integ tests should cover it(Ex. packages/@aws-cdk/aws-stepfunctions-tasks/test/sns/publish.integ.snapshot/aws-stepfunctions-tasks-sns-publish-integ.template.json)

Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

I will work on doing that.

TheRealAmazonKendra · 2022-09-02T21:09:05Z

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork

Looks like you either got this fixed or GitHub was having a hiccup of some sort when I was trying to pull this earlier.

Pull request has been modified.

TheRealAmazonKendra

Looks like the merge didn't go quite right.

jonnekaunisto · 2022-09-09T21:05:10Z

Yeah after merging it doesn't let me build anything, because I get errors like this:

/workspace/aws-cdk/packages/aws-cdk: yarn build  (25 remaining)
yarn run v1.22.19
$ cdk-build
test/diff.test.ts(265,30): error TS2749: 'StringDecoder' refers to a value, but is being used as a type here. Did you mean 'typeof StringDecoder'?
Error: /workspace/aws-cdk/node_modules/typescript/bin/tsc --build exited with error code 1

Any clue how to fix that?

Pull request has been modified.

TheRealAmazonKendra · 2022-09-20T02:43:46Z

@Mergifyio update

mergify · 2022-09-20T02:44:20Z

update

✅ Branch has been successfully updated

TheRealAmazonKendra

Looks like a test is improperly formatted per

@aws-cdk/aws-sns-subscriptions:   ERROR      integ.sns-sqs.lit 1.49s
@aws-cdk/aws-sns-subscriptions:       /codebuild/output/src410239659/src/github.com/aws/aws-cdk/packages/@aws-cdk/aws-sns-subscriptions/test/sns-sqs.lit.integ.snapshot/aws-cdk-sns-sqs.template.json: Unexpected token } in JSON at position 965

This suggests that they're been manually edited, which we should not be doing here. Please run the tests with --update-on-failed and ensure that the deployment succeeds before committing the resulting change.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

Pull request has been modified.

aws-cdk-automation

The Pull Request Linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

jonnekaunisto · 2022-09-27T23:44:30Z

What do I need to do to fix this: Fixes must contain a change to an integration test file and the resulting snapshot.

I did update the integration test files @TheRealAmazonKendra

Pull Request updated. Dissmissing previous PRLinter Review.

mergify · 2022-10-02T17:31:52Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2022-10-02T19:00:48Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: a51f66a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2022-10-02T19:01:09Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…cription (aws#21797) ---- Fixes aws#20665 by adding a dependency to sqs policy for sns subscriptions. ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* This is a follow up to aws#21259, which got closed for failing for too long

tobytipton · 2022-10-27T15:26:10Z

packages/@aws-cdk/aws-sns/lib/topic-base.ts

@@ -80,8 +80,8 @@ export abstract class TopicBase extends Resource implements ITopic {
  /**
   * Subscribe some endpoint to this topic
   */
-  public addSubscription(subscription: ITopicSubscription): Subscription {


making a change from subscription to topicSubscription actually can cause impact this actually breaks in python if you are using named arguments because this changes the argument from subscription to topic_subscription

…cription (aws#21797) ---- Fixes aws#20665 by adding a dependency to sqs policy for sns subscriptions. ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* This is a follow up to aws#21259, which got closed for failing for too long

jonnekaunisto added 4 commits July 20, 2022 22:52

fix(sns): add dependency to sqs policy for sns subscription

a8ac13b

Fix linting error

dde620f

Update CloudFormation package snapshot

87a1b3d

Change dependencies to a single dependency

5dad5a8

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Aug 29, 2022

aws-cdk-automation requested a review from a team August 29, 2022 03:35

jonnekaunisto added 3 commits August 29, 2022 16:37

Updated aws-events snapshot

20b7826

Updaet step functions snapshot

15cf7b4

Update ecs-service-externsion snapshot

e7f0336

TheRealAmazonKendra changed the title ~~fix(sns): race condition exists between sqs queue policy and sns subscription #21259~~ fix(sns): race condition exists between sqs queue policy and sns subscription Aug 30, 2022

TheRealAmazonKendra previously requested changes Sep 1, 2022

View reviewed changes

Merged from upstream

90dddc8

TheRealAmazonKendra previously requested changes Sep 9, 2022

View reviewed changes

Merge branch 'main' into sns-topic-fix

30c48d8

Merge branch 'main' into sns-topic-fix

069f47f

TheRealAmazonKendra previously requested changes Sep 20, 2022

View reviewed changes

aws-cdk-automation previously requested changes Sep 27, 2022

View reviewed changes

Fix snapshot

8963e67

aws-cdk-automation previously requested changes Sep 27, 2022

View reviewed changes

TheRealAmazonKendra added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Oct 2, 2022

TheRealAmazonKendra approved these changes Oct 2, 2022

View reviewed changes

Merge branch 'main' into sns-topic-fix

a51f66a

mergify bot merged commit cf43b03 into aws:main Oct 2, 2022

jonnekaunisto deleted the sns-topic-fix branch October 3, 2022 16:57

tobytipton reviewed Oct 27, 2022

View reviewed changes

MJDeligan mentioned this pull request Nov 5, 2022

cdklocal deployment fails localstack/localstack#7122

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sns): race condition exists between sqs queue policy and sns subscription #21797

fix(sns): race condition exists between sqs queue policy and sns subscription #21797

jonnekaunisto commented Aug 29, 2022 •

edited

Loading

gitpod-io bot commented Aug 29, 2022

TheRealAmazonKendra left a comment

jonnekaunisto commented Sep 1, 2022

TheRealAmazonKendra commented Sep 2, 2022

TheRealAmazonKendra left a comment

jonnekaunisto commented Sep 9, 2022

TheRealAmazonKendra commented Sep 20, 2022

mergify bot commented Sep 20, 2022

TheRealAmazonKendra left a comment

aws-cdk-automation left a comment

aws-cdk-automation left a comment

jonnekaunisto commented Sep 27, 2022 •

edited

Loading

mergify bot commented Oct 2, 2022

aws-cdk-automation commented Oct 2, 2022

mergify bot commented Oct 2, 2022

tobytipton Oct 27, 2022

fix(sns): race condition exists between sqs queue policy and sns subscription #21797

fix(sns): race condition exists between sqs queue policy and sns subscription #21797

Conversation

jonnekaunisto commented Aug 29, 2022 • edited Loading

All Submissions:

Adding new Unconventional Dependencies:

New Features

gitpod-io bot commented Aug 29, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

jonnekaunisto commented Sep 1, 2022

TheRealAmazonKendra commented Sep 2, 2022

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

jonnekaunisto commented Sep 9, 2022

TheRealAmazonKendra commented Sep 20, 2022

mergify bot commented Sep 20, 2022

✅ Branch has been successfully updated

TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

aws-cdk-automation left a comment

Choose a reason for hiding this comment

jonnekaunisto commented Sep 27, 2022 • edited Loading

mergify bot commented Oct 2, 2022

aws-cdk-automation commented Oct 2, 2022

AWS CodeBuild CI Report

mergify bot commented Oct 2, 2022

tobytipton Oct 27, 2022

Choose a reason for hiding this comment

jonnekaunisto commented Aug 29, 2022 •

edited

Loading

jonnekaunisto commented Sep 27, 2022 •

edited

Loading