Add review guidelines around pvcs #4223
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
Thanks @rjkernick |
|
/assign |
|
cc @kubernetes/charts-maintainers |
REVIEW_GUIDELINES.md
Outdated
| ## set, choosing the default provisioner. (gp2 on AWS, standard on | ||
| ## GKE, AWS & OpenStack) | ||
| ## | ||
| # storageClass: "-" |
There was a problem hiding this comment.
I now think it'd be better to add an empty default value. Thoughts?
There was a problem hiding this comment.
I'm fine with either way
REVIEW_GUIDELINES.md
Outdated
| subPath: data | ||
| - mountPath: /opt/sonarqube/extensions | ||
| name: sonarqube | ||
| subPath: extensions |
There was a problem hiding this comment.
Maybe don't make it SonarQube-specific. A simpler example should suffice. I'd also remove the subPath from the example. It's optional and could be misleading.
There was a problem hiding this comment.
Yep. Is there a better place to put the subPath example? I agree it might be confusing here, but I would like to document it somewhere for people to use.
There was a problem hiding this comment.
Don't know where to put it. I think it is not specific to charts and we don't have to repeat Kubernetes docs here. I'd remove it.
There was a problem hiding this comment.
Makes sense. Done.
REVIEW_GUIDELINES.md
Outdated
| @@ -76,6 +76,29 @@ image: | |||
| * The use of the `default` function should be avoided if possible in favor of defaults in `values.yaml`. | |||
| * It is usually best to not specify defaults for resources and to just provide sensible values that are commented out as a recommendation, especially when resources are rather high. This makes it easier to test charts on small clusters or Minikube. Setting resources should generally be a conscious choice of the user. | |||
|
|
|||
| ## Persistence | |||
|
|
|||
| * Persistence should be turned off by default | |||
There was a problem hiding this comment.
Actually we prefer to have persistence via PVC enabled by default, and it is in most of the existing charts.
| size: 10Gi | ||
| # existingClaim: "" | ||
| ``` | ||
|
|
There was a problem hiding this comment.
I think it would be great to also add an example of a PVC using those values:
{{- if and .Values.persistence.enabled (not .Values.persistence.existingClaim) }}
kind: PersistentVolumeClaim
apiVersion: v1
metadata:
<standard metadata>
spec:
accessModes:
- {{ .Values.persistence.accessMode | quote }}
resources:
requests:
storage: {{ .Values.persistence.size | quote }}
{{- if .Values.persistence.storageClass }}
{{- if (eq "-" .Values.persistence.storageClass) }}
storageClassName: ""
{{- else }}
storageClassName: "{{ .Values.persistence.storageClass }}"
{{- end }}
{{- end }}
{{- end }}
and in a PodSpec:
volumes:
- name: data
{{- if .Values.persistence.enabled }}
persistentVolumeClaim:
claimName: {{ .Values.persistence.existingClaim | default (include "fullname" .) }}
{{- else }}
emptyDir: {}
{{- end -}}
There was a problem hiding this comment.
Added the suggested examples
REVIEW_GUIDELINES.md
Outdated
|
|
||
| * Persistence should be turned off by default | ||
| * PVCs should support specifying an existing claim | ||
| * Storage class should be empty by default |
There was a problem hiding this comment.
Can we elaborate here and mention it should "be empty by default, to use the default storage class"?
|
@prydonius Made the suggested changes as they all made sense. Let me know if there is anything else. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: prydonius, rjkernick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
* upstream/master: (944 commits) Rename service port to http (helm#4442) [stable/neo4j] Change the image of the initContainer examples (helm#4269) move burrow to stable repo (helm#3481) Upgrade kube-state-metrics to 1.2.0, add new collectors (helm#4146) Add review guidelines around pvcs (helm#4223) [stable/parse] Release 0.3.10 (helm#4389) [stable/phabricator] Release 0.5.19 (helm#4433) Support exposing jmx and additional ports (helm#4072) Add default of "" for string comparison (helm#4420) [incubator/kafka] Makes readiness probe configurable (helm#3948) Published stash chart 0.7.0-rc.1 (helm#4410) Enable testing charts with test values (helm#4157) [incubator/kafka] Fix initContainer failure which did not error (helm#4400) [stable/etcd-operator] deployment typos and add tolerations (helm#4139) Typo fix in coscale/README.md (helm#4306) Typo fix in concourse/README.md (helm#4303) Typo fix in cockroachdb/README.md (helm#4302) [stable/jenkins] Bump appVersion (helm#4177) Typo fix in cluster-autoscaler/README.md (helm#4301) [stable/traefik] Bump appVersion to 1.5.4 (helm#4206) ...
* Add review guidelines around pvcs * Updates to pvc guidelines * Updates to match existing best practices and examples
* Add review guidelines around pvcs * Updates to pvc guidelines * Updates to match existing best practices and examples
* Add review guidelines around pvcs * Updates to pvc guidelines * Updates to match existing best practices and examples Signed-off-by: voron <av@arilot.com>
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer: