-
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
fix(stepfunctions): maxConcurrency does not support JsonPath #29330
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.
Some minor comments. LGTM overall
packages/@aws-cdk-testing/framework-integ/test/aws-stepfunctions/test/integ.map-distributed.ts
Show resolved
Hide resolved
Co-authored-by: Adam Wong <55506708+wong-a@users.noreply.github.com>
…ws-cdk into maxConcurrencyJsonPath
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 the PR, changes look good!
This does not invalidate JsonPaths for maxConcurrency, as I'm unsure how to do so without reverting #20279 . Open to suggestions
I'd suggest the same thing Kaizen suggested here and throw a descriptive error when we detect a JSON path in maxConcurrency. Should be backwards compatible as the deployment fails.
EDIT: will deal with JSON path in maxConcurrency
separately. Merging this PR for now so that people with the same issue as #20835 have a workaround
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). |
Issue # (if applicable)
Relates to #20835
Reason for this change
MaxConcurrency
does not supportJsonPath
. This change addsMaxConcurrencyPath
so that CDK users can specify aJsonPath
for theirMaxConcurrency
Note : This does not invalidate JsonPaths for
MaxConcurrency
, as I'm unsure how to do so without reverting #20279 . Open to suggestionsDescription of changes
Added a new
maxConcurrencyPath
field that accepts aJsonPath
value. Decided to go with another explicit field as it is similar to what is done forErrorPath
andCausePath
, in addition to most other Path fieldsDescription of how you validated changes
Added unit tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license