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-alpha): target properties override #27603

Merged
merged 9 commits into from
Oct 26, 2023

Conversation

lpizzinidev
Copy link
Contributor

@lpizzinidev lpizzinidev commented Oct 19, 2023

Allows to override a Schedule target's properties.
Supported properties: input, maxEventAge, and retryAttempts.

Example:

declare const target: targets.LambdaInvoke;

const oneTimeSchedule = new Schedule(this, 'Schedule', {
    schedule: ScheduleExpression.rate(cdk.Duration.hours(12)),
    target,
    targetOverrides: {
        input: ScheduleTargetInput.fromText("Overriding Target Input"),
        maxEventAge: Duration.seconds(180),
        retryAttempts: 5,
    },
});

Closes #27545.


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

@github-actions github-actions bot added effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Oct 19, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team October 19, 2023 06:41
@github-actions github-actions bot added the distinguished-contributor [Pilot] contributed 50+ PRs to the CDK label Oct 19, 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 Oct 19, 2023
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a 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.

@aws-cdk-automation aws-cdk-automation added the pr/needs-cli-test-run This PR needs CLI tests run against it. label Oct 23, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 23, 2023 07:21

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

@lpizzinidev
Copy link
Contributor Author

Exemption Request.
Not sure why this got tagged pr/needs-cli-test-run.

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Oct 23, 2023
@kaizencc kaizencc removed pr/needs-cli-test-run This PR needs CLI tests run against it. feature-request A feature should be added or improved. labels Oct 23, 2023
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.

Thanks @lpizzinidev! Super minor comments, otherwise i think we are gtg

*
* @default - The target's maximumEventAgeInSeconds is used.
*/
readonly maximumEventAge?: Duration;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's called maxEventAge in schedule-targets, lets call it that here too

*
* @default - The target's input is used.
*/
readonly input?: ScheduleTargetInput;
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
readonly input?: ScheduleTargetInput;
readonly input?: ScheduleTargetInput;

*
* @default - The target's maximumEventAgeInSeconds is used.
*/
readonly maximumEventAge?: Duration;
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
readonly maximumEventAge?: Duration;
readonly maximumEventAge?: Duration;

*
* @default - The target's maximumRetryAttempts is used.
*/
readonly maximumRetryAttempts?: number;
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
readonly maximumRetryAttempts?: number;
readonly maxRetryAttempts?: number;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to retryAttempts as in target class.

@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 Oct 23, 2023
@mergify mergify bot dismissed kaizencc’s stale review October 24, 2023 08:20

Pull request has been modified.

@github-actions github-actions bot added the feature-request A feature should be added or improved. label Oct 24, 2023
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.

One last nit @lpizzinidev

@@ -256,8 +256,22 @@ const target = new targets.LambdaInvoke(fn, {

## Overriding Target Properties

TODO: Not yet implemented. See section in [L2 Event Bridge Scheduler RFC](https://github.com/aws/aws-cdk-rfcs/blob/master/text/0474-event-bridge-scheduler-l2.md)
If you wish to reuse the same target in multiple schedules, you can override target properties like `input`,
`maximumRetryAttempts` and `maximumEventAgeInSeconds` when creating a Schedule using the `targetOverrides` parameter:
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
`maximumRetryAttempts` and `maximumEventAgeInSeconds` when creating a Schedule using the `targetOverrides` parameter:
`retryAttempts` and `maxEventAge` when creating a Schedule using the `targetOverrides` parameter:

schedule: ScheduleExpression.rate(cdk.Duration.hours(12)),
target,
targetOverrides: {
input: ScheduleTargetInput.fromText("Overriding Target Input"),
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
input: ScheduleTargetInput.fromText("Overriding Target Input"),
input: ScheduleTargetInput.fromText('Overriding Target Input'),

deadLetterConfig: targetConfig.deadLetterConfig,
retryPolicy: targetConfig.retryPolicy,
retryPolicy: retryPolicy.maximumEventAgeInSeconds || retryPolicy.maximumRetryAttempts ? retryPolicy : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like this, it's unnecessarily complicated. And what if we add a prop to retryPolicy in the future? Lets add a function that creates a retryPolicy but returns undefined if no properties inside the object are defined

@mergify mergify bot dismissed kaizencc’s stale review October 24, 2023 16:04

Pull request has been modified.

kaizencc
kaizencc previously approved these changes Oct 24, 2023
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.

Thanks!

@kaizencc kaizencc changed the title feat(schedule-alpha): support target properties override feat(schedule-alpha): target properties override Oct 24, 2023
@mergify
Copy link
Contributor

mergify bot commented Oct 24, 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).

@kaizencc kaizencc changed the title feat(schedule-alpha): target properties override feat(scheduler-alpha): target properties override Oct 26, 2023
@mergify mergify bot dismissed kaizencc’s stale review October 26, 2023 15:36

Pull request has been modified.

@mergify
Copy link
Contributor

mergify bot commented Oct 26, 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
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 9001791
  • 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 1433ff2 into aws:main Oct 26, 2023
9 checks passed
@mergify
Copy link
Contributor

mergify bot commented Oct 26, 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).

mrgrain pushed a commit that referenced this pull request Nov 1, 2023
Allows to override a `Schedule` target's properties.
Supported properties: `input`, `maxEventAge`, and `retryAttempts`.

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

const oneTimeSchedule = new Schedule(this, 'Schedule', {
    schedule: ScheduleExpression.rate(cdk.Duration.hours(12)),
    target,
    targetOverrides: {
        input: ScheduleTargetInput.fromText("Overriding Target Input"),
        maxEventAge: Duration.seconds(180),
        retryAttempts: 5,
    },
});
```

Closes #27545.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
distinguished-contributor [Pilot] contributed 50+ PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(aws-scheduler-alpha): Override target properties in schedule
3 participants