Conversation
shapirov103
left a comment
shapirov103
left a comment
There was a problem hiding this comment.
added a few high level comments. Overall, I like the structure that you proposed, it is consistent with the approaches currently applied. There are some trade-offs here and there that I brought up.
| import { CfnCapability } from "aws-cdk-lib/aws-eks"; | ||
| import { ClusterInfo } from "./types"; | ||
|
|
||
| export declare interface ClusterCapability { |
There was a problem hiding this comment.
let's add some jsdoc for the interface and each public method - it is more relevant for anything in the contract.
There was a problem hiding this comment.
Let's add capability to the docs/core-concepts.md since it is in the SPI
| this.clusterInfo.addProvisionedAddOn(addOnKeys[index], construct); | ||
| }); | ||
|
|
||
| if (blueprintProps.capabilities != null) { |
There was a problem hiding this comment.
about this specific placement: do you envision that capabilities must be provisioned BEFORE the addons and teams are applied? If yes, what provisions to you plan to ensure this?
For example, one approach could be to move the into the cluster provider (like V2). Then cluster provider organizes the dependencies in a particular way to ensure that by the time addons are applied the capabilities are there.
Another approach is to have it the way it is now - consistent with how addons are implemented. However, some dependency management logic can apply to make sure addons follow capabilities.
There was a problem hiding this comment.
Hmm, I think it makes sense that capabilities should be deployed first. It shouldn't really matter at the moment, but if we want to be able to support gitops approach to addons with the argocd capability we might have to make it more direct
lib/spi/capability-contracts.ts
Outdated
| import { ClusterInfo } from "./types"; | ||
|
|
||
| export declare interface ClusterCapability { | ||
| create(clusterInfo: ClusterInfo): CfnCapability |
There was a problem hiding this comment.
We don't have L2 support for capabilities yet, but is there any reason why we would expose specifically CfnCapability here vs let's something generic like IConstruct? Do you envision that customers will need to know anything about the internal structure of this returned object other than - it is a construct and I can depend on it?
Making it more generic can allow easier support for capabilities in the future if L2 support comes along. it is a tradeoff, but as a rule - do not expose more than necessary about internal approach.
There was a problem hiding this comment.
My thought process was that any capability will always return a Cfn Capability, otherwise it could basically be instantiated as an addon
|
|
||
| if (blueprintProps.capabilities != null) { | ||
| for (let capability of blueprintProps.capabilities) { | ||
| capability.create(this.clusterInfo); |
There was a problem hiding this comment.
with addons we keep track of them in the cluster info. If I want to check whether a particular capability is there - what is my approach from within an addon?
There was a problem hiding this comment.
I will do this, this way I can also ensure that addons + capabilities that do the same thing don't accidentally get deployed
zjaco13
force-pushed
the
feat/capabilities
branch
from
33220fb to
4d217d3
Compare
2 participants
Issue #, if available:
#1216
Description of changes:
Adds a new module for creating EKS Capabilities as part of the framework. Available to customers as a part of EKS Builder
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.