search

opensearch-project / helm-charts

:wheel_of_dharma: A community repository for Helm Charts of OpenSearch Project.
https://opensearch.org/docs/latest/opensearch/install/helm/
Apache License 2.0
173 stars 234 forks source link

fix: rename storageClass to storageClassName based in docs and comments #553

Open afzouni opened 4 months ago

afzouni commented 4 months ago

Description

In the values.yaml and README.md, it mentions defining storageClassName to use a custom storageClassName. However, the variable in statefulset.yaml uses storageClass instead. This pull request changes storageClass to storageClassName to ensure consistency and proper functionality.

Issues Resolved

Check List

For any changes to files within Helm chart directories:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license. For more information on following Developer Certificate of Origin and signing off your commits, please check here.

jfreeland commented 2 months ago

this should be fixed

peterzhuamazon commented 2 months ago

Adding @TheAlgo to take a look since this looks like a rollover when we migrate from devops repo to helm-charts repo.

cc: @prudhvigodithi

Hi @afzouni have you tested this change and confirm it failed before your change, but succeed after you apply the fix? Thanks.

afzouni commented 2 months ago

Hi @peterzhuamazon, yes, I tested it. Before the change, it failed, but after setting the storageClassName (I used longhorn in my case), it worked successfully.

peterzhuamazon commented 2 months ago

Adding @prudhvigodithi to take a look, I am ok with this one. Will need to update the version number and such soon.

Thanks.

peterzhuamazon commented 2 months ago

Updated changelog and charts version and rebased on main.

Thanks.

peterzhuamazon commented 1 week ago

Adding @prudhvigodithi and @TheAlgo to take a look as I am fine with this change.

I will update the version and changelog once we approve.

Thanks.

prudhvigodithi commented 1 week ago

Thanks @afzouni LGTM. Question is what happens for users who had storageClass in the values yaml and have the resources created, is that a breaking change for them when they pulled the latest version of the chart ?

peterzhuamazon commented 1 week ago

Thanks @prudhvigodithi , pending @afzouni responses before I give my approval and change version.

afzouni commented 1 week ago

Thanks @prudhvigodithi , pending @afzouni responses before I give my approval and change version.

Thanks @prudhvigodithi and @prudhvigodithi, Sorry, I’m a bit confused, What exactly do you need me to response to?

prudhvigodithi commented 1 week ago

Question is what happens for users who had storageClass in the values yaml and have the resources created, is that a breaking change for them when they pulled the latest version of the chart ?

@afzouni please check this?

afzouni commented 1 week ago

Question is what happens for users who had storageClass in the values yaml and have the resources created, is that a breaking change for them when they pulled the latest version of the chart ?

@afzouni please check this?

Sure, and sorry for my delay. If users previously defined storageClass in their values.yaml file, this change would indeed be breaking for them. Currently, setting storageClass works, as it references the storageClassName in the StatefulSet template https://github.com/opensearch-project/helm-charts/blob/a1c5b8f301d68649f0534b63bf545a61844ce651/charts/opensearch/templates/statefulset.yaml#L46-L52

However, after merging this pull request, if they pull the latest version of the chart and reapply it, the storageClassName will default to "" unless they update their values.yaml to use the new structure.

P.S: As an idea, instead of changing storageClass to storageClassName, it could be updated only in the README and values.yaml, but I’m not sure if this is a good approach due to storageClassName being directly referenced in the templates.