feat(aws-eks): helm chart from asset by plumdog · Pull Request #13496 · aws/aws-cdk

plumdog · 2021-03-09T15:52:40Z

This is my attempt to make that work. I think the API is a bit less nice that described in #9273 (comment) but requires less code and thus less decisions for me to make.

The permissions juggling is the bit I like least, but what I've landed on appears to work.

~~I haven't attempt to write any tests yet, as I'm not yet convinced I'm heading in the right direction.~~ I have added a test to make CI happy. I suspect the test could be made less ugly.

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

gitpod-io · 2021-03-09T15:52:42Z

mergify · 2021-03-09T15:56:49Z

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

aws-cdk-automation · 2021-03-16T10:37:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: d48d326
Result: FAILED
Build Logs (available for 30 days)

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

iliapolo

Pretty cool 👍

iliapolo · 2021-05-17T20:09:04Z

packages/@aws-cdk/aws-eks/lib/helm-chart.ts

+   * The name of the chart or an Asset.
   */
-  readonly chart: string;
+  readonly chart: string | Asset;


We don't use union types since this translate poorly in other languages (namely Java and C#).

Instead, you'll have to create a different property called chartAsset (or something like that

iliapolo · 2021-05-17T20:10:16Z

packages/@aws-cdk/aws-eks/lib/helm-chart.ts

+    let chartAssetPolicy: Policy | undefined = undefined;
+
+    if (typeof props.chart !== 'string') {
+      // Use of props.chart.grantRead(provider.handlerRole) causes a


Can you elaborate please?

iliapolo · 2021-05-17T20:12:41Z

packages/@aws-cdk/aws-eks/test/test.helm-chart.ts

      expect(stack).to(haveResource(eks.HelmChart.RESOURCE_TYPE, { Release: 'hismostprobablylongerthanfiftythreecharacterscaf15d09' }));
      test.done();
    },
