Copilot commented on code in PR #18793:
URL: https://github.com/apache/pinot/pull/18793#discussion_r3554053597
##########
helm/pinot/templates/namespace.yaml:
##########
@@ -0,0 +1,31 @@
+{{- if .Values.namespace.create }}
Review Comment:
`namespace.yaml` directly dereferences `.Values.namespace.create`. If users
upgrade with `helm upgrade --reuse-values` from a previous chart version (where
`namespace` didn't exist), this will error during template rendering. Use a
safe default for `.Values.namespace` before reading `.create`.
##########
helm/pinot/templates/controller/statefulset.yaml:
##########
@@ -24,6 +24,7 @@ metadata:
namespace: {{ include "pinot.namespace" . }}
labels:
{{- include "pinot.controllerLabels" . | nindent 4 }}
+ annotations: {{ toYaml .Values.controller.annotations | nindent 4 }}
Review Comment:
Rendering `metadata.annotations` from `.Values.controller.annotations`
without a default can produce `annotations: null` when `controller.annotations`
is absent (e.g., `helm upgrade --reuse-values`), which Kubernetes rejects
because annotations must be a map. Default to an empty dict before calling
`toYaml`.
##########
helm/pinot/templates/controller/statefulset.yaml:
##########
@@ -65,7 +66,7 @@ spec:
- name: controller
securityContext:
{{- toYaml .Values.controller.securityContext | nindent 10 }}
- image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
+ image: "{{ .Values.controllerImage.repository | default
.Values.image.repository }}:{{ .Values.controllerImage.tag | default
.Values.image.tag }}"
Review Comment:
`controllerImage` is a new values key; during `helm upgrade --reuse-values`
it may be missing entirely, and `.Values.controllerImage.repository` can fail
template rendering. Guard with `default (dict)` around the map before accessing
`repository`/`tag`.
##########
helm/pinot/templates/server/statefulset.yaml:
##########
@@ -65,7 +66,7 @@ spec:
- name: server
securityContext:
{{- toYaml .Values.server.securityContext | nindent 10 }}
- image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
+ image: "{{ .Values.serverImage.repository | default
.Values.image.repository }}:{{ .Values.serverImage.tag | default
.Values.image.tag }}"
Review Comment:
`serverImage` is a new values key; during `helm upgrade --reuse-values` it
may be missing and `.Values.serverImage.repository` can fail template
rendering. Guard with `default (dict)` around the map before reading
`repository`/`tag`.
##########
helm/pinot/templates/broker/statefulset.yaml:
##########
@@ -24,6 +24,7 @@ metadata:
namespace: {{ include "pinot.namespace" . }}
labels:
{{- include "pinot.brokerLabels" . | nindent 4 }}
+ annotations: {{ toYaml .Values.broker.annotations | nindent 4 }}
Review Comment:
Rendering `metadata.annotations` from `.Values.broker.annotations` without a
default can produce `annotations: null` when the key is absent (e.g., `helm
upgrade --reuse-values`), which Kubernetes rejects. Default to an empty dict
before calling `toYaml`.
##########
helm/pinot/templates/broker/statefulset.yaml:
##########
@@ -65,7 +66,7 @@ spec:
- name: broker
securityContext:
{{- toYaml .Values.broker.securityContext | nindent 10 }}
- image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
+ image: "{{ .Values.brokerImage.repository | default
.Values.image.repository }}:{{ .Values.brokerImage.tag | default
.Values.image.tag }}"
Review Comment:
`brokerImage` is a new values key; during `helm upgrade --reuse-values` it
may be missing and `.Values.brokerImage.repository` can fail template
rendering. Guard with `default (dict)` around the map before reading
`repository`/`tag`.
##########
helm/pinot/templates/minion-stateless/deployment.yaml:
##########
@@ -57,7 +58,7 @@ spec:
- name: minion-stateless
securityContext:
{{- toYaml .Values.minionStateless.securityContext | nindent 10 }}
- image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
+ image: "{{ .Values.minionImage.repository | default
.Values.image.repository }}:{{ .Values.minionImage.tag | default
.Values.image.tag }}"
Review Comment:
`minionImage` is a new values key; during `helm upgrade --reuse-values` it
may be missing and `.Values.minionImage.repository` can fail template
rendering. Guard with `default (dict)` around the map before reading
`repository`/`tag`.
##########
helm/pinot/templates/minion/statefulset.yaml:
##########
@@ -66,7 +67,7 @@ spec:
- name: minion
securityContext:
{{- toYaml .Values.minion.securityContext | nindent 10 }}
- image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}"
+ image: "{{ .Values.minionImage.repository | default
.Values.image.repository }}:{{ .Values.minionImage.tag | default
.Values.image.tag }}"
Review Comment:
`minionImage` is a new values key; during `helm upgrade --reuse-values` it
may be missing and `.Values.minionImage.repository` can fail template
rendering. Guard with `default (dict)` around the map before reading
`repository`/`tag`.
##########
helm/pinot/templates/minion-stateless/deployment.yaml:
##########
@@ -25,6 +25,7 @@ metadata:
namespace: {{ include "pinot.namespace" . }}
labels:
{{- include "pinot.minionStatelessLabels" . | nindent 4 }}
+ annotations: {{ toYaml .Values.minionStateless.annotations | nindent 4 }}
Review Comment:
Rendering `metadata.annotations` from `.Values.minionStateless.annotations`
without a default can produce `annotations: null` when the key is absent (e.g.,
`helm upgrade --reuse-values`), which Kubernetes rejects. Default to an empty
dict before calling `toYaml`.
##########
helm/pinot/templates/server/configmap.yaml:
##########
@@ -24,10 +24,10 @@ metadata:
namespace: {{ include "pinot.namespace" . }}
data:
pinot-server.conf: |-
-{{ if .Values.server.ConfigureServerPort }}
+{{- if .Values.server.ConfigureServerPort }}
Review Comment:
This configmap gate uses `.Values.server.ConfigureServerPort` (capital C),
but `values.yaml` defines `server.configureServerPort`. As written, the
`pinot.server.netty.port`/`adminapi.port` lines will never be rendered with the
default values. Align the key name (optionally supporting the legacy-cased key
for compatibility).
##########
helm/pinot/values.yaml:
##########
@@ -35,6 +37,11 @@ image:
tag: latest # 1.0.0, 0.12.1, latest
pullPolicy: Always # Use IfNotPresent when you pinged a version of image tag
+controllerImage: {}
+brokerImage: {}
+serverImage: {}
+minionImage: {}
Review Comment:
PR description says the `minionImage` override is applied to the controller
workload, but the templates actually use `controllerImage` for controller and
`minionImage` only for minion/minion-stateless. Please reconcile the
description vs. the implemented values keys so chart users know which override
to set.
##########
helm/pinot/templates/server/statefulset.yaml:
##########
@@ -24,6 +24,7 @@ metadata:
namespace: {{ include "pinot.namespace" . }}
labels:
{{- include "pinot.serverLabels" . | nindent 4 }}
+ annotations: {{ toYaml .Values.server.annotations | nindent 4 }}
Review Comment:
Rendering `metadata.annotations` from `.Values.server.annotations` without a
default can produce `annotations: null` when the key is absent (e.g., `helm
upgrade --reuse-values`), which Kubernetes rejects. Default to an empty dict
before calling `toYaml`.
##########
helm/pinot/templates/minion/statefulset.yaml:
##########
@@ -25,6 +25,7 @@ metadata:
namespace: {{ include "pinot.namespace" . }}
labels:
{{- include "pinot.minionLabels" . | nindent 4 }}
+ annotations: {{ toYaml .Values.minion.annotations | nindent 4 }}
Review Comment:
Rendering `metadata.annotations` from `.Values.minion.annotations` without a
default can produce `annotations: null` when the key is absent (e.g., `helm
upgrade --reuse-values`), which Kubernetes rejects. Default to an empty dict
before calling `toYaml`.
--
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]
