Skip to content
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

Allow empty strings pvc storageClassName #96

Closed
nervo opened this issue Jun 8, 2021 · 7 comments · Fixed by #115
Closed

Allow empty strings pvc storageClassName #96

nervo opened this issue Jun 8, 2021 · 7 comments · Fixed by #115
Labels
wontfix This will not be worked on

Comments

@nervo
Copy link

nervo commented Jun 8, 2021

In some situations, it coud be useful to define pvc storageClassName as empty string (not null) to disable dynamic provisioning.

Right now, it's not fully possible in this chart, and the way of doing it may vary depends on the chart/subchart:

PostgreSQL (using the storageClass "-" de facto standard) :

postgresql:
  ...
  persistence:
    ...
    storageClass: "-"

Elasticsearch (using full "volumeClaimTemplate" block) :

elasticsearch:
  ...
  volumeClaimTemplate:
    ...
    storageClassName: ""

Zammad itself:

persistence:
  ...
  storageClass: ""

In this last example, zammad is using half of the de facto standard :)
First, storageClass is used instead of storageClassName (which, in my opinion, is a good thing).
But then, the special value "-" is not handled, and worst, if storageClass is an ampty string, it leads to no storageClassName at all because of the "with" usage right here: https://github.com/zammad/zammad-helm/blob/master/zammad/templates/statefulset.yaml#L298

@monotek
Copy link
Member

monotek commented Jun 8, 2021

So you're asking to change the default to "-"?
Why should we change this for everyone if it can be set throught the values.yaml file?

@nervo
Copy link
Author

nervo commented Jun 8, 2021

@monotek nope :)

storageClassName must be either:

  • an empty string
  • a string
  • absent/null

Each one of these has a different meaning, empty string for no class at all, string for a specific class, and absent/null for the default class.
With the current implementation, there is no difference between empty string, absent or null because of go template "with" method, which treat all of them the same way.

As a workaround, a standard is to allow user to pass a special string "-" to storageClass, resulting in a true empty string in the final manifest.
You can find an implementation in one of the subchart your are using, bitnami/postgresql: https://github.com/bitnami/charts/blob/master/bitnami/postgresql/templates/statefulset.yaml#L607 and https://github.com/bitnami/charts/blob/master/bitnami/common/templates/_storage.tpl#L15

In all cases, the default should remain juste like it is now, absent/null, aka. the default system class

@stale
Copy link

stale bot commented Aug 7, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Aug 7, 2021
@nervo
Copy link
Author

nervo commented Aug 8, 2021

I still believe :)

@stale stale bot removed the wontfix This will not be worked on label Aug 8, 2021
@monotek
Copy link
Member

monotek commented Aug 13, 2021

Do mind to provice an pr?

@stale
Copy link

stale bot commented Oct 12, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix This will not be worked on label Oct 12, 2021
@stale stale bot closed this as completed Oct 20, 2021
@monotek
Copy link
Member

monotek commented Oct 20, 2021

willl be fixed in: #115

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
2 participants