-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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(stepfunctions-tasks): create scheduler #29458
base: main
Are you sure you want to change the base?
feat(stepfunctions-tasks): create scheduler #29458
Conversation
@cshields236 Please wait for a while. |
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.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request
. Additionally, if clarification is needed add Clarification Request
to a comment.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
562fae8
to
6a21c6d
Compare
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.
Thanks 👍 Left comments for adjustments
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
* | ||
* @see https://docs.aws.amazon.com/scheduler/latest/UserGuide/schedule-types.html | ||
*/ | ||
readonly scheduleExpression: string; |
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 about using a Schedule
class similar to this?
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.
It's a really nice suggestion. Do you mean implementing the Schedule
class in create-schedule.ts? Or importing this class directly?
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.
I see that the class is declared in multiple packages already.
I think you can create it inside the aws-stepfunctions-tasks/lib
package.
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.
I defined Schedule
class in create-schedule.ts
.
I also referred to this class, particularly the rate() method.
https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-events/lib/schedule.ts#L11
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
21bf7d1
to
1c9a271
Compare
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.
Thank you for your review!! I've addressed your comments.
I have one question about implementation of Schedule
class.
* | ||
* @see https://docs.aws.amazon.com/scheduler/latest/UserGuide/managing-targets.html | ||
*/ | ||
readonly target: string; |
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.
I want to name this parameter as targetArn
but it causes linter failure.
error: [awslint:props-no-arn-refs:aws-cdk-lib.aws_stepfunctions_tasks.EventBridgeSchedulerCreateScheduleTaskProps.targetArn] props must use strong types instead of attributes. props should not have "arn" suffix
* | ||
* @see https://docs.aws.amazon.com/scheduler/latest/UserGuide/schedule-types.html | ||
*/ | ||
readonly scheduleExpression: string; |
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.
It's a really nice suggestion. Do you mean implementing the Schedule
class in create-schedule.ts? Or importing this class directly?
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
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.
Thanks 👍 Left some comments for adjustments.
Sorry for the late review, I've been a bit busy recently.
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Show resolved
Hide resolved
…cheduler/create-schedule.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
…cheduler/create-schedule.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
…cheduler/create-schedule.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
…cheduler/create-schedule.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
@lpizzinidev Thanks always!! I've addressed your comments. There is no rush, so please feel free to review it whenever you have time. |
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.
Thanks 👍 Left suggestions for some final adjustments.
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions-tasks/lib/eventbridge-scheduler/create-schedule.ts
Outdated
Show resolved
Hide resolved
…cheduler/create-schedule.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
…cheduler/create-schedule.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
…cheduler/create-schedule.ts Co-authored-by: Luca Pizzini <lpizzini7@gmail.com>
@lpizzinidev Thanks a lot!! I've updated my PR according to your suggestion😁 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue # (if applicable)
Closes #29351.
Reason for this change
Although the creation of schedule task is supported, the AWS CDK currently lacks the capability to create these without a custom task definition.
Description of changes
I've introduced the
EventBridgeSchedulerCreateScheduleTask
class to address this gap.The original issue discussed the need for both creating and updating schedules. However, to maintain focus and simplicity, this PR will only cover the creation aspect. A subsequent PR will be dedicated to schedule updates.
Description of how you validated changes
I have added both integ and unit tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license