Skip to content
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): start and end time for schedule construct #28306

Merged
merged 16 commits into from
Dec 13, 2023

Conversation

sakurai-ryo
Copy link
Contributor

This PR added support for start and end time of the schedule.

Description

Currently, users cannot set a start time and an end time for the schedule.
A schedule without a start date will begin as soon as it is created and available, and without an end date, it will continue to invoke its target indefinitely.
With this feature, users can set the start and end dates of a schedule, allowing for more flexible schedule configurations.
https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-resource-scheduler-schedule.html#cfn-scheduler-schedule-startdate
https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-resource-scheduler-schedule.html#cfn-scheduler-schedule-enddate

In CloudFormation, users can use this feature as follows:

  TestSchedule:
    Type: AWS::Scheduler::Schedule
    Properties: 
      StartDate: "2024-12-01T13:09:00.000Z"
      EndDate: "2025-12-01T00:00:00.001Z"
      ScheduleExpression: "at(2023-01-01T00:00:00)"
      State: "ENABLED"
      Target: # target

Major changes

add property to ScheduleProps interface

Added startDate and endDate properties, and typed these values as string based on the following PR comments.
#26819 (comment)

It is not necessary to specify both startDate and endDate, they can be set independently.
Validation is performed on the following points.

  • Error if not following ISO 8601 format.
    • Must include milliseconds (yyyy-MM-ddTHH:mm:ss.SSSZ)
  • Error if startDate is later than endDate

If a time before the current time is specified, the following error occurs in CFn, but no validation is performed because the timing of validation in CDK and the timing of actual deployment are different.
The StartDate you specify cannot be earlier than 5 minutes ago.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@aws-cdk-automation aws-cdk-automation requested a review from a team December 8, 2023 17:43
@github-actions github-actions bot added p2 valued-contributor [Pilot] contributed between 6-12 PRs to the CDK labels Dec 8, 2023
@sakurai-ryo sakurai-ryo changed the title feat(scheduler): add support for start and end time of the schedule feat(scheduler): add support for start and end time of the schedule construct Dec 9, 2023
@aws-cdk-automation aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 9, 2023
Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall 👍
I left some suggestions for minor improvements.

@@ -213,6 +233,8 @@ export class Schedule extends Resource implements ISchedule {
return this.metricAll('InvocationsSentToDeadLetterCount_Truncated_MessageSizeExceeded', props);
}

private static readonly ISO8601_REGEX = /^(-?(?:[1-9][0-9]*)?[0-9]{4})-(1[0-2]|0[1-9])-(3[01]|0[1-9]|[12][0-9])T(2[0-3]|[01][0-9]):([0-5][0-9]):([0-5][0-9])\.[0-9]{3}(Z)?$/;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static readonly ISO8601_REGEX = /^(-?(?:[1-9][0-9]*)?[0-9]{4})-(1[0-2]|0[1-9])-(3[01]|0[1-9]|[12][0-9])T(2[0-3]|[01][0-9]):([0-5][0-9]):([0-5][0-9])\.[0-9]{3}(Z)?$/;
private static readonly ISO8601_REGEX = /^([0-2]\d{3})-(0[0-9]|1[0-2])-([0-2]\d|3[01])T([01]\d|2[0-4]):([0-5]\d):([0-6]\d)((\.\d{3})?)Z$/;

A previous issue was caused by TransitionDate being formatted without the final Z so a different regex may be better in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right.
I made some modifications from your suggestion because CFn throws the following error if it does not include milliseconds.
StartDate needs to follow the format yyyy-MM-ddTHH:mm:ss.SSSZ

private validateTimeFrame(startDate?: string, endDate?: string) {
if (startDate && !Schedule.ISO8601_REGEX.test(startDate)) {
throw new Error(`startDate needs to follow the format yyyy-MM-ddTHH:mm:ss.SSSZ but got ${startDate}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new Error(`startDate needs to follow the format yyyy-MM-ddTHH:mm:ss.SSSZ but got ${startDate}`);
throw new Error(`startDate must be in ISO 8601 format, got ${startDate}`);

}
if (endDate && !Schedule.ISO8601_REGEX.test(endDate)) {
throw new Error(`endDate needs to follow the format yyyy-MM-ddTHH:mm:ss.SSSZ but got ${endDate}`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
throw new Error(`endDate needs to follow the format yyyy-MM-ddTHH:mm:ss.SSSZ but got ${endDate}`);
throw new Error(`endDate must be in ISO 8601 format, got ${endDate}`);

Comment on lines 344 to 351
const start = new Date(startDate);
const end = new Date(endDate);

if (end <= start) {
throw new Error(`startDate must come before the endDate but got startDate: ${startDate}, endDate: ${endDate}`);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (startDate && endDate) {
const start = new Date(startDate);
const end = new Date(endDate);
if (end <= start) {
throw new Error(`startDate must come before the endDate but got startDate: ${startDate}, endDate: ${endDate}`);
}
}
if (startDate && endDate && startDate > endDate) {
throw new Error(`startDate must preceed endDate, got startDate: ${startDate}, endDate: ${endDate}`);
}

It should be fine to compare the strings directly given the format.

### Configuring a start and end time of the Schedule

If you choose a recurring schedule, you can set the start and end time of the Schedule by specifying the startDate and endDate.
These values must follow the format `yyyy-MM-ddTHH:mm:ss.SSSZ`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
These values must follow the format `yyyy-MM-ddTHH:mm:ss.SSSZ`.
These values must follow the ISO 8601 format (`yyyy-MM-ddTHH:mm:ss.SSSZ`).

@aws-cdk-automation aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Dec 9, 2023
@sakurai-ryo
Copy link
Contributor Author

@lpizzinidev
Thanks for your review! I fixed them.

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 11, 2023
@kaizencc kaizencc changed the title feat(scheduler): add support for start and end time of the schedule construct feat(scheduler): start and end time for schedule construct Dec 11, 2023
@@ -138,6 +138,21 @@ new Schedule(this, 'Schedule', {
});
```

### Configuring a start and end time of the Schedule

If you choose a recurring schedule, you can set the start and end time of the Schedule by specifying the startDate and endDate.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you choose a recurring schedule, you can set the start and end time of the Schedule by specifying the startDate and endDate.
If you choose a recurring schedule, you can set the start and end time of the Schedule by specifying the `startDate` and `endDate`.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer the props to be just named start and end, or startTime/endTime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In CFn, it is StartDate and EndDate, so I shortened it to start and end.

Comment on lines 152 to 153
startDate: '2023-01-01T00:00:00.000Z',
endDate: '2023-02-01T00:00:00.000Z',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we standardized on using the Date type a while back. Similar to what we are doing here. We can then convert to whatever format scheduler expects.

How do you feel about that? I think that's the best course of action here as it allows for more types of inputs, but feel free to push back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comment!

Using the Date type to allow for a variety of user inputs is certainly appealing.
However, I have some reservations about it, primarily due to potential issues with timezones.
When we initialize a Date object with a string that lacks timezone specification(new Date("2022-08-04T15:19:46.125")), the system's timezone is used by default.
For instance, if a local machine is set to Asia/Tokyo and the CI environment to UTC, the same time would be offset by 9 hours between these environments.
Since we are only dealing with ISO 8601 format and exclusively in UTC for this properties, using a string might be a safer option to avoid these timezone pitfalls.

By typing these values as string representation in the ISO 8601 format, we could mitigate these inconsistencies and ensure more predictable behavior across different environments.

This is just my personal opinion and I would love to get feedback from the CDK team.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CFN accepts startDate only in UTC time. I think when we do new Date().toISOString() the Date is automatically converted to UTC time, specifically ISO 8601, which is what we want.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toISOString

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, toISOString method is used to convert to ISO 8601 format (with UTC timezone), but in the previous comment I was referring to the timezone at the initialization of the Date object.

However, I would like to change these properties to use Date type, since this problem does not occur if the user passes a string with timezone at initialization, and this is not a CDK problem but a JavaScript Date class specification problem.
Sorry for the confusion.

@aws-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 11, 2023
@mergify mergify bot dismissed kaizencc’s stale review December 12, 2023 03:41

Pull request has been modified.

@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 12, 2023
@sakurai-ryo
Copy link
Contributor Author

@kaizencc
Thanks for your review!
I fixed it and left some comments.

@sakurai-ryo
Copy link
Contributor Author

sakurai-ryo commented Dec 13, 2023

@kaizencc
Thanks! Changed the start and end property types to use Date type.
#28306 (comment)

Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely @sakurai-ryo, thanks for your contribution!

Copy link
Contributor

mergify bot commented Dec 13, 2023

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-cdk-automation aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Dec 13, 2023
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 2f12a18
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify mergify bot merged commit 0b4ab1d into aws:main Dec 13, 2023
12 checks passed
Copy link
Contributor

mergify bot commented Dec 13, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p2 valued-contributor [Pilot] contributed between 6-12 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants