[bitnami/common] [bitnami/wordpress] Use global.storageClass for fallback, not override

[aragilar]

Description of the change

This fixes #24845 as we now prefer the more specific value over the global value, rather than the other way around.

Benefits

As above, this fixes #24845.

Possible drawbacks

Applicable issues

• fixes [bitnami/wordpress] Appears that "global.storageClass" overrides more specific storageClass settings #24845

Additional information

Checklist

• Chart version bumped in Chart.yaml according to semver. This is not necessary when the changes only affect README.md files.
• Variables are documented in the values.yaml and added to the README.md using readme-generator-for-helm
• Title of the pull request follows this pattern [bitnami/<name_of_the_chart>] Descriptive title
• All commits signed off and in agreement of Developer Certificate of Origin (DCO)

[aragilar]

Let me know if there are any issues with this (or feel free to push any fixes).

[juan131]

Hi @aragilar

Thanks so much for this contribution!

It's true that it'd be more convenient global.storageClass to behave as a fallback instead of taking precedence over more specific values (e.g. persistence.storageClass in a chart such as WordPress).

There's a reason though for this behavior: being consistent with other global values and their associated common helpers. To be more precise, to be aligned with global.imageRegistry and the common helper "common.images.image". We want global.imageRegistry to overwrite more specific values (e.g. image.registry in a chart such as WordPress) to provide an easy way to overwrite the registry for every image included in the chart. This is very useful when consuming images from a private registry.

However, it's true that they're different use cases since we're setting a default value for image.registry while we're not setting any for persistence.storageClass and we're relying on the cluster default one. That said, having two different global values behaving in a different way would be very confusing and we should explore different alternatives such as the ones below:

• Should we rename global.storageClass to global.defaultStorageClass and apply your changes? By adding the default prefix we're justifying its different behavior.
• Should we add support for a boolean parameter on "common.storage.class" that defines whether to treat global as default or not (similar to the boolean parameters we define in helpers such as "common.secrets.passwords.manage")?

[aragilar]

Cool! I'm happy with defaultStorageClass if you are (and in terms of backwards-compat, that seems best). I'm happy to make that change, or feel free to modify this, whichever is easier on your side.

Side note, I'm not sure how best (or how well) doing a transition would work, but if the current storageClass was overrideStorageClass (or something similar), then I would have not tried to use it as the default (I was thinking along the lines that global would be treated like in a programming language, and that local would override it). I'm happy to make a wishlist-level issue if you think it's worth raising (or maybe just some wording changes to make things more obvious)?

[juan131]

Hi @aragilar

We should apply the change in the "common" library to expect .global.defaultStorageClass instead of .global.storageClass and release a new version of the library. This can be done in this PR.

Then, we'll have to create a PR per chart bumping the chart deps and applying the changes below (we can take part of this tedious part):

index 176a0ae34..cbb11af5a 100644
--- a/bitnami/wordpress/values.yaml
+++ b/bitnami/wordpress/values.yaml
@@ -9,7 +9,7 @@

 ## @param global.imageRegistry Global Docker image registry
 ## @param global.imagePullSecrets Global Docker registry secret names as an array
-## @param global.storageClass Global StorageClass for Persistent Volume(s)
+## @param global.defaultStorageClass Global default StorageClass for Persistent Volume(s)
 ##
 global:
   imageRegistry: ""

It also seems we'll have to pay special attention while adapting JupyterHub & Cassandra, see:

• https://github.com/bitnami/charts/blob/main/bitnami/jupyterhub/templates/_helpers.tpl#L220-241
• https://github.com/bitnami/charts/blob/main/bitnami/cassandra/templates/_helpers.tpl#L102-122

[aragilar]

@juan131 I think my new commit adds the right flow in terms of which values override each other, but I'd appreciate the second set of eyes. I'm happy to drop the first commit, or squash, or whatever is your preferred process (or feel free to make changes to the branch yourself).

[juan131]

Please correct me if I'm wrong but this will fail if global object isn't defined since you're trying to access a "storageClass" property of a null object.
Try with this:

Suggested change

	{{/*
	Uses https://stackoverflow.com/a/68807258/1306020 to avoid lots of ifs.
	This allows all storage classes to be overridden by .global.storageClass, and to
	fallback to .global.defaultStorageClass if no specific class was set.
	*/}}
	{{- $storageClass := (.global).storageClass | default .persistence.storageClass | default (.global).defaultStorageClass | default "" -}}
	{{- $defaultStorageClass := ternary (default "" .global.storageClass) "" .global -}}
	{{- $storageClass := default $defaultStorageClass .persistence.storageClass -}}

[aragilar]

Sorry, got pulled on to other projects so this dropped down the my todo list. My understanding from the linked stackoverflow answer (though I'm very new to how the helm/go templating language works) is you can do (.maybe_null).also_maybe_null and this won't error due to the extra parentheses? Happy to use your flow if you think it's better.

[juan131]

You're definitely right! Indeed, I love this syntax, I sent a PR to adopt it broadly in the common library, see #25446

[github-actions]

This Pull Request has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thank you for your contribution.

[aragilar]

Was there anything I needed to do for this/what are the next steps?

[juan131]

Hi @aragilar

Yes! Please ensure you merge the latest changes in the origin's main branch in your fork's branch and then, update the PR. Thanks in advance.

[github-actions]

Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Pull Request. Do not hesitate to reopen it later if necessary.