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
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/@aws-cdk/aws-scheduler-alpha/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

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


```ts
declare const target: targets.LambdaInvoke;

new Schedule(this, 'Schedule', {
schedule: ScheduleExpression.rate(cdk.Duration.hours(12)),
target: target,
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.

});
```

## Scheduler Targets

Expand Down
46 changes: 45 additions & 1 deletion packages/@aws-cdk/aws-scheduler-alpha/lib/schedule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,26 @@ export interface ScheduleProps {
* @default - All events in Scheduler are encrypted with a key that AWS owns and manages.
*/
readonly key?: kms.IKey;

/**
* The date, in UTC, after which the schedule can begin invoking its target.
* EventBridge Scheduler ignores startDate for one-time schedules.
*
* Specify an absolute time in the ISO 8601 format. For example, 2020-12-01T00:00:00.000Z.
*
* @default - no value
*/
readonly startDate?: string;

/**
* The date, in UTC, before which the schedule can invoke its target.
* EventBridge Scheduler ignores endDate for one-time schedules.
*
* Specify an absolute time in the ISO 8601 format. For example, 2020-12-01T00:00:00.000Z.
*
* @default - no value
*/
readonly endDate?: string;
}

/**
Expand Down Expand Up @@ -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


/**
* The schedule group associated with this schedule.
*/
Expand Down Expand Up @@ -254,6 +276,8 @@ export class Schedule extends Resource implements ISchedule {

this.retryPolicy = targetConfig.retryPolicy;

this.validateTimeFrame(props.startDate, props.endDate);

const resource = new CfnSchedule(this, 'Resource', {
name: this.physicalName,
flexibleTimeWindow: { mode: 'OFF' },
Expand All @@ -276,6 +300,8 @@ export class Schedule extends Resource implements ISchedule {
sageMakerPipelineParameters: targetConfig.sageMakerPipelineParameters,
sqsParameters: targetConfig.sqsParameters,
},
startDate: props.startDate,
endDate: props.endDate,
});

this.scheduleName = this.getResourceNameAttribute(resource.ref);
Expand Down Expand Up @@ -306,4 +332,22 @@ export class Schedule extends Resource implements ISchedule {
const isEmptyPolicy = Object.values(policy).every(value => value === undefined);
return !isEmptyPolicy ? policy : undefined;
}
}

private validateTimeFrame(startDate?: string, endDate?: string) {
if (startDate && !Schedule.ISO8601_REGEX.test(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}`);

throw new Error(`startDate needs to follow the format yyyy-MM-ddTHH:mm:ss.SSSZ but got ${startDate}`);
}
if (endDate && !Schedule.ISO8601_REGEX.test(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}`);

throw new Error(`endDate needs to follow the format yyyy-MM-ddTHH:mm:ss.SSSZ but got ${endDate}`);
}

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}`);
}
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.

}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,38 @@
}
}
}
},
"ScheduleWithTimeFrameC1C8BDCC": {
"Type": "AWS::Scheduler::Schedule",
"Properties": {
"EndDate": "2025-10-01T00:00:00.000Z",
"FlexibleTimeWindow": {
"Mode": "OFF"
},
"ScheduleExpression": "rate(12 hours)",
"ScheduleExpressionTimezone": "Etc/UTC",
"StartDate": "2024-04-15T06:30:00.000Z",
"State": "ENABLED",
"Target": {
"Arn": {
"Fn::GetAtt": [
"Function76856677",
"Arn"
]
},
"Input": "\"Input Text\"",
"RetryPolicy": {
"MaximumEventAgeInSeconds": 180,
"MaximumRetryAttempts": 3
},
"RoleArn": {
"Fn::GetAtt": [
"Role1ABCC5F0",
"Arn"
]
}
}
}
}
},
"Parameters": {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 7 additions & 0 deletions packages/@aws-cdk/aws-scheduler-alpha/test/integ.schedule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,13 @@ new scheduler.Schedule(stack, 'CustomerKmsSchedule', {
key,
});

new scheduler.Schedule(stack, 'ScheduleWithTimeFrame', {
schedule: expression,
target: target,
startDate: `${new Date().getFullYear() + 1}-04-15T06:30:00.000Z`,
endDate: `${new Date().getFullYear() + 2}-10-01T00:00:00.000Z`,
});

new IntegTest(app, 'integtest-schedule', {
testCases: [stack],
});
Expand Down
Loading
Loading