-
Notifications
You must be signed in to change notification settings - Fork 359
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
Helm Chart: statefulset template, broken logic #122
Comments
@jburnitz thanks for creating this issue! Yeah I guess the best solution is that we only define |
This is the same in the chart, so we should fix it. As for this sentence from the description:
The default value of "Filesystem" is for the volumeMode field, which we don't set. |
@matskiv I can try to fix this minor issue if that's okay (especially if I get a few days to work on it). I didn't find the mentioned main/chart/templates/statefulset.yaml#L32 repo but only the main/charts/**/templates one. This is an excellent issue to be worked on before diving into the actual vcluster's logic. |
@vsantos Yeah, absolutely! I'll assign this to you then. You can take your time, there is no rush with this issue.
Yeah, we added more charts since this was reported 😄 I would say that the issue applies to all btw: a tip for quick validation. First time I was testing some chart changes I did in a way too complicated way, so sharing just in case.
I used a very similar command to this just the other day, it's very helpful for quickly checking the output of the chart. |
@matskiv Do you guys think is a good idea to add some kind of "automated" mechanism to make it easier to test static YAML files as part of the scope of this PR? We could use bats, it's a very old tool but good in make assertions in bash scripts. Your suggested logic to test locally could be automated to ensure that a given Or maybe we should just fix the broken logic and in the future start a new thread about automation alternatives? |
@vsantos Oh, I would love to have automated tests for the charts. This has been on my todo list for a while, but it always gets pushed back 😂 I would try to first look for some tools tailored to Helm, to have sort of a "unit tests" for the Helm charts. |
@matskiv I would like to contribute to this issue, could you please assign it to me? |
@hiteshwani29 Thank you for your interest. It's assigned to you now. |
https://github.com/loft-sh/vcluster/blob/main/chart/templates/statefulset.yaml#L32
spec.volumeClaimTemplates.0.spec.storageClassName by default renders as null instead of a value or empty string. This breaks kustomize validation.
I'm actually surprised Helm doesn't complain about a null value to the key, seems like a break of yaml spec.
K8s doesn't care if the field is defined or not, and the docs simply state to use a string.
storageClassName: "" # Empty string must be explicitly set otherwise default StorageClass will be set
https://kubernetes.io/docs/concepts/storage/persistent-volumes/#class-1
https://v1-21.docs.kubernetes.io/docs/reference/generated/kubernetes-api/v1.21/#persistentvolumeclaimspec-v1-core
Proposed:
The text was updated successfully, but these errors were encountered: