You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "dev-lpq (via GitHub)" <gi...@apache.org> on 2023/04/12 05:35:41 UTC
[GitHub] [kyuubi] dev-lpq opened a new pull request, #4698: [K8S][HELM]define Kyuubi common labels
dev-lpq opened a new pull request, #4698:
URL: https://github.com/apache/kyuubi/pull/4698
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
It's a default implementation created by helm create chart_name. Label info is relatively fixed and is frequently used in templates, reducing code redundancy. Define common tags that make yaml cleaner and easy to read.
### _How was this patch tested?_
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] [Run test](https://kyuubi.readthedocs.io/en/master/develop_tools/testing.html#running-tests) locally before make a pull request
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4698: [K8S][HELM] Define Kyuubi common labels
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#issuecomment-1508947629
@dev-lpq and @dnskr based on the discussion, I am inclined to respect the Helm-generated content, it's kind of a "standard" practice.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] dnskr commented on pull request #4698: [K8S][HELM] Define Kyuubi common labels
Posted by "dnskr (via GitHub)" <gi...@apache.org>.
dnskr commented on PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#issuecomment-1508923046
> What does the "default" mean?
>
> * the labels
> * the `_helpers.tpl`
>
Both. Here is the result of running `helm create kyuubi` command:
```
$ cat kyuubi/templates/_helpers.tpl
{{/*
Expand the name of the chart.
*/}}
{{- define "kyuubi.name" -}}
{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" }}
{{- end }}
{{/*
Create a default fully qualified app name.
We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec).
If release name contains chart name it will be used as a full name.
*/}}
{{- define "kyuubi.fullname" -}}
{{- if .Values.fullnameOverride }}
{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- $name := default .Chart.Name .Values.nameOverride }}
{{- if contains $name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}
{{- end }}
{{/*
Create chart name and version as used by the chart label.
*/}}
{{- define "kyuubi.chart" -}}
{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }}
{{- end }}
{{/*
Common labels
*/}}
{{- define "kyuubi.labels" -}}
helm.sh/chart: {{ include "kyuubi.chart" . }}
{{ include "kyuubi.selectorLabels" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}
{{/*
Selector labels
*/}}
{{- define "kyuubi.selectorLabels" -}}
app.kubernetes.io/name: {{ include "kyuubi.name" . }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}
{{/*
Create the name of the service account to use
*/}}
{{- define "kyuubi.serviceAccountName" -}}
{{- if .Values.serviceAccount.create }}
{{- default (include "kyuubi.fullname" .) .Values.serviceAccount.name }}
{{- else }}
{{- default "default" .Values.serviceAccount.name }}
{{- end }}
{{- end }}
```
```
$ cat kyuubi/templates/serviceaccount.yaml
{{- if .Values.serviceAccount.create -}}
apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ include "kyuubi.serviceAccountName" . }}
labels:
{{- include "kyuubi.labels" . | nindent 4 }}
{{- with .Values.serviceAccount.annotations }}
annotations:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- end }}
```
To me it is not the best formatted and clear code.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] codecov-commenter commented on pull request #4698: [K8S][HELM]define Kyuubi common labels
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#issuecomment-1504776371
## [Codecov](https://codecov.io/gh/apache/kyuubi/pull/4698?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#4698](https://codecov.io/gh/apache/kyuubi/pull/4698?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (e98d1d9) into [master](https://codecov.io/gh/apache/kyuubi/commit/f0615a9aab0a5b755c16ee0b966a5ea59b98bd10?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f0615a9) will **decrease** coverage by `0.03%`.
> The diff coverage is `n/a`.
```diff
@@ Coverage Diff @@
## master #4698 +/- ##
============================================
- Coverage 57.99% 57.96% -0.03%
Complexity 13 13
============================================
Files 580 580
Lines 32218 32218
Branches 4304 4304
============================================
- Hits 18684 18676 -8
- Misses 11749 11750 +1
- Partials 1785 1792 +7
```
[see 9 files with indirect coverage changes](https://codecov.io/gh/apache/kyuubi/pull/4698/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] dnskr commented on pull request #4698: [K8S][HELM] Centralize Kyuubi labels definition
Posted by "dnskr (via GitHub)" <gi...@apache.org>.
dnskr commented on PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#issuecomment-1508995940
@dev-lpq BTW, [skywalking-kubernetes](https://github.com/apache/skywalking-kubernetes/tree/master/chart) charts have some problems/inconsistencies related to common labels:
- [adapter](https://github.com/apache/skywalking-kubernetes/tree/master/chart/adapter) has common labels defined in _helpers.tpl, but uses them in Deployment resource only.
- [operator](https://github.com/apache/skywalking-kubernetes/tree/master/chart/operator) also has common labels defined in _helpers.tpl, but uses them in Deployment resource only.
- [skywalking](https://github.com/apache/skywalking-kubernetes/tree/master/chart/skywalking) uses approach with explicitly defined labels (our current implementation).
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4698: [K8S][HELM] Define Kyuubi common labels
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#issuecomment-1508531926
> It's a default implementation created by helm create chart_name.
What does the "default" mean?
- the labels
- the `_helpers.tpl`
I'm +0 for centralizing the labels into `_helpers.tpl`, the major reason is to reduce redundant codes, but given the reason listed by @dnskr, this may be kind of an over-engineering change.
Another thing, `kyuubi.common.labels` is not a good name, which is too "common", unless it is a common practice in Helm charts.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] dev-lpq commented on pull request #4698: [K8S][HELM] Define Kyuubi common labels
Posted by "dev-lpq (via GitHub)" <gi...@apache.org>.
dev-lpq commented on PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#issuecomment-1508790822
> * It's less clear and readable: template renders template which also renders template
@dnskr @pan3793 Thanks for your review and answer. But as the helm official doc, `_helpers.tpl` : A place to put template helpers that you can re-use throughout the char. There is no doubt that Kyuubi labels are re-used in char.
https://helm.sh/docs/chart_template_guide/getting_started/#a-quick-glimpse-of-mycharttemplates
in my view：
1. Follow the helm official doc, and define re-used labels in the _helpers.tpl that make Yaml cleaner and easy to read.
2. Don't worry about indents formatted, I think that defining helm labels helps manage Yaml indents formatted.
4. Label info is relatively fixed and is frequently used in templates, so there is no impact on the developer when jumping between templates and helpers.tpl
@pan3793 I change the name from kyuubi.common.labels to kyuubi.selectorLabels by viewing other Apache repo [skywalking-kubernetes](https://github.com/apache/skywalking-kubernetes/blob/master/chart/adapter/templates/_helpers.tpl), [apisix-helm-chart](https://github.com/apache/apisix-helm-chart/blob/master/charts/apisix-dashboard/templates/_helpers.tpl).
If the Kyuubi define label is an over-engineering change, please ignore it. Thanks!
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 closed pull request #4698: [K8S][HELM] Centralize Kyuubi labels definition
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 closed pull request #4698: [K8S][HELM] Centralize Kyuubi labels definition
URL: https://github.com/apache/kyuubi/pull/4698
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] dev-lpq commented on a diff in pull request #4698: [K8S][HELM] Centralize Kyuubi labels definition
Posted by "dev-lpq (via GitHub)" <gi...@apache.org>.
dev-lpq commented on code in PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#discussion_r1167426140
##########
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.
Review Comment:
ok, change the labels comment
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4698: [K8S][HELM] Define Kyuubi common labels
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#discussion_r1167061453
##########
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.
Review Comment:
the code is short and clear, the comment should explain the purpose of the definition instead of what the code does.
Labels used by selectors to match Kyuubi resources.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] dev-lpq commented on pull request #4698: [K8S][HELM] Centralize Kyuubi labels definition
Posted by "dev-lpq (via GitHub)" <gi...@apache.org>.
dev-lpq commented on PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#issuecomment-1509621334
> Helm-generated
> @dev-lpq BTW, [skywalking-kubernetes](https://github.com/apache/skywalking-kubernetes/tree/master/chart) charts have some problems/inconsistencies related to common labels:
>
> * [adapter](https://github.com/apache/skywalking-kubernetes/tree/master/chart/adapter) has common labels defined in _helpers.tpl, but uses them in Deployment resource only.
> * [operator](https://github.com/apache/skywalking-kubernetes/tree/master/chart/operator) also has common labels defined in _helpers.tpl, but uses them in Deployment resource only.
> * [skywalking](https://github.com/apache/skywalking-kubernetes/tree/master/chart/skywalking) uses approach with explicitly defined labels (our current implementation).
@dnskr Thanks, I still think it's best to follow the Helm-generated label definitions.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on a diff in pull request #4698: [K8S][HELM] Define Kyuubi common labels
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on code in PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#discussion_r1167061453
##########
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.
Review Comment:
the code is short and clear, the comment should explain the purpose of the definition instead of what the code does.
Let's respect the Helm-generated comments.
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4698: [K8S][HELM] Centralize Kyuubi labels definition
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#issuecomment-1508952945
Also cc @hddong @zwangsheng
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [kyuubi] pan3793 commented on pull request #4698: [K8S][HELM] Centralize Kyuubi labels definition
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #4698:
URL: https://github.com/apache/kyuubi/pull/4698#issuecomment-1511542992
Thanks, merged to master
--
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: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org