fix: include rendered templates in helm manifest generation#878
fix: include rendered templates in helm manifest generation#878parthivsaikia wants to merge 3 commits intomeshery:masterfrom
Conversation
Summary of ChangesHello @parthivsaikia, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Helm manifest generation process by ensuring that Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly updates GetManifestsFromHelm to render Helm chart templates in addition to CRDs, addressing a key functionality gap. The implementation is sound, using strings.Builder for efficiency and proper error wrapping. My review focuses on improving the function's reusability and robustness. I've suggested making the Helm release options and values configurable to increase flexibility. Additionally, I've recommended enhancing the manifest filtering to exclude Helm test files, which will make the output more predictable for consumers.
| releaseOptions := chartutil.ReleaseOptions{ | ||
| Name: "meshery-helm-release", | ||
| Namespace: "default", | ||
| Revision: 1, | ||
| IsInstall: true, | ||
| } | ||
| manifests := "" | ||
|
|
||
| caps := chartutil.DefaultCapabilities | ||
|
|
||
| values, err := chartutil.ToRenderValues(chart, chartutil.Values{}, releaseOptions, caps) | ||
| if err != nil { | ||
| return "", ErrApplyHelmChart(fmt.Errorf("failed to generate render values: %w", err)) | ||
| } |
There was a problem hiding this comment.
The function currently uses hardcoded releaseOptions (like release name and namespace) and always renders the chart with empty values (chartutil.Values{}). This limits the function's flexibility, as it's not possible to specify a different release name, namespace, or provide custom values for template rendering, which are common requirements when working with Helm.
To improve reusability, consider making these configurable. You could introduce an options struct to pass these values into the function. This would make the function much more versatile for use in a library.
Signed-off-by: PARTHIV PRATIM SAIKIA <parthivsaikia985@gmail.com>
parthivsaikia
force-pushed
the
fix-process-templates-folder
branch
from
07fc8dd to
ec7bbb7
Compare
Signed-off-by: PARTHIV PRATIM SAIKIA <parthivsaikia985@gmail.com>
leecalcote
left a comment
leecalcote
left a comment
There was a problem hiding this comment.
@parthivsaikia this seems like a lot of custom code for something that we should be getting from the Helm go client without having to reinvent the wheel on our own. @aabidsofi19
|
I have used helm client in most places but I will try to refactor it further if possible. |
2 participants
Description
This PR updates
GetManifestsFromHelmto correctly render Helm chart templates instead of only retrieving CRDs. Previously, the function ignored thetemplates/directory, which caused "no components found" errors during model generation because standard resources (Deployments, Services, etc.) were missing.This PR fixes #877
Signed commits