vectordotdev / helm-charts

Helm charts for Vector.
https://vector.dev
Mozilla Public License 2.0
103 stars 89 forks source link

fix(vector): move minReadySeconds to global scope #305

Closed rbakhtaraev closed 1 year ago

rbakhtaraev commented 1 year ago

In this case, we must use the global (root) context for access to .Values.minReadySeconds variable. Without it, Helm try to find it in .Values.updateStrategy scope.

closes: #306

neuronull commented 1 year ago

I validated this locally, thanks for dropping a fix!

neuronull commented 1 year ago

The chart version just needs to be upgraded here: https://github.com/vectordotdev/helm-charts/blob/develop/charts/vector/Chart.yaml#L3

neuronull commented 1 year ago

Ah, the changelog doesn't need to be modified in the individual PRs, we update it with a script as part of the release process: https://github.com/vectordotdev/helm-charts#releasing

kaden-l-nelson commented 1 year ago

@spencergilbert What're your thoughts on taking this PR instead? https://github.com/vectordotdev/helm-charts/pull/308/files

308 makes it so that min ready seconds can be configured in the absence of an update strategy.

skygrammas commented 1 year ago

@kaden-l-nelson makes a good point. In the absence of an update strategy value, would the {{- with .Values.updateStrategy }} block not be generated? Resulting in minReadySeconds not being templated despite it being reference in the root context with this proposed change?

spencergilbert commented 1 year ago

@kaden-l-nelson makes a good point. In the absence of an update strategy value, would the {{- with .Values.updateStrategy }} block not be generated? Resulting in minReadySeconds not being templated despite it being reference in the root context with this proposed change?

That's correct - it does look like the with line isn't in the right place. There doesn't need to be any connection between the updateStrategy and the minReadySeconds values so they should be templated separately.

cc @neuronull

spencergilbert commented 1 year ago

@rbakhtaraev looks like this needs a helm-docs run to update the badges.