-
Notifications
You must be signed in to change notification settings - Fork 617
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
feat: add support for parameters for storageClass #2434
feat: add support for parameters for storageClass #2434
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @sakshi-1505. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@zetaab @pierreprinetti @jichenjc Please take a look. |
@@ -160,6 +160,12 @@ secret: | |||
|
|||
storageClass: | |||
enabled: true | |||
parameters: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this sample ? so should be comment out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really believe commenting our things in helm's values but always like to put each & every change behind a toggle to maintain backward compatibility. It's not a sample but actual values file, & I have kept all changed behind the storageClass.parameters.Enabled
toggle, so this is a backward compatible change where folks if want can enable & by default it won't take effect.
charts/cinder-csi-plugin/values.yaml
Outdated
parameters: | ||
enabled: false | ||
type: gp2 | ||
availabilityZone: us-east-1a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am talking about here, e.g apparently we don't want any chart default to this AWS region?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got your point, let me fix this real quick
Signed-off-by: sakshi-1505 <[email protected]>
c892c4e
to
06f9751
Compare
Please check again @jichenjc |
/ok-to-test |
@jichenjc @pierreprinetti can I please get a review here? |
/lgtm |
availabilityZone: {{ .Values.storageClass.parameters.availabilityZone }} | ||
{{ end }} | ||
{{ range .Values.storageClass.parameters.additionalParameters }} | ||
{{ .key }}: {{ .value }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this will be properly indented? Could we do it like here instead?
cloud-provider-openstack/charts/manila-csi-plugin/templates/nodeplugin-daemonset.yaml
Lines 120 to 123 in 2565d89
{{- with .Values.nodeplugin.hostAliases }} | |
hostAliases: | |
{{- toYaml . | nindent 8 }} | |
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sakshi-1505: Seems like you've missed this one comment.
Oh, you're missing the release note box too. Grab it from the template: cloud-provider-openstack/.github/PULL_REQUEST_TEMPLATE.md Lines 31 to 33 in 66624e3
|
@dulek please recheck |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you check one of my previous comments?
Besides that let's improve the release note a bit. How about:
[cinder-csi-plugin] Added support for modifying StorageClass parameters in the Helm chart.
availabilityZone: {{ .Values.storageClass.parameters.availabilityZone }} | ||
{{ end }} | ||
{{ range .Values.storageClass.parameters.additionalParameters }} | ||
{{ .key }}: {{ .value }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sakshi-1505: Seems like you've missed this one comment.
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
fixes #1980