Skip to content
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(msk-alpha): MSK Kafka versions 2.8.2.tiered and 3.5.1 and StorageMode property #27560

Merged
merged 36 commits into from
Dec 1, 2023

Conversation

chrispidcock
Copy link
Contributor

@chrispidcock chrispidcock commented Oct 16, 2023

This patch adds support for the tiered storage mode and Kafka versions 2.8.2.tiered & 3.5.1 in the aws-msk-alpha package.


Changes

  • added Kafka versions 2.8.2.tiered & 3.5.1.
  • added storageMode L1 construct property to the L2 msk
  • added unit and integ tests for Kafka versions and 'storageMode
  • updated test versions to latest supported Kafka version as desired

Ref:


Closes #27551


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@github-actions github-actions bot added feature-request A feature should be added or improved. p2 beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK labels Oct 16, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team October 16, 2023 14:21
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@chrispidcock chrispidcock changed the title Adding new msk kafka versions feat(aws-msk-alpha): MSK Kafka versions 2.8.2.tiered and 3.5.1 and StorageMode property Oct 17, 2023
@chrispidcock chrispidcock changed the title feat(aws-msk-alpha): MSK Kafka versions 2.8.2.tiered and 3.5.1 and StorageMode property feat(msk-alpha): MSK Kafka versions 2.8.2.tiered and 3.5.1 and StorageMode property Oct 17, 2023
@github-actions github-actions bot added the effort/small Small work item – less than a day of effort label Oct 18, 2023
@aws-cdk-automation aws-cdk-automation dismissed their stale review October 18, 2023 15:03

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

Copy link
Contributor

@lpizzinidev lpizzinidev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks 👍

@chrispidcock
Copy link
Contributor Author

Hello @lpizzinidev ,

I was wondering if you could please give me any advice on getting this PR reviewed by a maintainer. Is there anything further you could recommend I contribute or add to this PR?

Thank you for all your previous work to review my code!

Thanks

@lpizzinidev
Copy link
Contributor

Hi @chrispidcock
I'll reach out to some team members and see if they are available for a review.
Thanks for your patience 💪

packages/@aws-cdk/aws-msk-alpha/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk-alpha/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts Outdated Show resolved Hide resolved
kaizencc
kaizencc previously approved these changes Nov 17, 2023
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for opening this PR @chrispidcock and for reviewing @lpizzinidev. A couple of things:

  • I am approving this PR, but ideally these would be separated out into two different PRs since they introduce two completely separate features. There are a few benefits to this approach -- faster/easier reviews, each feature shows up individually in our changelog, etc. I am allowing this because it is already done. @lpizzinidev if you see something like this in a future PR you review, feel free to ask for a separation.
  • We won't be able to release this until after re:invent. Apologies there. Just want to make sure you're aware of the timeline @chrispidcock.

Copy link
Contributor Author

@chrispidcock chrispidcock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There looks to be this sneaky whitespace that is breaking the build.

packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-msk-alpha/lib/cluster.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed kaizencc’s stale review November 19, 2023 04:47

Pull request has been modified.

@chrispidcock chrispidcock force-pushed the adding-new-msk-kafka-versions branch from 08feb95 to e94f72c Compare November 24, 2023 14:51
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@chrispidcock
Copy link
Contributor Author

@kaizencc Hello,

I had some issues post your code review, and I was wondering if you could review again, please, as I had to make a change to satisfy the tests.

Thank you.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Copy link
Contributor

mergify bot commented Dec 1, 2023

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

@mergify mergify bot merged commit f9f15fa into aws:main Dec 1, 2023
10 checks passed
@chrispidcock
Copy link
Contributor Author

Thank you everyone for your help to get this through.

chenjane-dev pushed a commit to chenjane-dev/aws-cdk that referenced this pull request Dec 5, 2023
…eMode property (aws#27560)

This patch adds support for the `tiered` `storage mode` and Kafka versions `2.8.2.tiered` & `3.5.1` in the `aws-msk-alpha` package.

---

### Changes

- added Kafka versions `2.8.2.tiered` & `3.5.1`.
- added `storageMode` L1 construct property to the L2 msk
- added unit and integ tests for `Kafka versions and 'storageMode`
- updated test versions to latest supported Kafka version as desired

Ref:
- [aws: MSK supported Kafka versions](https://docs.aws.amazon.com/msk/latest/developerguide/supported-kafka-versions.html)

---

Closes aws#27551

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 pr/needs-maintainer-review This PR needs a review from a Core Team Member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(msk-alpha): Add Kafka versions 2.8.2.tiered and support for tiered storage mode
4 participants