fix(aws-lambda-event-sources): unsupported properties for SelfManagedKafkaEventSource and ManagedKafkaEventSource #17965
Conversation
…KafkaEventSource and ManagedKafkaEventSource
|
|
|
@kaizen3031593 not trying to interrupt or stress you, but is there anything i can do to help you review this? :) |
There was a problem hiding this comment.
This looks great @zehsor! I'm fine with the breaking change (yes, it is) because it never would have worked in the first place. And no need to test -- I'll mark the PR as exempt.
I have a few nits involving new lines that I normally would go in and do myself, but you haven't configured the PR to allow me to do this (next time, try PRing from a branch of your fork and the default permissions should allow maintainer contributions). Since I can't go in and change them myself, I'm going to bother you and have you remove them. Then, I'll approve the PR. Thanks for the contribution!
kaizencc
added
the
pr-linter/exempt-test
|
Thanks @kaizen3031593 for the great feedback. I think the formatting issue happened due to trying out different things in the beginning, sorry for that. As i usually work with gitlab i learned something new about github pull requests, will follow your instructions next time :) |
|
Thank you for contributing! Your pull request will be updated from master 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 master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
…KafkaEventSource and ManagedKafkaEventSource (aws#17965) This PR fixes a bug in the CDK where some `kafkaEventSource` properties are actually unsupported. These properties exist only for kinesis and dynamodb streams. The existing KafkaEventSourceProps Interface erroneously extends an interface that includes kinesis and dynamodb specific properties. This PR separates these properties into a `Base` interface with shared stream properties for all 3, as well as an interface for `kinesis` and `dynamodb` specific properties. Unit testing unavailable because the scope of the PR is to remove properties. It is enough to ensure that current tests still succeed. We are allowing the breaking changes specified in `allowed-breaking-changes.txt` because they never worked in the first place. Fixes aws#17934. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…EventSources (#19995) Closes #19917. Seems this was missed in a previous fix: #17965. Adding breaking changes to `allowed-breaking-changes.txt` because they never worked in the first place. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
…EventSources (aws#19995) Closes aws#19917. Seems this was missed in a previous fix: aws#17965. Adding breaking changes to `allowed-breaking-changes.txt` because they never worked in the first place. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This PR fixes a bug in the CDK where some
kafkaEventSourceproperties are actually unsupported. These properties exist only for kinesis and dynamodb streams. The existing KafkaEventSourceProps Interface erroneously extends an interface that includes kinesis and dynamodb specific properties. This PR separates these properties into aBaseinterface with shared stream properties for all 3, as well as an interface forkinesisanddynamodbspecific properties.Unit testing unavailable because the scope of the PR is to remove properties. It is enough to ensure that current tests still succeed.
We are allowing the breaking changes specified in
allowed-breaking-changes.txtbecause they never worked in the first place.Fixes #17934.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license