+    'should handle chart from S3 asset'(test: Test) {


This also deserves an integ test. Please add add a case to https://github.com/aws/aws-cdk/blob/master/packages/@aws-cdk/aws-eks/test/integ.eks-cluster.ts

iliapolo · 2021-05-17T20:13:03Z

packages/@aws-cdk/aws-eks/README.md


+Additionally, the `chart` property can be an `aws-s3-assets.Asset`,
+allowing use of local, private charts.
+


Add a code example

iliapolo

Mistakenly clicked on Comment instead of Request changes

github-actions · 2021-05-18T06:07:07Z

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

plumdog · 2021-05-18T06:24:32Z

Do not close.

@iliapolo thanks for your feedback, I hope I'll be able to make the changes soon.

pradoz · 2021-07-15T03:35:30Z

Any updates on this? It seems like the related issues #10247 and #10421 have been closed and the last comment on #9273 was the PR opened here.

plumdog · 2021-07-16T08:02:01Z

I need to make the code review changes, and resolve the conflicts. However, I won't be able to do this for some time (weeks/months maybe) so I'm happy for anyone to take this PR and progress it if they wish.

pradoz · 2021-07-16T15:55:07Z

I need to make the code review changes, and resolve the conflicts. However, I won't be able to do this for some time (weeks/months maybe) so I'm happy for anyone to take this PR and progress it if they wish.

Thanks for the reply and foundation you've laid so far!

I started poking around last night. I'm a noob and this would be my first major open source contribution - I've been working with CDK for <2 months. If you have any advice, specifically related to ideas you've thought about since your last attempt/commit, but have not had the time to implement. Currently, I was looking into implementing it as a chartAsset as @iliapolo suggested.

Things I may need help on:

solve the chart vs chartAsset problem
Integ. test (public s3 bucket for the helm chart?)

**Update: ** I believe I have a working solution, but am unable to test it due to a lambda cli layer error in Gitpod (https://community.gitpod.io/t/docker-inside-gitpod/3688). Because of this, I cannot run a unit test nor cdk synth the CFN template for the integration test.

iliapolo · 2021-08-05T12:30:15Z

Closing in favor #15899. @plumdog Feel free to re-open or collaborate with @pradoz.
Thanks for your work on this!

@plumdog

Adding on to the work @plumdog started on #13496 and @pradoz in #15899. Implemented the @iliapolo's [suggested changes](https://github.com/aws/aws-cdk/pull/15899/files#r683431181) Related to #9273 ### Use Case To be able to use private helm charts without needing a private chart repository. ### Proposed Solution [Allow helm charts](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-eks.HelmChart.html) to be an asset by introducing the property `chartAsset`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@plumdog

Adding on to the work @plumdog started on aws#13496 and @pradoz in aws#15899. Implemented the @iliapolo's [suggested changes](https://github.com/aws/aws-cdk/pull/15899/files#r683431181) Related to aws#9273 ### Use Case To be able to use private helm charts without needing a private chart repository. ### Proposed Solution [Allow helm charts](https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-eks.HelmChart.html) to be an asset by introducing the property `chartAsset`. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

github-actions bot assigned iliapolo Mar 9, 2021

github-actions bot added the @aws-cdk/aws-eks Related to Amazon Elastic Kubernetes Service label Mar 9, 2021

plumdog changed the title ~~Issue 10247: aws-eks, HelmChart from asset~~ feat(aws-eks) HelmChart from asset Mar 9, 2021

plumdog changed the title ~~feat(aws-eks) HelmChart from asset~~ feat(aws-eks) helm chart from asset Mar 9, 2021

plumdog changed the title ~~feat(aws-eks) helm chart from asset~~ feat(aws-eks): helm chart from asset Mar 9, 2021

plumdog force-pushed the aws-eks-issue-10247-helm-chart-from-asset branch from b82b23c to ef9bdc1 Compare March 9, 2021 16:01

plumdog mentioned this pull request Mar 9, 2021

[aws-eks] Support S3 private bucket repository from helm deployment #9273

Open

2 tasks

plumdog added 3 commits March 16, 2021 09:50

feat(aws-eks): helm chart from asset

ba4e209

Linting fix

6f9455a

Add a test

d48d326

plumdog force-pushed the aws-eks-issue-10247-helm-chart-from-asset branch from ef9bdc1 to d48d326 Compare March 16, 2021 10:11

iliapolo reviewed May 17, 2021

View reviewed changes

iliapolo requested changes May 17, 2021

View reviewed changes

iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label May 17, 2021

github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label May 18, 2021

peterwoodworth removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jun 11, 2021

iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 20, 2021

pradoz mentioned this pull request Aug 5, 2021

feat(eks): install helm chart from asset #15899

Closed

iliapolo closed this Aug 5, 2021

cmckni3 mentioned this pull request Oct 28, 2021

feat(eks): install helm chart from asset #17217

Merged


		Additionally, the `chart` property can be an `aws-s3-assets.Asset`,
		allowing use of local, private charts.

Conversation

plumdog commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gitpod-io bot commented Mar 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify bot commented Mar 9, 2021

Uh oh!

aws-cdk-automation commented Mar 16, 2021

AWS CodeBuild CI Report

Uh oh!

iliapolo left a comment

Choose a reason for hiding this comment

Uh oh!

iliapolo May 17, 2021

Choose a reason for hiding this comment

Uh oh!

iliapolo May 17, 2021

Choose a reason for hiding this comment

Uh oh!

iliapolo May 17, 2021

Choose a reason for hiding this comment

Uh oh!

iliapolo May 17, 2021

Choose a reason for hiding this comment

Uh oh!

iliapolo left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented May 18, 2021

Uh oh!

plumdog commented May 18, 2021

Uh oh!

pradoz commented Jul 15, 2021

Uh oh!

plumdog commented Jul 16, 2021

Uh oh!

pradoz commented Jul 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iliapolo commented Aug 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

plumdog commented Mar 9, 2021 •

edited

Loading

gitpod-io bot commented Mar 9, 2021 •

edited

Loading

pradoz commented Jul 16, 2021 •

edited

Loading