feat(eks): install helm chart from asset#15899
feat(eks): install helm chart from asset#15899pradoz wants to merge 21 commits intoaws:masterfrom pradoz:master
Conversation
|
|
| with open(values_file, "w") as f: | ||
| f.write(json.dumps(values, indent=2)) | ||
|
|
||
| if not request_type == "Delete" and chart_asset is not None and chart_asset.startswith('s3://'): |
There was a problem hiding this comment.
Move this logic in between here:
And extract the whole download-and-extract-asset logic into a function.
Also - if you are expecting URL's to start with s3:// - validate that. Don't ignore it if they dont.
There was a problem hiding this comment.
chart_asset.startswith('s3://'): should validate that URL's start with s3:// unless I'm missing something obvious. Could you please explain more?
There was a problem hiding this comment.
Right now you apply the chart asset logic if chart_asset_url is defined and it starts with s3://.
What happens if this function is invoked for some reason on a chart_asset_url in the form of not-s3://?
Currently you will just ignore this, and fallback to the regular logic. Is this the correct behavior? This is a miss-configuration by the user, and we should fail, letting them know that a URL must start with s3:// if it is provided.
Same thing for repository, what happens if a user configured repository as well as chart_asset_url? This should fail, not fallback.
if chart_asset_url != None:
if not chart_asset_url.startswith('s3://'):
raise RuntimeError("'ChartAssetURL' must start with 's3://'")
if repository != None:
raise RuntimeError("'Repository' is not allowed when configuring 'ChartAssetURL'")
if version != None:
raise RuntimeError("'Version' is not allowed when configuring 'ChartAssetURL'")
chart = get_chart_asset_from_url(chart_asset_url, outdir)| path: path.join(__dirname, 'test-chart'), | ||
| }); | ||
| this.cluster.addHelmChart('test-chart', { | ||
| chart: 'test-chart-asset', |
There was a problem hiding this comment.
Is this really necessary? Seems like we can now make this an optional property, and validate that either chart or chartAsset is used.
Also - does repository and version have any meaning when using an asset? Should we assert they are not specified when using chartAsset?
There was a problem hiding this comment.
I agree with the point on making it an optional property, then validating that either chart or chartAsset is used.
For the second point, I don't believe so. I'll test that assertion out.
There was a problem hiding this comment.
I believe that repository should be None, but it would be another feature/issue to support versions for chartAsset. A possible solution would be to create nested file paths in s3, where some path references nested directories to scan for a specified version number.
There was a problem hiding this comment.
I agree with the point on making it an optional property, then validating that either chart or chartAsset is used.
I don't see this change...am I missing something? We also need to validate the neither repository nor version is used if chartAsset is defined.
|
Thanks for the quick reply @iliapolo I'll make these requested changes or add explanations soon |
|
The requested changes by @iliapolo have been made. I will make sure this PR stays up-to-date with If anyone has feedback or questions regarding the changes, please let me know! |
|
Thanks @pradoz - I'll take a look next week 👍 |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
@pradoz Few more comments. See above. |
|
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. |
|
@pradoz This was automatically closed for no activity but i'm happy to pick this up again when you have capacity. |
|
Is this PR still being worked on? We are looking for this functionality for a customer using CDK. |
|
See #17217 for comments and updates |
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*
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*
Adding on to the work @plumdog started on #13496 and implementing the changes suggested by @iliapolo
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