search

bjw-s / helm-charts

A collection of Helm charts
https://bjw-s.github.io/helm-charts/
Apache License 2.0
524 stars 98 forks source link

Persistence items are disabled by default but the docs say the opposite (app-template) #251

Closed phybros closed 5 months ago

phybros commented 6 months ago

The docs for app-template say that we can leave enabled: true out of our persistence items but in fact they appear to be disabled by default according to https://github.com/bjw-s/helm-charts/blob/bd032c5ab33edad52f7a218a42ff2d51f4a58c3a/charts/library/common/values.yaml#L614C8-L614C8

This bit me just now, I left enabled off the yaml and sure enough the items were disabled. Seems like maybe just a docs update is required?

haraldkoch commented 6 months ago

I think that this is only true for the config item, and only because it's in the values.yaml file as a (disabled) example.

phybros commented 6 months ago

Makes sense. Sounds like a pretty straightforward docs update. I can submit a PR

On Sat, Dec 23, 2023 at 6:36 PM Harald Koch @.***> wrote:

I think that this is only true for the config item, and only because it's in the values.yaml file as a (disabled) example.

— Reply to this email directly, view it on GitHub https://github.com/bjw-s/helm-charts/issues/251#issuecomment-1868389776, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAPEU3DCIB5FCH2E7G2HDZ3YK5TILAVCNFSM6AAAAABBA7ZTJOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNRYGM4DSNZXGY . You are receiving this because you authored the thread.Message ID: @.***>

qlonik commented 6 months ago

Hey, I encountered this too, when creating a type: persistentVolumeClaim entry. It seems that because of the strict check for $pvc.enabled here, it behaves like this. Should this branch instead check if the key is either not present or present and set to true? Then it should get back to the defaulting to true scenario as docs say.

bjw-s commented 6 months ago

Hey, I encountered this too, when creating a type: persistentVolumeClaim entry. It seems that because of the strict check for $pvc.enabled here, it behaves like this. Should this branch instead check if the key is either not present or present and set to true? Then it should get back to the defaulting to true scenario as docs say.

Good find! This is definitely an issue, as it will prevent PVC's from being created when the enabled flag isn't set. I will fix that in an upcoming version.

As for the config persistence item, @haraldkoch is correct that this should be the only "exception" since it is already present but disabled in the default values for legacy purposes. V3 will drop these default values, making it so that all persistence items adhere to the "enabled by default" behavior.