feat(eks): Support cdk8s charts #10562
Conversation
|
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). |
|
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 |
| "fs-extra": "^9.0.1", | ||
| "pkglint": "0.0.0", | ||
| "ts-node": "^9.0.0", | ||
| "typescript": "~3.8.3", | ||
| "ubergen": "0.0.0" | ||
| }, | ||
| "peerDependencies": { | ||
| "constructs": "^3.0.4" | ||
| "constructs": "^3.0.4", | ||
| "cdk8s": "^0.30.0" |
There was a problem hiding this comment.
That's a very good point. Technically they will only need this if they use this feature but that's not something we can express through regular peer deps. @iliapolo Thoughts?
There was a problem hiding this comment.
Here's a trick that we might be able to use: I wonder if we should not advertise this as a peer dependency but rather just document that it's required in the closure.
There was a problem hiding this comment.
Do you mean just for monocdk? Might require some build twicks because its omission is causing a build failure. We can try it though, agree it will be a better experience.
And in case it's not going to solve this quirk for v2 where that dependency will actually be needed to compile the code...I think for v2 thats just a hit we will have to take, one of the disadvantages of having a monolithic package.
There was a problem hiding this comment.
I would try to list it as a buildDependency and exclude the eslint rule.
I don't think we can take the hit for monocdk. We need a solution for this type of "opt in peer". Npm has a concept of "optionalDependencies" which might be what we need
There was a problem hiding this comment.
I'll give it a try
There was a problem hiding this comment.
“I do not like it, I do not like it at all; I do not like it as a peer dependency, I do not like it as dev dependency”
There was a problem hiding this comment.
Just to make sure we are all in agreement on the meaning of adding this as a peer-dependency (or any type of dependency which is requires the user to supply it). This means that the EKS module will always have to support the minimal version of cdk8s declared in monocdk, if not, it will be considered a breaking change for monocdk customers. Might be worth considering extracting the APIs to a separate package
There was a problem hiding this comment.
It's a little late now but I 100 million % agree with Neta.
It can't be an optionalDependency: cdk8s.Chart must be downloaded and exist to even COMPILE this, and to typecheck the downstream .d.ts files.
The only feasible solution is to use IoC and an integration package:
aws-eks : IKubeDefinition
aws-eks-cdk8s: class Cdk8sDefinition implements IKubeDefinition
Natively integrate EKS with the
cdk8slibrary.Closes #9670
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license