-
Notifications
You must be signed in to change notification settings - Fork 198
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify input type Helm #977 #979
Conversation
|
@@ -55,7 +74,7 @@ Special flags: | |||
|
|||
See the [helm doc](https://helm.sh/docs/helm/helm_template/) for further detail. | |||
|
|||
#### Example | |||
## Example |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix precommit error
markdownlint-fix.........................................................Failed
- hook id: markdownlint-fix
- exit code: 1
docs/pages/input_types/helm.md:77 MD001/heading-increment/header-increment Heading levels should only increment by one level at a time [Expected: h2; Actual: h4]
@Moep90 I don't understand this. The PR already updates the documentation. Do you think it's not sufficient or are you talking about updating something else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks safe and a nice simplification for using helm 👍
NVM, my bad. |
Hey @gburiola this is awesome! One question - how does this deal with caching? E.g. will subsequent executions of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me, just requesting a small comment
It doesn't cache anything. It fetches the chart on the fly using Bear in mind that the the current way using dependencies also makes an HTTP call to the depency URL every time |
Are you sure about that? I thought that we have the flags |
Yes.
I'm not using dependency mgmt in this case. I'm using the helm cli I've updated the docs on 614807e |
- input_type: helm and input_paths or - input_type: helm and helm_params.chart_name
OK, worth making it very obvious in the documentation of the behaviour of this alternative way if we still think it's worth it! Is there no way helm will do some sort of caching at all? If not, this feels suboptimal to me. |
@gburiola @ramaro helm does seem to support caching? https://helm.sh/docs/helm/helm/ We also need to understand what happens if multiple compile of the same helm compete to download the charts at the same time. |
Fixes issue #977
test
this target file supports both formats of
input_type: helm
:As we can see in the output below, the
helm
binary is called in two different ways for each mode.FULL OUTPUT