-
Notifications
You must be signed in to change notification settings - Fork 198
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: adding support for karpenter blockdevicemapping #890
feat: adding support for karpenter blockdevicemapping #890
Conversation
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.
@paulmowat thank you for this contribution. Please see a couple relatively minor comments.
lib/addons/karpenter/index.ts
Outdated
noDevice?: string; | ||
} | ||
|
||
interface Ebs { |
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.
Ebs is a bit too bold for this interface as it appears to represent ElasticBlockStorage as a service. How about either "EbsVolumeSettings|
EbsVolumeMappingsor just
EbsMappings`?
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 feedback. I've renamed to EBSVolumeMapping
lib/addons/karpenter/index.ts
Outdated
|
||
interface BlockDeviceMapping { |
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.
These should be exported along with the KarpenterAddOnProps
(not part of the PR atm) as customers will have issue consuming these otherwise. Let's add export
keyword to all public data types.
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 feedback. I've exported all the interfaces (including KarpenterAddOnProps) and now use these as part of the unit test.
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.
LGTM
Validated with unit tests, e2e will be used with the upgraded versions of helm/cdk in the 1.13 release prep. |
Description of changes:
Adding in support to pass blockDeviceMappings through to karpenter as per https://karpenter.sh/v0.30/concepts/node-templates/#specblockdevicemappings
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.