Add ability to create AWS::Scheduler::Schedule triggers #11707
Conversation
There was a problem hiding this comment.
Thank you @joelwalden ! That looks really good. Please see my comments
|
@medikoo Thanks for taking a look! Do the other changes look good as well? |
|
Also let's hold off on merging this. There's a required param that's missing from the target. Will update by EoD. |
| @@ -113,18 +128,38 @@ class AwsCompileScheduledEvents { | |||
| ); | |||
| } | |||
|
|
|||
| if (method === 'scheduler' && !roleArn) { | |||
There was a problem hiding this comment.
This is the execution role that the Scheduler will use to invoke the target lambda? How would you feel about generating a default execution role if a roleArn was not provided?
As this PR currently stands, a minimal execution role would look like the following:
"ScheduleExecutionRole": {
"Type": "AWS::IAM::Role",
"Properties": {
"AssumeRolePolicyDocument": {
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Principal": {
"Service": "scheduler.amazonaws.com"
},
"Action": ["sts:AssumeRole"]
}
]
},
"Policies": [
{
"PolicyName": "invoke",
"PolicyDocument": {
"Version": "2012-10-17",
"Statement": [
{
"Effect": "Allow",
"Action": ["lambda:InvokeFunction"],
"Resource": /* lambdaLogicalId ARN */
}
]
}
}
]
}
}...or you could create an unscoped version ("Resource": "*") also.
If you're not open to this, could we at least update the documentation to note that this property is required when the method is set to scheduler? Thanks for your consideration.
There was a problem hiding this comment.
@drboyer I like this idea; I assume it'd be a good idea to check if a default role has been created (perhaps via a tag) and assign it rather than creating a new one for each function in a project? There are quotas for roles on an account, and as this PR is mainly meant to address an issue where devs can hit them, so I don't want to cause the same problem I'm trying to solve here.
There was a problem hiding this comment.
@joelwalden good point. So the idea would be check if there's a "default role" with a certain tag attached (or name possibly?), and only create a generic execution role if one does not exist? And I'd imagine the logic would be skipped entirely if a roleArn was provided.
There was a problem hiding this comment.
Instead of creating a new role behind the scenes, wouldn't it be ok, to rely on execution role that Framework is creating ?
There was a problem hiding this comment.
@medikoo Sounds good; we can extend that to include the assumeRole for scheduler.amazonaws.com and use it if one isn't passed. Will update.
There was a problem hiding this comment.
Might I add, if we go with the idea of creating a scheduler group for all schedules in a service, we may also want to do the same with the role?
|
These schedules would go in the default group. Can we allow for a groupName to be passed onto the scheduler? Or maybe create a group by default based on the service + lambda name? |
|
Another feature that Eventbridge scheduler supports is specifying a timezone for the schedule. |
@joelwalden sent you a PR in your repo. @medikoo would you be kind enough to review it as well? |
Will take a look this weekend; thanks! |
|
Can this get some traction please? DST is approaching soon so it might be a good time to try this out. |
@KillDozerX2 can you continue work done in this PR? |
@medikoo Yeah, I can pick it. Can send you a new PR soon, let me know if I should create a new PR. Had already sent a PR to @joelwalden's fork. |
DST is here now, but I would still love to see this get implemented. Let me know if there is something I can do to get this over the line. |
|
Closing this PR in favor of #11935 |
Adds the ability to create AWS::Scheduler::Schedule triggers as an alternative to AWS::Event::Rule, which may be preferable if hitting account-wide limits on number of AWS::Event::Rule triggers per bus (Lambdas are only able to be triggered from the default bus).
Unless they need to use
inputPathorinputTransformer, developers should be able to just add themethodconfig option and seamlessly migrate to use Scheduler if the devs find themselves up against the account quotas.Closes: #11680