-
Notifications
You must be signed in to change notification settings - Fork 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(scheduler): schedule expression construct #25422
Conversation
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.
@filletofish think you can add the README to this PR too and for the integration test we need an exemption, hopefully @kaizencc can arrange that :-) |
Sure, will do |
Exemption Request |
I think adding a stability banner to the README would also be a good idea while we do PRs do create the full functionality.
|
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.
@filletofish this entire PR needs to be migrated to a [new] folder with path packages/@aws-cdk/aws-scheduler-alpha
. The motivation is that aws-cdk-lib
is for stable modules only (including scheduler L1s). You'll need to add in the package.json and other config files into the new folder.
|
||
Amazon EventBridge Scheduler](https://aws.amazon.com/blogs/compute/introducing-amazon-eventbridge-scheduler/) is a feature from Amazon EventBridge that allows you to create, run, and manage scheduled tasks at scale. With EventBridge Scheduler, you can schedule one-time or recurrently tens of millions of tasks across many AWS services without provisioning or managing underlying infrastructure. |
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.
another comment on impending linter errors that i feel should have been enforced somewhere: these paragraphs have to be broken into separate lines with max 150 characters i believe
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 readme. So the rule on characters per line in README is not enforced anywhere? What would be a good linter to add this rule to?
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 has been enforced in the past, i think our repo restructure may have wreaked havoc on our linter perhaps. i've been meaning to look into it
packages/aws-cdk-lib/aws-scheduler/test/schedule-expression.test.ts
Outdated
Show resolved
Hide resolved
|
||
There are no official hand-written ([L2](https://docs.aws.amazon.com/cdk/latest/guide/constructs.html#constructs_lib)) constructs for this service yet. Here are some suggestions on how to proceed: | ||
```ts |
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 guess we need a disclaimer here saying that targets doesn't work yet.
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.
My initial thoughts were to add the whole README the information about all upcoming PRs already in this first PR. We could add a notice that this is not all implemented yet but will gradually come in multiple PRs to keep the PR size smaller and reviewable. What do you think @kaizencc? Reworking the README every PR seems a but useless.
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.
good with one big disclaimer :) commented that elsewhere but i just saw this
|
||
There are no official hand-written ([L2](https://docs.aws.amazon.com/cdk/latest/guide/constructs.html#constructs_lib)) constructs for this service yet. Here are some suggestions on how to proceed: | ||
```ts |
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.
My initial thoughts were to add the whole README the information about all upcoming PRs already in this first PR. We could add a notice that this is not all implemented yet but will gradually come in multiple PRs to keep the PR size smaller and reviewable. What do you think @kaizencc? Reworking the README every PR seems a but useless.
Co-authored-by: Kaizen Conroy <[email protected]>
Co-authored-by: Jacco Kulman <[email protected]>
Co-authored-by: Jacco Kulman <[email protected]>
Move L2 constructs to @aws-cdk. Also add an experimental banner.
coverageThreshold: { | ||
global: { | ||
...baseConfig.coverageThreshold.global, | ||
branches: 60, |
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.
Set coverage to 60, because current coverage (while sufficient IMO) is at 67%. We can remove this override later
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 tests seem pretty comprehensive, so im fine with this. if you look at the coverage
folder of your local checkout, you should be able to see what branches are not being covered and determine if they are necessary for unit tests.
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.
Basically just formatting and setting up the folder left. Can you do the following for me as well:
run yarn lint
and run yarn rosetta:extract
. I expect at least rosetta to fail for you. You'll need to add a rosetta
folder similar to the other alpha packages out there.
packages/@aws-cdk/aws-scheduler-alpha/lib/schedule-expression.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/schedule-expression.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/schedule-expression.ts
Outdated
Show resolved
Hide resolved
/** | ||
* Retrieve the expression for this schedule | ||
*/ | ||
public abstract readonly expressionString: string; | ||
/** | ||
* Retrieve the expression for this schedule | ||
*/ | ||
public abstract readonly timeZone?: TimeZone; |
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.
formatting
packages/@aws-cdk/aws-scheduler-alpha/lib/schedule-expression.ts
Outdated
Show resolved
Hide resolved
coverageThreshold: { | ||
global: { | ||
...baseConfig.coverageThreshold.global, | ||
branches: 60, |
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 tests seem pretty comprehensive, so im fine with this. if you look at the coverage
folder of your local checkout, you should be able to see what branches are not being covered and determine if they are necessary for unit tests.
} catch (e) { | ||
if (e instanceof RangeError) { | ||
throw new Error('Invalid date'); | ||
} | ||
throw e; | ||
} |
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.
lets add a test for this actually
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
baseConfig.rules['import/no-extraneous-dependencies'] = ['error', { devDependencies: true, peerDependencies: true } ]; | ||
baseConfig.rules['import/order'] = 'off'; | ||
baseConfig.rules['@aws-cdk/invalid-cfn-imports'] = 'off'; |
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.
btw, whats going on with these? we shouldn't turn them off
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 am not sure, I think almost every package in "@aws-cdk" folder has the same config
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.
Turned off everything except no-extraneous-dependencies
Co-authored-by: Kaizen Conroy <[email protected]>
Co-authored-by: Kaizen Conroy <[email protected]>
Co-authored-by: Kaizen Conroy <[email protected]>
Co-authored-by: Kaizen Conroy <[email protected]>
@kaizencc, output of Command Output
|
@filletofish unfortunately if rosetta fails we won't be able to merge the PR. Here are our options:
I'm ok with either but I think just having the README reflect what currently exists is the better way to go |
@filletofish the code is good and i'm ready to approve, we just have to get the build to succeed. looks like it currently is failing because the version of jest in scheduler may be mismatched with the version elsewhere. once it succeeds i shall approve! |
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 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 main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This PR contains implementation of Schedule Expression. While a schedule is the main resource in Amazon EventBridge Scheduler, this PR adds schedule expression class schedule expression on which schedule depends.
Every schedule has a schedule expression that determines when, and with what frequency, the schedule runs. EventBridge Scheduler supports three types of schedules: rate, cron, and one-time schedules. When you create a schedule, you configure a target for the schedule to invoke.
To reuse existing
events.Schedule
functionality, classScheduleExpression
uses it to generate schedule expression strings.Implementation is based on RFC: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0474-event-bridge-scheduler-l2.md
Advances #23394
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license