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

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

afzouni
Copy link

@afzouni afzouni commented Jul 1, 2024

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

  • No specific issue number, but this change addresses the inconsistency between the documentation and the statefulset.yaml file regarding the storageClassName.

Check List

  • [* ] Commits are signed per the DCO using --signoff

For any changes to files within Helm chart directories:

  • Helm chart version bumped
  • Helm chart CHANGELOG.md updated to reflect change

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
Copy link

this should be fixed

@peterzhuamazon
Copy link
Member

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
Copy link
Author

afzouni commented Aug 30, 2024

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
Copy link
Member

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

Thanks.

@peterzhuamazon
Copy link
Member

Updated changelog and charts version and rebased on main.

Thanks.

@peterzhuamazon
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@afzouni
Copy link
Author

afzouni commented Nov 7, 2024

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
Copy link
Member

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
Copy link
Author

afzouni commented Nov 7, 2024

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

{{- if .Values.persistence.storageClass }}
{{- if (eq "-" .Values.persistence.storageClass) }}
storageClassName: ""
{{- else }}
storageClassName: "{{ .Values.persistence.storageClass }}"
{{- end }}
{{- end }}

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 👀 In Review
Development

Successfully merging this pull request may close these issues.

4 participants