-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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): add httpinvoke step functions task #28673
feat(stepfunctions-tasks): add httpinvoke step functions task #28673
Conversation
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.
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.
Add task policies: events:RetrieveConnectionCredentials, secretsmanager:GetSecretValue, secretsmanager:DescribeSecret, states:InvokeHTTPEndpoint.
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. |
…t. Test creates an API Gateway endpoint with basic auth Connection, then uses the connection in its task to invoke the endpoint.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
… template for README compilation.
…o Rosetta fixture gives duplicate import errors on them. Add SecretValue as core import with others in template.
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.
Great stuff. Mostly nits on formatting.
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.
Thank you for your contribution! This looks great overall but I just have one suggested change (in addition to the feedback already given).
* | ||
* @default - ArrayEncodingFormat.INDICES | ||
*/ | ||
readonly arrayEncodingFormat?: ArrayEncodingFormat; |
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.
Given that this field is reliant on urlEncodeBody
being set, these two fields should probably be one field. Maybe the only field here should still be called urlEncodeBody
but have it take an Encoding
(not sure about this name) class with a static function that has an optional input of ArrayEncodingFormat
. Something along the lines of
urlEncodeBody: Encoding.set(ArrayEncodingFormat.INDICES);
to include encoding format, or
urlEncodeBody: Encoding.set();
to just include encoding with the default format.
This way, if they leave off this prop altogether, it's defaulted to false
.
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'll also add the note here that set
probably isn't a great function name here.
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.
Thanks for pointing this out, I agree it's a bit awkward with the 2 params. I played around a bit, and I think a single prop can work with a couple extra enum options and renaming it to urlEncodingFormat
. You can not pass it or specify URLEncodingFormat.NONE
to get no encoding, pass it as URLEncodingFormat.DEFAULT
to get encoding with the default array encoding (the underlying service defaults to INDICES
), or specify the existing values to use those.
…. Rename enum to URLEncodingFormat and add DEFAULT and NONE options. Restructure the buildTaskParameters method for clarity with the new logic, and define an interface for it. Add specific test case for NONE.
Pull request has been modified.
@msambol @TheRealAmazonKendra Thanks for reviewing, I think I've addressed all of your comments. Please let me know if there's anything else! |
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.
Thank you for all your work on this!
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). |
@Mergifyio update |
❌ Mergify doesn't have permission to updateFor security reasons, Mergify can't update this pull request. Try updating locally. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
This adds an
HttpInvoke
Step Functions task construct, which allows calling public APIs as described here.IConnection
, setting required permissions to use its credentials.apiRoot
prop). This allows passing the relative endpoint path at execution time (apiEndpoint
prop).Closes #28278
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license