dnskr commented on code in PR #4690: URL: https://github.com/apache/kyuubi/pull/4690#discussion_r1163371982
########## charts/kyuubi/README.md: ########## @@ -32,6 +32,13 @@ cluster using the [Helm](https://helm.sh) package manager. - Kubernetes cluster - Helm 3.0+ +## Template rendering +When you want to test the template rendering, but not actually install anything. This command will render the templates. +It will return the rendered template to you so you can see the output. +```shell +helm install --dry-run --generate-name ../kyuubi +``` + Review Comment: There are two ways to render templates: - locally with `helm template --debug` - on server side with `helm install --dry-run --debug` Would you mind to mention both and maybe leave link to the [Helm doc](https://helm.sh/docs/chart_template_guide/debugging) page? ########## charts/kyuubi/templates/kyuubi-rbac.yaml: ########## @@ -17,15 +17,19 @@ {{- if .Values.rbac.create }} apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: {{ .Release.Name }} + labels: + {{- include "kyuubi.labels" . | nindent 4 }} +rules: {{- toYaml .Values.rbac.rules | nindent 2 }} +--- +apiVersion: rbac.authorization.k8s.io/v1 Review Comment: It's recommended to use dedicated files for a resource definition on the [Best Practices](https://helm.sh/docs/chart_best_practices/templates/#structure-of-templates) page: > Each resource definition should be in its own template file. ########## charts/kyuubi/templates/_helpers.tpl: ########## @@ -31,3 +31,21 @@ For details, see 'kyuubi.frontend.protocols': https://kyuubi.readthedocs.io/en/m {{- end }} {{- $protocols | join "," }} {{- end }} + +{{/* +Generate kyuubi common labels. +*/}} +{{- define "kyuubi.common.labels" -}} +app.kubernetes.io/name: {{ .Chart.Name }} +app.kubernetes.io/instance: {{ .Release.Name }} +{{- end -}} + +{{/* +Generate kyuubi labels. +*/}} +{{- define "kyuubi.labels" -}} +{{ include "kyuubi.common.labels" . }} +helm.sh/chart: {{ .Chart.Name }}-{{ .Chart.Version }} +app.kubernetes.io/version: {{ .Values.image.tag | default .Chart.AppVersion | quote }} +app.kubernetes.io/managed-by: {{ .Release.Service }} +{{- end -}} Review Comment: Could you please explain why you prefer this way rather than current implementation? Of course, it's a correct way to define common labels, and it's a default implementation created by `helm create chart_name`. But I found it exhausting to jump between templates and helpers.tpl during development, plus it's harder to manage indents and multiple `include`, so I would prefer to keep helpers.tpl as small as possible and have labels defined explicitly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
