-
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(step-functions): add bucketNamePath in item reader #31619
Conversation
**Reason for this change** - For `DistributedMap` state of StepFunctions, `IItemReader` currently only allows S3 bucket as input source to be declared statically in CDK. - In other words, current CDK implementation only caters to static use-case where we know either `bucket` or `bucketName` (from which we can create `IBucket`) and pass it to `IItemReader`. - Whereas via AWS management console, if we create `DistributedMap` manually, then we can also convey S3 source dynamically using State Input / JsonPath. - In other words, for dynamic use-case, we will neither have `bucket` nor `bucketName` i.e. we only know state input variable which will convey `bucketName` e.g. `$.bucketName`. - So, if we want to use `IItemReader` for dynamic use case also, then, to avoid making breaking change (e.g. changing type of `bucket` from `IBucket` to `string`), we will: - (1) need to make `bucket: IBucket` an optional prop in `ItemReaderProps` (refer [Making properties optional](https://github.com/aws/jsii/blob/main/packages/jsii-diff/BREAKING_CHANGES.md#making-properties-optional)) - (2) and add another optional field `bucketNamePath: string` to convey state input variable name (e.g. $.bucketName) **What changes are being made?** - Add `bucketNamePath` as optional prop in `ItemReaderProps`. - Make `bucket` an optional prop instead of required prop in `ItemReaderProps`. - Adapt implementing classes of `IItemReader` to handle `bucket` being optional (refer [Making properties optional](https://github.com/aws/jsii/blob/main/packages/jsii-diff/BREAKING_CHANGES.md#making-properties-optional)). - Add `validateItemReader` in `IItemReader` which implementing classes will implement to handle mutual exclusivity of `bucket` and `bucketNamePath`. - Modify `DistributedMap:validateState` to validate `IItemReader` if present. - Modify README to explain and add examples relevant to changes. - Add new unit-tests for `DistributedMap`. - Add new integration tests to confirm vali **How are these changes tested?** - Build changes: `cd ./packages/aws-cdk-lib/ && yarn build aws-stepfunctions && yarn watch` - Add and run new unit-tests for `DistributedMap`: `yarn test aws-stepfunctions/test` - Run `yarn rossetta:extract -d aws-stepfunctions -v` to confirm validity of README changes - Add new integration test and run (with snapshot created): - Build once and watch: `npx lerna run build --scope=@aws-cdk-testing/framework-integ && yarn watch` - Test: `yarn build --directory test/aws-stepfunctions/test && yarn integ test/aws-stepfunctions/test/integ.item-reader-path-s3-object.js --update-on-failed` - Verified expected step function execution result during snapshot creation
Note: Changes of this PR were suggested as part of discussion from #30836. |
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 the PR. I recalled when we first discussed about this issue, I mentioned that making a property from required to optional is a breaking change. As I discussed more with the team, loosening the restriction can be ok (ideally we want to avoid it if possible). In this scenario, I think this is a valid reason to make it optional.
I think this solution is fine with me. I'll leave it to Mohamed for a final review. Meanwhile I'll close the old PR since that approach is abandoned.
packages/aws-cdk-lib/aws-stepfunctions/lib/states/distributed-map/item-reader.ts
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions/lib/states/distributed-map/item-reader.ts
Show resolved
Hide resolved
packages/aws-cdk-lib/aws-stepfunctions/lib/states/distributed-map/item-reader.ts
Outdated
Show resolved
Hide resolved
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.
in general the change looks good to me, I left some minor comments
Pull request has been modified.
- for IItemReader, if bucket is not given, then policy statement can allow step function to list any bucket.
packages/aws-cdk-lib/aws-stepfunctions/lib/states/distributed-map/item-reader.ts
Show resolved
Hide resolved
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). |
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). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #29409.
Reason for this change
DistributedMap
state of StepFunctions,IItemReader
currently only allows S3 bucket as input source to be declared statically in CDK.bucket
orbucketName
(from which we can createIBucket
) and pass it toIItemReader
.DistributedMap
manually, then we can also convey S3 source dynamically using State Input / JsonPath.bucket
norbucketName
i.e. we only know state input variable which will conveybucketName
e.g.$.bucketName
.IItemReader
for dynamic use case also, then, to avoid making breaking change (e.g. changing type ofbucket
fromIBucket
tostring
), we will:bucket: IBucket
an optional prop inItemReaderProps
(refer Making properties optional)bucketNamePath: string
to convey state input variable name (e.g. $.bucketName)Description of changes
bucketNamePath
as optional prop inItemReaderProps
.bucket
an optional prop instead of required prop inItemReaderProps
.IItemReader
to handlebucket
being optional (refer Making properties optional).validateItemReader
inIItemReader
which implementing classes will implement to handle mutual exclusivity ofbucket
andbucketNamePath
.DistributedMap:validateState
to validateIItemReader
if present.DistributedMap
.Description of how you validated changes
cd ./packages/aws-cdk-lib/ && yarn build aws-stepfunctions && yarn watch
DistributedMap
:yarn test aws-stepfunctions/test
yarn rossetta:extract -d aws-stepfunctions -v
to confirm validity of README changesnpx lerna run build --scope=@aws-cdk-testing/framework-integ && yarn watch
yarn build --directory test/aws-stepfunctions/test && yarn integ test/aws-stepfunctions/test/integ.item-reader-path-s3-object.js --update-on-failed
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license