feat(step-functions): add bucketNamePath in item reader#31619
feat(step-functions): add bucketNamePath in item reader#31619mergify[bot] merged 5 commits intoaws:mainfrom ChakshuGupta13:modify_item_reader
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.
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
moelasmar
left a comment
There was a problem hiding this comment.
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
DistributedMapstate of StepFunctions,IItemReadercurrently only allows S3 bucket as input source to be declared statically in CDK.bucketorbucketName(from which we can createIBucket) and pass it toIItemReader.DistributedMapmanually, then we can also convey S3 source dynamically using State Input / JsonPath.bucketnorbucketNamei.e. we only know state input variable which will conveybucketNamee.g.$.bucketName.IItemReaderfor dynamic use case also, then, to avoid making breaking change (e.g. changing type ofbucketfromIBuckettostring), we will:bucket: IBucketan optional prop inItemReaderProps(refer Making properties optional)bucketNamePath: stringto convey state input variable name (e.g. $.bucketName)Description of changes
bucketNamePathas optional prop inItemReaderProps.bucketan optional prop instead of required prop inItemReaderProps.IItemReaderto handlebucketbeing optional (refer Making properties optional).validateItemReaderinIItemReaderwhich implementing classes will implement to handle mutual exclusivity ofbucketandbucketNamePath.DistributedMap:validateStateto validateIItemReaderif present.DistributedMap.Description of how you validated changes
cd ./packages/aws-cdk-lib/ && yarn build aws-stepfunctions && yarn watchDistributedMap:yarn test aws-stepfunctions/testyarn rossetta:extract -d aws-stepfunctions -vto confirm validity of README changesnpx lerna run build --scope=@aws-cdk-testing/framework-integ && yarn watchyarn build --directory test/aws-stepfunctions/test && yarn integ test/aws-stepfunctions/test/integ.item-reader-path-s3-object.js --update-on-failedChecklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license