-
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-targets): step function start execution target #27424
Changes from all commits
24416f9
ae2e8ae
6062a8d
d3302f0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,2 +1,3 @@ | ||
export * from './target'; | ||
export * from './lambda-invoke'; | ||
export * from './lambda-invoke'; | ||
export * from './stepfunctions-start-execution'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
import { IScheduleTarget, ISchedule } from '@aws-cdk/aws-scheduler-alpha'; | ||
import { Names } from 'aws-cdk-lib'; | ||
import { IRole } from 'aws-cdk-lib/aws-iam'; | ||
import { IStateMachine } from 'aws-cdk-lib/aws-stepfunctions'; | ||
import { ScheduleTargetBase, ScheduleTargetBaseProps } from './target'; | ||
import { sameEnvDimension } from './util'; | ||
|
||
/** | ||
* Use an AWS Step function as a target for AWS EventBridge Scheduler. | ||
*/ | ||
export class StepFunctionsStartExecution extends ScheduleTargetBase implements IScheduleTarget { | ||
constructor( | ||
private readonly stateMachine: IStateMachine, | ||
private readonly props: ScheduleTargetBaseProps, | ||
) { | ||
super(props, stateMachine.stateMachineArn); | ||
} | ||
|
||
protected addTargetActionToRole(schedule: ISchedule, role: IRole): void { | ||
const stateMachineEnv = this.stateMachine.env; | ||
if (!sameEnvDimension(stateMachineEnv.region, schedule.env.region)) { | ||
throw new Error(`Cannot assign stateMachine in region ${stateMachineEnv.region} to the schedule ${Names.nodeUniqueId(schedule.node)} in region ${schedule.env.region}. Both the schedule and the stateMachine must be in the same region.`); | ||
} | ||
|
||
if (!sameEnvDimension(stateMachineEnv.account, schedule.env.account)) { | ||
throw new Error(`Cannot assign stateMachine in account ${stateMachineEnv.account} to the schedule ${Names.nodeUniqueId(schedule.node)} in account ${schedule.env.region}. Both the schedule and the stateMachine must be in the same account.`); | ||
} | ||
|
||
if (this.props.role && !sameEnvDimension(this.props.role.env.account, stateMachineEnv.account)) { | ||
throw new Error(`Cannot grant permission to execution role in account ${this.props.role.env.account} to invoke target ${Names.nodeUniqueId(this.stateMachine.node)} in account ${stateMachineEnv.account}. Both the target and the execution role must be in the same account.`); | ||
} | ||
Comment on lines
+20
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might make sense to extract this common logic block somewhere (since its the same for step functions and lambda and probably some targets more, except the message), do you have a favorite spot for it? Should it go to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kaizencc do you have any opinion on this? I personally do not mind the duplication of the code There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind it for now, but would be happy with less duplication :). At best we can create an issue for it and be able to point to it in the future if this gets out of hand |
||
|
||
this.stateMachine.grantStartExecution(role); | ||
} | ||
} |
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 would recommend to add unit tests for this target too. See https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-scheduler-targets-alpha/test/target.test.ts#L9. We probably need to rename that test file and create one file with unit tests per each target.
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 created a new unit test which contains almost identical unit tests for step functions.
I also renamed the existing file
target.test.ts
intolambda-invoke.test.ts
.