-
Notifications
You must be signed in to change notification settings - Fork 4.2k
fix(stepfunctions-tasks): allow passing apiEndpoint as intrinsic function (under feature flag) #32139
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
base: main
Are you sure you want to change the base?
fix(stepfunctions-tasks): allow passing apiEndpoint as intrinsic function (under feature flag) #32139
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32139 +/- ##
=======================================
Coverage 82.38% 82.38%
=======================================
Files 120 120
Lines 6937 6937
Branches 1170 1170
=======================================
Hits 5715 5715
Misses 1119 1119
Partials 103 103
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
de76858
to
b278145
Compare
Thanks for your help in getting these changes to green @ePak. I think it was the |
This PR has been in the MERGE CONFLICTS 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. |
…mic values in the path. Update README entry with more complex example.
c512f0e
to
2d3c186
Compare
I'm not sure what I'm supposed to do about generated snapshot files failing CodeQL checks. |
Workaround with JSONata in the mean time, tested to work with CDK 2.178.1 thanks to #28673 and #32343:
|
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.
Left a question.
Additionally, I see that this fix changes all the apiEndpoint
values to use State.Format(..)
. I am wondering if this may push some existing Step Function Definition over the length limit, hence, causing a breaking change to those CDK users. I understand that this may be a very edge case but I would like to get more input from the CDK team to see if we need a feature flag for this fix. I will get back asap. Thank you.
@@ -80,7 +84,7 @@ describe('AWS::StepFunctions::Tasks::HttpInvoke', () => { | |||
}, | |||
End: true, | |||
Arguments: { | |||
ApiEndpoint: 'https://api.example.com/path/to/resource', | |||
ApiEndpoint: "States.Format('{}/{}', 'https://api.example.com', 'path/to/resource')", |
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 see on line 72 above that this state definition uses JSONata
. Does States.Format(...)
work with JSONata
?
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'm not sure about how JSONata
works, as that was all added in while I was trying to get the original fix in. Some brief research suggests it would not be compatible.
JsonPath and Jsonata should probably be handled separately in the construct, but that jsonPath
static method was already added to just call the existing constructor, so it can't even be used for this fix now. Maybe the existing logic can be reworked without breaking newly-existing functionality.
Hi @MathieuGilbert, I have consulted other members in the CDK team. We think that this fix requires a feature flag because the current approach of the fix will introduce changes to existing CDK stacks and potentially cause problems. Other than the length limit problem, for stacks without this fix, the To move forward, I suggest implementing the fix behind feature flag. Thank you for looking into this bug and making contributions to the CDK repo. |
Thanks for looking at this @samson-keung. I'm not sure how much more effort I can put towards this. This is my 3rd PR since July trying to get the fix in, and attempting to contribute has been a very discouraging experience overall. If I can justify the extra hours, I'll try figuring out feature flags, address the issue exposed by that |
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.
@MathieuGilbert Understandable. We are currently improving our workflow to better track PRs that have been for a review for a long time.
For now, I will mark this PR as "Request changes" for tracking purposes. If there are team members available, we can pick this up.
Appreciate your effort on this. I am happy to take notes on what is most discouraging you from contributing if you want to share your thoughts. I cannot make guarantees but I will think about what I can do.
Thanks again!
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 14 days if no action is taken. |
…tpinvoke-format-apiendpoint-integ
…ecific test case for when the flag is off.
…cking the format of the apiRoot and apiEndpoint props, to the tasks queryLanguage. This prevents it from treating it as JSONPath when neither prop is actually a JSONata expression.
a42d5ce
to
ded392f
Compare
Pull request has been modified.
@samson-keung I guess a big part was around working with the integration test framework and the snapshots. I was relying more on other tests as examples than the documentation, and then I kept getting stuck in CI with failing snapshot tests with no clear way forward. I'm still not clear whether it's even possible to update an integration test, or we have to always create a new one. |
…tpinvoke-format-apiendpoint-integ
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 requeue |
✅ The queue state of this pull request has been cleaned. It can be re-embarked automatically |
@Mergifyio rebase |
☑️ Nothing to do, the required conditions are not met
|
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.
Hi @MathieuGilbert ! Apologies for the delay, it seems @Mergifyio has not merged your PR. Not sure why it silently failed. Currently, the PR has merge conflicts that will need to fixed in order to get it merged.
@Mergifyio update |
☑️ Nothing to do, the required conditions are not met
|
…tpinvoke-format-apiendpoint-integ
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #29925
Closes #29925, #30749.
Reason for this change
The
apiEndpoint
prop currently only works when it's a string (ie.TaskInput.fromText('some/text')
), with the task failing when passed as a reference (ie.TaskInput.fromText(JsonPath.format('some/text/{}', '123')
). This is needed to allow for dynamic parts in the path.Description of changes
ApiEndpoint
task parameter to use theJsonPath.format
intrinsic function to combine theapiRoot
andapiEndpoint
props, instead of basic string concatenation.Description of how you validated changes
fromJsonPathAt
for the endpoint.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license