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): ScheduleTargetInput #25663

Merged
merged 15 commits into from
Jun 29, 2023

Conversation

Jacco
Copy link
Contributor

@Jacco Jacco commented May 21, 2023

This PR contains implementation of ScheduleTargetInput. While a schedule is the main resource in Amazon EventBridge Scheduler, this PR adds ScheduleTargetInput on which ScheduleTargetBase depends.

Every Schedule has a target that determines what extra information is sent to the target when the schedule is triggered. Also 4 ContextAttributes can be used that will be resolved at trigger-time.

To be able to create sensible unit tests, also the a start is made to add the Schedule and the LambdaInvoke target as described in the RFC.

Implementation is based on RFC: https://github.com/aws/aws-cdk-rfcs/blob/master/text/0474-event-bridge-scheduler-l2.md

Also added a small fix to 2 of the unit tests of the previous PR for this module.

Advances #23394


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

@gitpod-io
Copy link

gitpod-io bot commented May 21, 2023

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels May 21, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team May 21, 2023 18:32
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.

packages/@aws-cdk/aws-scheduler-alpha/lib/input.ts Outdated Show resolved Hide resolved
Comment on lines 9 to 10
* Pass text to the target, it is possible to embed `ContextAttributes` and other
* cdk references.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we specify what is cdk references?

packages/@aws-cdk/aws-scheduler-alpha/test/input.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/input.ts Outdated Show resolved Hide resolved
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.

Minor comments, but I dont really have a problem with most of this

@Jacco
Copy link
Contributor Author

Jacco commented Jun 7, 2023

About at

This is from the AutoScaling module:

  /**
   * Construct a Schedule from a moment in time
   */
  public static at(moment: Date): Schedule {
    return new LiteralSchedule(`at(${formatISO(moment)})`);
  }

It implicitly converts the date passed in to UTC with the local timeZone (local to cdk). Probably also not very intuitive.

How about the following:

  public static at(localYear: number, localMonth: number, localDay: number, localHour: number, localMinute: number, localSeconds: int, timeZone?: TimeZone): ScheduleExpression {
   ...
  }

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

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.

Looks like there's still a few open comments to be addressed before this gets approved. I'll take another look next week.

packages/@aws-cdk/aws-scheduler-alpha/test/input.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/targets.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/schedule.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-scheduler-alpha/lib/schedule.ts Outdated Show resolved Hide resolved
Jacco and others added 7 commits June 16, 2023 11:44
Co-authored-by: Filipp Fediakov <fediakovphilip@gmail.com>
Co-authored-by: Filipp Fediakov <fediakovphilip@gmail.com>
Co-authored-by: Filipp Fediakov <fediakovphilip@gmail.com>
Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>
comment changed
…co/aws-cdk into feat(scheduler)--ScheduleTargetInput
@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Jun 19, 2023
@kaizencc kaizencc reopened this Jun 19, 2023
@kaizencc kaizencc added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. labels Jun 19, 2023
kaizencc
kaizencc previously approved these changes Jun 19, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review June 19, 2023 18:11

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

@mergify mergify bot dismissed kaizencc’s stale review June 19, 2023 20:38

Pull request has been modified.

@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 Jun 22, 2023
@Jacco Jacco requested a review from kaizencc June 23, 2023 19:07
kaizencc
kaizencc previously approved these changes Jun 29, 2023
@mergify
Copy link
Contributor

mergify bot commented Jun 29, 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).

@mergify mergify bot dismissed kaizencc’s stale review June 29, 2023 19:16

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Jun 29, 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).

@mergify mergify bot merged commit bc9f3de into aws:main Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants