feat(sns): enable passing PolicyDocument to TopicPolicy #10559
Conversation
merging new changes from master of aws/aws-cdk
TopicPolicy class had a bug. It did specify a mandatory prop for IAM policy document to use in AWS::SNS::TopicPolicy fixes aws#7934
|
|
There was a problem hiding this comment.
Sorry for the delay here. I was really reticent to make this a required property and then in the future want to go back to making it optional which would require a breaking change. It looks like the issue here is that we don't JSONify the PolicyDocument before passing it to the L1. So we can make the property optional, then call toJSON on this.policyDocument so that the default value is correctly passed to the template.
Ignore me 🤣
|
Actually I'm dismissing that review as that still doesn't result in an empty policy document in the template. Passing an empty JSON object also doesn't work as then it fails at deploy time. It does look like there is no sensible default we could provide for the policy document so required is the way to go. Just gonna double confirm that though. |
mergify
bot
dismissed
MrArnoldPalmer’s stale review
Pull request has been modified.
MrArnoldPalmer
added
the
pr/do-not-merge
mergify
bot
dismissed
MrArnoldPalmer’s stale review
Pull request has been modified.
github-actions
bot
added
the
@aws-cdk/aws-sns
MrArnoldPalmer
removed
the
pr/do-not-merge
There was a problem hiding this comment.
Because the construct is in fact usable now, we can't break users by adding a new required property to input. It does make sense to still add this property to allow users to pass existing PolicyDocuments/Statements etc, so I just changed it to be optional and added an additional test to prevent future breakage of the topicPolicy.document.addStatements workflow.
MrArnoldPalmer
changed the title
MrArnoldPalmer
changed the title
mergify
bot
dismissed
MrArnoldPalmer’s stale review
Pull request has been modified.
mergify
bot
dismissed
MrArnoldPalmer’s stale review
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Adds optional `policyDocument` prop to `TopicPolicyProps` to allow passing existing policy documents. fixes aws#7934 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds optional `policyDocument` prop to `TopicPolicyProps` to allow passing existing policy documents. fixes aws#7934 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds optional `policyDocument` prop to `TopicPolicyProps` to allow passing existing policy documents. fixes aws#7934 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Adds optional
policyDocumentprop toTopicPolicyPropsto allow passing existing policy documents.fixes #7934
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license