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

The mountPath parameter is not respected in values.yaml #240

Closed MurzNN closed 7 months ago

MurzNN commented 7 months ago

Details

The mountPath parameter is not respected in the values.yaml file, the persistence name is used instead. You can reproduce this in the vaultwarden's example chart:

In the values.yaml file we have a mountPath parameter: https://github.com/bjw-s/helm-charts/blob/fbeaa67f7d75a6f2c35c77a5e17bf129360aed17/examples/helm/vaultwarden/values.yaml#L62

But actually seems it is not get into account when rendering a template, because if I change it to something else, like this:

  persistence:
    config:
      enabled: true
      type: persistentVolumeClaim
      accessMode: ReadWriteOnce
      size: 1Gi
-     mountPath: /config
+     mountPath: /config2

no changes will be produced in the target template.

But if I change the persistence name like this:

  persistence:
-   config:
+   config2:
      enabled: true
      type: persistentVolumeClaim
      accessMode: ReadWriteOnce
      size: 1Gi
      mountPath: /config

then the mount path will be changed, together with the resource name.

What did you expect to happen:

if the mountPath is present, it should be used to set the mount path instead of the persistence resource name.

MurzNN commented 7 months ago

Looking into the source code, seems we need to put now a more complex construction like this?

  persistence:
    config:
      enabled: true
      type: persistentVolumeClaim
      accessMode: ReadWriteOnce
      size: 1Gi
-     mountPath: /config2
+     advancedMounts:
+       main:
+         main:
+           - path: /config2

If so, could you please update the example chart with vaultwarden, and explain the reasons why the more simple way is removed? And maybe consider returning it, to simplify the things?

MurzNN commented 7 months ago

Found a documentation here https://github.com/bjw-s/helm-charts/blob/fbeaa67f7d75a6f2c35c77a5e17bf129360aed17/charts/library/common/values.yaml#L656 So seems we need to just update the vaultwarden example then?

bjw-s commented 7 months ago

Looks like that example hasn't been updated properly during the v2 upgrade. I will add this to #240

bjw-s commented 7 months ago

Updated the examples in #240