MonkeyCanCode commented on code in PR #626:
URL: https://github.com/apache/polaris/pull/626#discussion_r1929932037
##########
helm/polaris/templates/deployment.yaml:
##########
@@ -77,36 +60,67 @@ spec:
securityContext:
{{- tpl (toYaml .Values.podSecurityContext) . | nindent 8 }}
{{- end }}
+ {{- if .Values.extraInitContainers }}
+ initContainers:
+ {{- tpl (toYaml .Values.extraInitContainers) . | nindent 8 }}
+ {{- end }}
containers:
- name: {{ .Chart.Name }}
- {{- if .Values.securityContext}}
+ {{- if .Values.containerSecurityContext }}
securityContext:
- {{- tpl (toYaml .Values.securityContext) . | nindent 12 }}
+ {{- tpl (toYaml .Values.containerSecurityContext) . | nindent 12 }}
{{- end }}
image: "{{ tpl .Values.image.repository . }}:{{ tpl
.Values.image.tag . | default .Chart.Version }}"
imagePullPolicy: {{ tpl .Values.image.pullPolicy . }}
- {{- if .Values.extraEnv }}
+ {{- if or .Values.storage.secret.name .Values.extraEnv }}
env:
+ {{- if .Values.storage.secret.name }}
Review Comment:
Maybe use a helper function for secret to env?
##########
helm/polaris/templates/_helpers.tpl:
##########
@@ -79,3 +102,198 @@ app.kubernetes.io/instance: {{ .Release.Name }}
{{- default "default" .Values.serviceAccount.name }}
{{- end }}
{{- end }}
+
+{{/*
+Merges the advanced configuration into the destination map.
+*/}}
+{{- define "polaris.mergeAdvancedConfig" -}}
+{{- $advConfig := index . 0 -}}
+{{- $prefix := index . 1 -}}
+{{- $dest := index . 2 -}}
+{{- range $key, $val := $advConfig -}}
+{{- $name := ternary $key (print $prefix "." $key) (eq $prefix "") -}}
+{{- if kindOf $val | eq "map" -}}
+{{- list $val $name $dest | include "polaris.mergeAdvancedConfig" -}}
+{{- else -}}
+{{- $_ := set $dest $name $val -}}
+{{- end -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Prints the configuration option to the destination configmap entry. See
confimap.yaml.
+Any nil values will be printed as empty config options; otherwise, the value
will be evaluated
+as a template against the global context, then printed. Furthermore, if the
value contains
+line breaks, they will be escaped and a multi-line option will be printed.
+*/}}
+{{- define "polaris.appendConfigOption" -}}
+{{- $key := index . 0 -}}
+{{- $value := index . 1 -}}
+{{- $global := index . 2 -}}
+{{- $valAsString := "" -}}
+{{- if ne $value nil -}}
+{{- $valAsString = tpl (toString $value) $global -}}
+{{- if contains "\r\n" $valAsString -}}
+{{- $valAsString = $valAsString | nindent 4 | replace "\r\n" "\\\r\n" -}}
+{{- else if contains "\n" $valAsString -}}
+{{- $valAsString = $valAsString | nindent 4 | replace "\n" "\\\n" -}}
+{{- end -}}
+{{- end -}}
+{{ print $key "=" $valAsString }}
+{{- end -}}
+
+{{/*
+Convert a dict into a string formed by a comma-separated list of key-value
pairs: key1=value1,key2=value2, ...
+*/}}
+{{- define "polaris.dictToString" -}}
+{{- $list := list -}}
+{{- range $k, $v := . -}}
+{{- $list = append $list (printf "%s=%s" $k $v) -}}
+{{- end -}}
+{{ join "," $list }}
+{{- end -}}
+
+{{/*
+Prints the config volume definition for deployments and jobs.
+*/}}
+{{- define "polaris.configVolume" -}}
+- name: config-volume
+ projected:
+ sources:
+ - configMap:
+ name: {{ include "polaris.fullname" . }}
+ items:
+ - key: application.properties
+ path: application.properties
+ {{- if .Values.authentication.tokenBroker.secret.name }}
+ - secret:
+ name: {{ tpl .Values.authentication.tokenBroker.secret.name . }}
+ items:
+ {{- if eq .Values.authentication.tokenBroker.type "rsa-key-pair" }}
+ - key: {{ tpl .Values.authentication.tokenBroker.secret.publicKey .
}}
+ path: public.pem
+ - key: {{ tpl .Values.authentication.tokenBroker.secret.privateKey .
}}
+ path: private.pem
+ {{- end }}
+ {{- if eq .Values.authentication.tokenBroker.type "symmetric-key" }}
+ - key: {{ tpl .Values.authentication.tokenBroker.secret.secretKey .
}}
+ path: symmetric.key
+ {{- end }}
+ {{- end }}
+ {{- if and ( eq .Values.persistence.type "eclipse-link" )
.Values.persistence.eclipseLink.secret.name }}
+ - secret:
+ name: {{ tpl .Values.persistence.eclipseLink.secret.name . }}
+ items:
+ - key: {{ tpl .Values.persistence.eclipseLink.secret.key . }}
+ path: persistence.xml
+ {{- end }}
+{{- end -}}
+
+{{/*
+Converts a Kubernetes quantity to a number (int64 if possible or float64
otherwise).
+It handles raw numbers as well as quantities with suffixes
+like m, k, M, G, T, P, E, ki, Mi, Gi, Ti, Pi, Ei.
+It also handles scientific notation.
+Quantities should be positive, so negative values, zero, or any unparseable
number
+will result in a failure.
+https://kubernetes.io/docs/reference/kubernetes-api/common-definitions/quantity/
+*/}}
+{{- define "polaris.quantity" -}}
+{{- $quantity := . -}}
+{{- $n := $quantity | float64 -}}
+{{- if kindIs "string" $quantity -}}
+{{- if hasSuffix "m" $quantity -}}
+{{- $n = divf (trimSuffix "m" $quantity | float64) 1000.0 -}}
+{{- else if hasSuffix "k" $quantity -}}
+{{- $n = trimSuffix "k" $quantity | int64 | mul 1000 -}}
+{{- else if hasSuffix "M" $quantity -}}
+{{- $n = trimSuffix "M" $quantity | int64 | mul 1000000 -}}
+{{- else if hasSuffix "G" $quantity -}}
+{{- $n = trimSuffix "G" $quantity | int64 | mul 1000000000 -}}
+{{- else if hasSuffix "T" $quantity -}}
+{{- $n = trimSuffix "T" $quantity | int64 | mul 1000000000000 -}}
+{{- else if hasSuffix "P" $quantity -}}
+{{- $n = trimSuffix "P" $quantity | int64 | mul 1000000000000000 -}}
+{{- else if hasSuffix "E" $quantity -}}
+{{- $n = trimSuffix "E" $quantity | int64 | mul 1000000000000000000 -}}
+{{- else if hasSuffix "ki" $quantity -}}
+{{- $n = trimSuffix "ki" $quantity | int64 | mul 1024 -}}
+{{- else if hasSuffix "Mi" $quantity -}}
+{{- $n = trimSuffix "Mi" $quantity | int64 | mul 1048576 -}}
+{{- else if hasSuffix "Gi" $quantity -}}
+{{- $n = trimSuffix "Gi" $quantity | int64 | mul 1073741824 -}}
+{{- else if hasSuffix "Ti" $quantity -}}
+{{- $n = trimSuffix "Ti" $quantity | int64 | mul 1099511627776 -}}
+{{- else if hasSuffix "Pi" $quantity -}}
+{{- $n = trimSuffix "Pi" $quantity | int64 | mul 1125899906842624 -}}
+{{- else if hasSuffix "Ei" $quantity -}}
+{{- $n = trimSuffix "Ei" $quantity | int64 | mul 1152921504606846976 -}}
+{{- end -}}
+{{- end -}}
+{{- if le ($n | float64) 0.0 -}}
+{{- fail (print "invalid quantity: " $quantity) -}}
+{{- end -}}
+{{- if kindIs "float64" $n -}}
+{{- printf "%f" $n -}}
+{{- else -}}
+{{- printf "%v" $n -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Prints the ports section of the container spec. Also validates all port names
and numbers to ensure
+that they are consistent and that there are no overlaps.
+*/}}
+{{- define "polaris.containerPorts" -}}
+ports:
+{{- $ports := dict -}}
+
+{{- /* Main service ports */ -}}
+{{- range $i, $port := .Values.service.ports -}}
+{{- if hasKey $ports $port.name -}}
+{{- fail (printf "service.ports[%d]: port name already taken: %v" $i
$port.name) -}}
+{{- end -}}
+{{- $portNumber := coalesce $port.targetPort $port.port -}}
+{{- if has $portNumber (values $ports) -}}
+{{- fail (printf "service.ports[%d]: port number already taken: %v" $i
$portNumber) -}}
+{{- end -}}
+{{- $_ := set $ports $port.name $portNumber }}
Review Comment:
nits: lines 261-264 and 276-278, may want to add 2 more space for each of
them to fix indentation (still valid yaml files)
##########
helm/polaris/templates/tests/test-connection.yaml:
##########
@@ -35,5 +35,7 @@ spec:
- name: wget
image: busybox
command: ['wget']
- args: ['{{ include "polaris.fullname" . }}:{{ index
.Values.service.ports "polaris-metrics" }}/q/health']
+ args:
+ - --spider
+ - '{{ include "polaris.fullnameWithSuffix" (list . "mgmt") }}:{{ get
(first .Values.managementService.ports) "port" }}/q/health/ready'
restartPolicy: Never
Review Comment:
Seem likes now the pod will take some time to start up. From my local
testing, this will always fail. Should we consider add an init container to
sleep for X seconds before run the connection test? Here is a sample change:
```
initContainers:
- name: sleep
image: busybox
command: ['sleep', '30']
```
##########
helm/polaris/templates/_helpers.tpl:
##########
@@ -79,3 +102,198 @@ app.kubernetes.io/instance: {{ .Release.Name }}
{{- default "default" .Values.serviceAccount.name }}
{{- end }}
{{- end }}
+
+{{/*
+Merges the advanced configuration into the destination map.
+*/}}
+{{- define "polaris.mergeAdvancedConfig" -}}
+{{- $advConfig := index . 0 -}}
+{{- $prefix := index . 1 -}}
+{{- $dest := index . 2 -}}
+{{- range $key, $val := $advConfig -}}
+{{- $name := ternary $key (print $prefix "." $key) (eq $prefix "") -}}
+{{- if kindOf $val | eq "map" -}}
+{{- list $val $name $dest | include "polaris.mergeAdvancedConfig" -}}
+{{- else -}}
+{{- $_ := set $dest $name $val -}}
+{{- end -}}
+{{- end -}}
+{{- end -}}
+
+{{/*
+Prints the configuration option to the destination configmap entry. See
confimap.yaml.
+Any nil values will be printed as empty config options; otherwise, the value
will be evaluated
+as a template against the global context, then printed. Furthermore, if the
value contains
+line breaks, they will be escaped and a multi-line option will be printed.
+*/}}
+{{- define "polaris.appendConfigOption" -}}
+{{- $key := index . 0 -}}
+{{- $value := index . 1 -}}
+{{- $global := index . 2 -}}
+{{- $valAsString := "" -}}
+{{- if ne $value nil -}}
+{{- $valAsString = tpl (toString $value) $global -}}
+{{- if contains "\r\n" $valAsString -}}
+{{- $valAsString = $valAsString | nindent 4 | replace "\r\n" "\\\r\n" -}}
+{{- else if contains "\n" $valAsString -}}
+{{- $valAsString = $valAsString | nindent 4 | replace "\n" "\\\n" -}}
+{{- end -}}
+{{- end -}}
+{{ print $key "=" $valAsString }}
+{{- end -}}
+
+{{/*
+Convert a dict into a string formed by a comma-separated list of key-value
pairs: key1=value1,key2=value2, ...
+*/}}
+{{- define "polaris.dictToString" -}}
+{{- $list := list -}}
+{{- range $k, $v := . -}}
+{{- $list = append $list (printf "%s=%s" $k $v) -}}
+{{- end -}}
+{{ join "," $list }}
+{{- end -}}
+
+{{/*
+Prints the config volume definition for deployments and jobs.
+*/}}
+{{- define "polaris.configVolume" -}}
+- name: config-volume
+ projected:
+ sources:
+ - configMap:
+ name: {{ include "polaris.fullname" . }}
+ items:
+ - key: application.properties
+ path: application.properties
+ {{- if .Values.authentication.tokenBroker.secret.name }}
+ - secret:
+ name: {{ tpl .Values.authentication.tokenBroker.secret.name . }}
+ items:
+ {{- if eq .Values.authentication.tokenBroker.type "rsa-key-pair" }}
+ - key: {{ tpl .Values.authentication.tokenBroker.secret.publicKey .
}}
Review Comment:
nits: lines 173-175 and 179-180, may want to add 2 more space for each of
them to fix indentation (still valid yaml files)
##########
helm/polaris/README.md:
##########
@@ -45,41 +45,105 @@ A Helm chart for Polaris.
### Optional
-When using a custom `persistence.xml`, a Kubernetes Secret must be created for
`.persistenceConfigSecret`. Below is a sample command:
+When using a custom `persistence.xml`, a Kubernetes Secret must be created for
it. Below is a sample command:
```bash
kubectl create secret generic polaris-secret -n polaris
--from-file=persistence.xml
```
### From local directory (for development purposes)
-From Polaris repo root:
+The below instructions assume Minikube is running and Helm is installed.
+
+If necessary, load the Docker images into Minikube:
+
+```bash
+eval $(minikube -p minikube docker-env)
Review Comment:
nits: I used [kind](https://kind.sigs.k8s.io/) a lot more as oppose to
minikube (both are great...kind is very lightwight and super fast). I am okay
with switching to minikube, we may want to update
https://github.com/apache/polaris/blob/main/run.sh as well to match the same.
##########
.github/workflows/helm.yml:
##########
@@ -99,7 +99,7 @@ jobs:
if: steps.list-changed.outputs.changed == 'true'
run: |
eval $(minikube -p minikube docker-env)
- ./gradlew :polaris-quarkus-server:assemble \
+ ./gradlew :polaris-quarkus-server:assemble
:polaris-quarkus-admin:assemble \
-Dquarkus.container-image.build=true \
-PeclipseLinkDeps=com.h2database:h2:2.3.232
Review Comment:
While testing the helm, I noticed the bootstrap pod will shows the bootstrap
is completed. However, when trying to use the polaris CLI against the server,
it will say the instance is not yet bootstrapped. Not sure if this is known
issue?
--
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]
