-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
feat: add sqs and sns policies based on destination config #1299
feat: add sqs and sns policies based on destination config #1299
Conversation
samtranslator/model/iam.py
Outdated
@classmethod | ||
def sqs_send_message_role_policy(cls, queue_arn, logical_id): | ||
document = { | ||
'PolicyName': 'SQSPublishPolicy' + logical_id, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I change the naming convention to logical_id + SQSPublishPolicy
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the existing convention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
existing convention is FunctioRole logical_id+ 'Policy'+'integer number'. But I cannot follow this convention as it might conflict with the explicit Policies
section.
logical_id
is Functioname+EventName
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated the policy name to <eventlogicalid> + <sqspolicy/snspolicy>
@@ -61,30 +64,35 @@ | |||
], | |||
"Policies": [ | |||
{ | |||
"PolicyName": "MyFunctionForBatchingExampleRolePolicy0", | |||
"PolicyName": "SQSPublishPolicyMyFunctionForBatchingExampleDynamoDBStreamEvent", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SQSPublishPolicyMyFunctionForBatchingExampleDynamoDBStreamEvent
will be changed as MyFunctionForBatchingExampleDynamoDBStreamEventSQSPublishRolePolicy
Codecov Report
@@ Coverage Diff @@
## develop #1299 +/- ##
===========================================
- Coverage 94.61% 94.39% -0.23%
===========================================
Files 78 78
Lines 4252 4280 +28
Branches 844 854 +10
===========================================
+ Hits 4023 4040 +17
- Misses 110 114 +4
- Partials 119 126 +7
Continue to review full report at Codecov.
|
Issue #, if available:
SAM expects the users to define
sqs:SendMessage
andsns:publish
policies whenDestinationConfig
property is set for Kinesis and DynamoDb event types (documentation).Description of changes:
This PR removes the need to specify these policies explicitly if a property
Type
is specified by the user inOnFailure
property ofDestinationConfig
.If
Type
is not given users have to specify the policiles explicitly. Hence, this change is backwards compatible.Description of how you validated changes:
Deployed the template and verified if the right policies are added and also verified if the DestinationConfig is displayed without
Type
parameter in the consoleChecklist:
make pr
passesexamples/2016-10-31
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.