The code change adds new versions to the already existing/ supported Kafka versions.

• The supported versions were not mentioned in the README so far - I decided to not start with this.
• The unit/ integration tests all use the default version 2.6.1 of Kafka. I thought about updating those tests, but skipped. Happy to apply a change (new PR or as part of this).
  Nevertheless, if we go with the default/ recommended version it would be in my opinion 2.8.1 - the newly introduced versions would also not be covered by this.

Please let me know your thoughts.

The code change adds new versions to the already existing/ supported Kafka versions.

• The supported versions were not mentioned in the README so far - I decided to not start with this.
• The unit/ integration tests all use the default version 2.6.1 of Kafka. I thought about updating those tests, but skipped. Happy to apply a change (new PR or as part of this).
  Nevertheless, if we go with the default/ recommended version it would be in my opinion 2.8.1 - the newly introduced versions would also not be covered by this.

Please let me know your thoughts.

Thank you for your contribution!

I've made a README exemption. I don't think that it's necessary in this case.

It's unfortunate that we have a testing gap here that we didn't notice, but I'd like to not increase that gap. I'd like to see test cases added for the new versions (not replacing the old ones). If running integ tests is a blocker for any reason, please let us know and we can run them once the test cases themselves are written.

Hi @TheRealAmazonKendra !
I tried to come up with a unit test for the Kafka versions. To my understanding we would need to check/ensure that msk.KafkaVersion.VX_Y_Z contains always the correct version as string, e.g. msk.KafkaVersion.V3_2_0 -> '3.2.0'.
Not sure if a parameterized test is what you had in mind, but its a starting point for discussion.
(ok, noticed that there is a different flavor for the syntax around test.each - will have a look)

On the integration test I am unsure about the expectation. I noticed that three clusters with Kafka version 2.8.1 are generated, all with a different way for e.g. authentication. Would i need to setup an additional cluster with the newer Kafka versions? Or three because of the different authentication mechanisms?

@TheRealAmazonKendra I was able to run the integration tests in an AWS account and notices that it creates a Private CA - which is quite costly. Is there any information how contributors typically deal with this kind of costs?

Hi @TheRealAmazonKendra ! I tried to come up with a unit test for the Kafka versions. To my understanding we would need to check/ensure that msk.KafkaVersion.VX_Y_Z contains always the correct version as string, e.g. msk.KafkaVersion.V3_2_0 -> '3.2.0'. Not sure if a parameterized test is what you had in mind, but its a starting point for discussion. (ok, noticed that there is a different flavor for the syntax around test.each - will have a look)

On the integration test I am unsure about the expectation. I noticed that three clusters with Kafka version 2.8.1 are generated, all with a different way for e.g. authentication. Would i need to setup an additional cluster with the newer Kafka versions? Or three because of the different authentication mechanisms?

Apologies for the delay, I missed the notification last week. Just one for each. We just want to see that it deploys.

@TheRealAmazonKendra I was able to run the integration tests in an AWS account and notices that it creates a Private CA - which is quite costly. Is there any information how contributors typically deal with this kind of costs?

Yes, and we need to make the documentation better because this question comes up frequently so I apologize for it not being more clear. Just write the test cases and commit them. Then tag me with a note to run the tests and update the snapshots for you.

Hello @TheRealAmazonKendra! As discussed I updated the integration test code and added three MSK clusters, one for each version added. Please let me know if I need to change/ update.

@Mergifyio update

update

❌ Base branch update has failed

Well, this exercise was worthwhile because I found that our test was actually broken. I've pushed the fix and the updated snapshots. As long as this passes, I see it as good to go.

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

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: 0b293fe
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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).