-
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(stepfunctions-tasks): Support calling ApiGateway REST and HTTP APIs #13033
Conversation
awesome, thanks @ayush987goyal - I'll review it this week. |
Hi @shivlaks , quick reminder for this :) |
Hi @ayush987goyal , there is a conflict. Could you solve it? |
const httpApi = new apigatewayv2.HttpApi(stack, 'HttpApi'); | ||
|
||
// WHEN | ||
const task = new InvokeApiGatewayHttpApi(stack, 'Invoke', { |
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.
Bookmarks to see if there's a way to grant permissions in the case of the API route having AWS_IAM auth on it
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.
Yep; looks like the props here take an authType that can be set to AuthType.IAM_ROLE
Look forward to putting this to use when it lands
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.
@ayush987goyal thanks for picking this one up! looking good. left some minor feedback.
would also love if @nija-at could take a look as the primary keeper of the API Gateway modules
packages/@aws-cdk/aws-stepfunctions-tasks/lib/apigateway/base-types.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/apigateway/base.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/apigateway/base-types.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/apigateway/base.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/apigateway/base.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/apigateway/base.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/apigateway/base.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/aws-stepfunctions-tasks/lib/apigateway/base.ts
Outdated
Show resolved
Hide resolved
@shivlaks I think this change is ready to be closed now. Could you please take a look? |
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.
@ayush987goyal - LGTM!! added a do-not-merge
as I'd love for @nija-at to take a look as well, primarily because these are new stable APIs and he reviewed the predecessor PR :)
great work!! 🎉
@nija-at We meet again so soon! :P |
* | ||
* @see https://docs.aws.amazon.com/step-functions/latest/dg/connect-api-gateway.html | ||
*/ | ||
export class InvokeApiGatewayHttpApi extends InvokeApiGatewayApiBase { |
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.
We use the word Invoke
in the context of lambda because that's intuitive to lambda and is the official term for its operation. Should we find a different word here, say Call
? (This is what the official docs call it)
Why have you chosen to separate these into two different classes? How would it be if this was modified to a single class, say CallApiGatewayEndpoint
, which takes two optional properties of types IHttpApi
and IRestApi
and enforces one, and only one, of them is provided?
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 agree on using
Call
instead ofInvoke
. - I implemented it this way to make the separation explicit so that if in future there is a way to call WebSocket API as well (which I believe might have different way compared to these two), we have clear separation and callers will not get confused. Given that, I am still okay with implementing a single class with validations around the passed props. Let me know your thoughts @nija-at @shivlaks
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.
- Another point for separation could be that some props (eg. stageName) can be optional for one but required for other. Also, some prop might not even be supported for one of them.
feat(stepfunctions-tasks): Support calling APIGW REST and HTTP APIs
Taking ownership of the original PR #11565 by @Sumeet-Badyal
API as per documentation here:
https://docs.aws.amazon.com/step-functions/latest/dg/connect-api-gateway.html
https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-control-access-using-iam-policies-to-invoke-api.html
closes #11566
closes #11565
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license