chore: init event queue #20
Conversation
adityachoudhari26
requested review from
jsbroks and
zacharyblasczyk
as code owners
adityachoudhari26
had a problem deploying
to
Helm Charts
GitHub Actions
Failure
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces a new Helm subchart Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Helm as Helm (ctrlplane)
participant K8s as Kubernetes API
participant Dep as Deployment (event-queue)
participant HPA as HPA
participant Pod as Pod
participant Sec as Secrets
participant Kafka as Kafka (global.kafkaBrokers)
User->>Helm: helm install/upgrade ctrlplane (v0.4.0)
Helm->>K8s: Apply parent resources + event-queue subchart
K8s-->>Dep: Create/Update Deployment (event-queue)
K8s-->>HPA: Create/Update HPA (cpu target 70%)
Dep->>K8s: Manage ReplicaSet / Pods
K8s-->>Pod: Schedule Pod(s)
Pod->>Sec: Read POSTGRES_URL, AES key, GitHub bot creds
Pod->>Kafka: Connect to global.kafkaBrokers
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
adityachoudhari26
temporarily deployed
to
Helm Charts
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (11)
charts/ctrlplane/charts/event-queue/values.yaml (3)
6-10: Avoid Always pull policy by default.Use IfNotPresent to reduce registry load and cold‑start latency in most environments.
image: repository: ctrlplane/event-queue tag: latest - pullPolicy: Always + pullPolicy: IfNotPresent
33-39: Right-size default resources.Defaults (1 vCPU req, 4 vCPU limit, 1–4 GiB) may cause unnecessary scheduling failures on small clusters. Consider lower, safer defaults.
resources: requests: - cpu: 1000m - memory: 1Gi + cpu: 250m + memory: 256Mi limits: - cpu: 4000m - memory: 4Gi + cpu: 1000m + memory: 1Gi
24-26: Expose pod security context in values.You reference
pod.securityContextin the Deployment; define a stub to make it discoverable/configurable.pod: securityContext: {}charts/ctrlplane/charts/event-queue/templates/serviceaccount.yaml (1)
1-16: Guard optional metadata keys and add EOF newline.Avoid emitting empty
annotations:/labels:blocks and add a trailing newline.{{- if .Values.serviceAccount.create -}} apiVersion: v1 kind: ServiceAccount metadata: name: {{ include "event-queue.serviceAccountName" . }} namespace: {{ $.Release.Namespace }} labels: - {{- include "event-queue.labels" . | nindent 4 }} - {{- if .Values.serviceAccount.labels -}} - {{- toYaml .Values.serviceAccount.labels | nindent 4 }} - {{- end }} - annotations: - {{- if .Values.serviceAccount.annotations -}} - {{- toYaml .Values.serviceAccount.annotations | nindent 4 }} - {{- end }} + {{- include "event-queue.labels" . | nindent 4 }} + {{- with .Values.serviceAccount.labels }} + {{- toYaml . | nindent 4 }} + {{- end }} + {{- with .Values.serviceAccount.annotations }} + annotations: + {{- toYaml . | nindent 4 }} + {{- end }} {{- end }} +charts/ctrlplane/charts/event-queue/templates/hpa.yaml (1)
1-28: Parameterize CPU target and add EOF newline.Expose CPU target in values (e.g.,
hpa.targetCPUUtilizationPercentage, default 70) and ensure file ends with newline.metrics: - type: Resource resource: name: cpu target: type: Utilization - averageUtilization: 70 + averageUtilization: {{ default 70 .Values.hpa.targetCPUUtilizationPercentage }} +charts/ctrlplane/charts/event-queue/templates/deployment.yaml (4)
40-41: Honor imagePullPolicy from values.image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" + imagePullPolicy: {{ .Values.image.pullPolicy }}
71-90: Quote GITHUB_BOT_NAME for safety.Consistent quoting avoids YAML/templating surprises.
- name: GITHUB_BOT_NAME - value: {{ .name }} + value: {{ .name | quote }}
1-1: Remove unused local var.
$imageCfgis defined but never used.-{{- $imageCfg := dict "global" $.Values.global.image "local" $.Values.image -}}
58-95: EOF newline.Add a trailing newline to satisfy linters.
charts/ctrlplane/charts/event-queue/templates/_helpers.tpl (2)
13-24: Safer fullname logic: avoid substring collisions in contains check.Using contains can falsely match incidental substrings (e.g., release "rapid" vs chart "api"). Prefer equality or a hyphen‑delimited suffix match.
Apply this minimal change:
-{{- if contains $name .Release.Name }} +{{- if or (eq .Release.Name $name) (hasSuffix (printf "-%s" $name) .Release.Name) }}
36-43: Optional: add app.kubernetes.io/part-of for cross‑chart grouping.Helps UIs and policies group resources (e.g., all components of “ctrlplane”). Wire it via a global value so it’s optional.
{{- define "event-queue.labels" -}} helm.sh/chart: {{ include "event-queue.chart" . }} {{ include "event-queue.selectorLabels" . }} {{- if .Chart.AppVersion }} app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} {{- end }} +{{- with .Values.global.partOf }} +app.kubernetes.io/part-of: {{ . | quote }} +{{- end }} app.kubernetes.io/managed-by: {{ .Release.Service }} {{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
charts/ctrlplane/Chart.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
charts/ctrlplane/Chart.yaml(2 hunks)charts/ctrlplane/charts/event-queue/.helmignore(1 hunks)charts/ctrlplane/charts/event-queue/Chart.yaml(1 hunks)charts/ctrlplane/charts/event-queue/templates/_helpers.tpl(1 hunks)charts/ctrlplane/charts/event-queue/templates/deployment.yaml(1 hunks)charts/ctrlplane/charts/event-queue/templates/hpa.yaml(1 hunks)charts/ctrlplane/charts/event-queue/templates/serviceaccount.yaml(1 hunks)charts/ctrlplane/charts/event-queue/values.yaml(1 hunks)charts/ctrlplane/values.yaml(1 hunks)
🧰 Additional context used
🪛 YAMLlint (1.37.1)
charts/ctrlplane/charts/event-queue/templates/hpa.yaml
[error] 7-7: syntax error: expected the node content, but found '-'
(syntax)
[warning] 13-13: too many spaces after hyphen
(hyphens)
[error] 28-28: no new line character at the end of file
(new-line-at-end-of-file)
charts/ctrlplane/charts/event-queue/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: too many spaces after hyphen
(hyphens)
[warning] 14-14: too many spaces after hyphen
(hyphens)
[error] 16-16: no new line character at the end of file
(new-line-at-end-of-file)
charts/ctrlplane/charts/event-queue/templates/deployment.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 9-9: too many spaces after hyphen
(hyphens)
[warning] 13-13: too many spaces after hyphen
(hyphens)
[warning] 27-27: too many spaces after hyphen
(hyphens)
[error] 96-96: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (3)
charts/ctrlplane/charts/event-queue/.helmignore (1)
1-24: .helmignore looks good.charts/ctrlplane/charts/event-queue/Chart.yaml (1)
1-6: LGTM.Chart metadata is valid. Optional: add maintainers/home/sources later.
charts/ctrlplane/charts/event-queue/templates/_helpers.tpl (1)
4-6: Solid naming helper; mirrors Helm defaults and is DNS‑safe.No issues. Truncation and trailing dash trim are correct.
| {{- define "event-queue.selectorLabels" -}} | ||
| app.kubernetes.io/name: {{ include "event-queue.name" . }} | ||
| app.kubernetes.io/instance: {{ .Release.Name }} | ||
| {{- end }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Selector labels are stable and appropriate.
Good separation from general labels; this should keep Service selectors and Deployments aligned across upgrades.
Run to confirm all matchLabels use the same helper:
🏁 Script executed:
#!/bin/bash
# Verify selectorLabels are used consistently
rg -n -C2 'matchLabels' charts/ctrlplane/charts/event-queue/templates \
| rg -n 'include "event-queue.selectorLabels"'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
dir='charts/ctrlplane/charts/event-queue/templates'
if [[ ! -d "$dir" ]]; then
echo "DIR_NOT_FOUND:$dir"
exit 0
fi
echo "---- matchLabels occurrences ----"
rg -n -S 'matchLabels' "$dir" -C2 || true
echo
echo "---- include \"event-queue.selectorLabels\" occurrences ----"
rg -n -S 'include "event-queue.selectorLabels"' "$dir" -C2 || true
echo
echo "---- Per-file check for files containing 'matchLabels' ----"
rg -l -S 'matchLabels' "$dir" | while IFS= read -r f; do
printf "FILE: %s\n" "$f"
if rg -q -S 'include "event-queue.selectorLabels"' "$f"; then
echo " -> contains include: YES"
else
echo " -> contains include: NO"
fi
doneLength of output: 59
🏁 Script executed:
#!/bin/sh
set -eu
if ! command -v rg >/dev/null 2>&1; then
printf 'RG_NOT_FOUND\n'
exit 0
fi
echo "---- Helper definition occurrences (searching charts/) ----"
rg -n -S 'define "event-queue.selectorLabels"' charts -C2 || true
echo
echo '---- include "event-queue.selectorLabels" occurrences (searching charts/) ----'
rg -n -S 'include "event-queue.selectorLabels"' charts -C2 || true
echo
echo "---- matchLabels occurrences under charts/ctrlplane ----"
if [ -d "charts/ctrlplane" ]; then
rg -n -S 'matchLabels' charts/ctrlplane -C2 || true
else
echo "DIR_NOT_FOUND:charts/ctrlplane"
fi
echo
echo "---- Per-file check for files containing 'matchLabels' under charts/ctrlplane ----"
if [ -d "charts/ctrlplane" ]; then
rg -l -S 'matchLabels' charts/ctrlplane || true | while IFS= read -r f; do
printf 'FILE: %s\n' "$f"
if rg -q -S 'include "event-queue.selectorLabels"' "$f"; then
echo " -> contains include: YES"
else
echo " -> contains include: NO"
fi
done
fiLength of output: 4044
Use selector-only labels in Deployment selectors (not full .labels).
Deployment matchLabels include the full ".labels" (which adds helm.sh/chart and optional app.kubernetes.io/version); those are not stable and must not be part of selectors. Use only ".selectorLabels" (or remove chart/version from selector labels) so selectors remain immutable across upgrades.
Affected files:
- charts/ctrlplane/charts/event-queue/templates/deployment.yaml
- charts/ctrlplane/charts/webservice/templates/deployment.yaml
- charts/ctrlplane/charts/pty-proxy/templates/deployment.yaml
- charts/ctrlplane/charts/otel/templates/deployment.yaml
- charts/ctrlplane/charts/event-worker/templates/deployment.yaml
Fix: in each deployment's matchLabels replace include ".labels" with include ".selectorLabels".
🤖 Prompt for AI Agents
In charts/ctrlplane/charts/event-queue/templates/_helpers.tpl around lines 48 to
51, the helper defined for selector labels is correct but Deployment resources
are currently using the full ".labels" (which includes helm.sh/chart and
app.kubernetes.io/version) in their matchLabels; update each affected Deployment
(charts/ctrlplane/charts/event-queue/templates/deployment.yaml,
charts/ctrlplane/charts/webservice/templates/deployment.yaml,
charts/ctrlplane/charts/pty-proxy/templates/deployment.yaml,
charts/ctrlplane/charts/otel/templates/deployment.yaml,
charts/ctrlplane/charts/event-worker/templates/deployment.yaml) to replace
include "<component>.labels" with include "<component>.selectorLabels" in
matchLabels so selectors contain only stable, immutable labels.
| {{- define "event-queue.serviceAccountName" -}} | ||
| {{- if .Values.serviceAccount.create }} | ||
| {{- default (include "event-queue.fullname" .) .Values.serviceAccount.name }} | ||
| {{- else }} | ||
| {{- default "default" .Values.serviceAccount.name }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Guard against creating a ServiceAccount named “default”.
If users set serviceAccount.create=true and serviceAccount.name=default, the chart may attempt to create the preexisting “default” SA. Ensure the manifest gates creation appropriately.
Suggested guard in templates/serviceaccount.yaml (outside this file):
{{- if and .Values.serviceAccount.create (ne (include "event-queue.serviceAccountName" .) "default") }}
apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ include "event-queue.serviceAccountName" . }}
labels:
{{- include "event-queue.labels" . | nindent 4 }}
{{- end }}Quick checks:
🏁 Script executed:
#!/bin/bash
# Ensure SA manifest is gated by create and not generated for 'default'
rg -nP -C2 '(?m)^\s*{{-?\s*if\s+.*serviceAccount\.create' charts/ctrlplane/charts/event-queue/templates/serviceaccount.yaml || echo "Missing create gate"
rg -n 'include "event-queue.serviceAccountName"' charts/ctrlplane/charts/event-queue/templates/serviceaccount.yaml || echo "SA name helper not used"
# Ensure Deployment uses the helper
rg -n 'serviceAccountName:.*include "event-queue.serviceAccountName"' charts/ctrlplane/charts/event-queue/templates/deployment.yaml || echo "Deployment not using SA name helper"Length of output: 219
Prevent creating the cluster "default" ServiceAccount
serviceaccount.yaml currently only checks .Values.serviceAccount.create; add a name check so the chart won't try to create the preexisting "default" SA.
File: charts/ctrlplane/charts/event-queue/templates/serviceaccount.yaml
Suggested guard:
{{- if and .Values.serviceAccount.create (ne (include "event-queue.serviceAccountName" .) "default") }}
apiVersion: v1
kind: ServiceAccount
metadata:
name: {{ include "event-queue.serviceAccountName" . }}
labels:
{{- include "event-queue.labels" . | nindent 4 }}
{{- end }}🤖 Prompt for AI Agents
In charts/ctrlplane/charts/event-queue/templates/serviceaccount.yaml around
where the ServiceAccount is conditionally created (lines referenced by the
review), the template only checks .Values.serviceAccount.create and can attempt
to create the cluster "default" ServiceAccount; change the conditional to
require both .Values.serviceAccount.create and that the resolved serviceAccount
name (include "event-queue.serviceAccountName" .) is not "default" so the chart
will not try to create the built-in default SA. Wrap the ServiceAccount resource
in an if that uses and .Values.serviceAccount.create (ne (include
"event-queue.serviceAccountName" .) "default") and leave the existing helper
unchanged.
| selector: | ||
| matchLabels: | ||
| {{- include "ctrlplane.selectorLabels" $ | nindent 6 }} | ||
| {{- include "event-queue.labels" . | nindent 6 }} | ||
| template: |
There was a problem hiding this comment.
Fix selector to match pod labels (avoid cross-chart helper).
matchLabels must be a subset of pod template labels. Using ctrlplane.selectorLabels risks mismatch. Use your chart’s selector labels only.
selector:
matchLabels:
- {{- include "ctrlplane.selectorLabels" $ | nindent 6 }}
- {{- include "event-queue.labels" . | nindent 6 }}
+ {{- include "event-queue.selectorLabels" . | nindent 6 }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| selector: | |
| matchLabels: | |
| {{- include "ctrlplane.selectorLabels" $ | nindent 6 }} | |
| {{- include "event-queue.labels" . | nindent 6 }} | |
| template: | |
| selector: | |
| matchLabels: | |
| {{- include "event-queue.selectorLabels" . | nindent 6 }} | |
| template: |
🤖 Prompt for AI Agents
In charts/ctrlplane/charts/event-queue/templates/deployment.yaml around lines 17
to 21, the Deployment selector is including helper from the ctrlplane chart
which can mismatch the pod template labels; remove the include of
"ctrlplane.selectorLabels" and instead use only this chart's selector labels
(e.g., include "event-queue.selectorLabels" or the same "event-queue.labels"
helper), and ensure the pod template.metadata.labels exactly match that selector
so matchLabels is a subset of the pod labels.
| {{- if .tolerations }} | ||
| tolerations: | ||
| {{- toYaml .tolerations | nindent 8 }} | ||
| {{- end }} |
There was a problem hiding this comment.
Reference values for tolerations.
.tolerations is undefined; use .Values.tolerations.
- {{- if .tolerations }}
+ {{- if .Values.tolerations }}
tolerations:
- {{- toYaml .tolerations | nindent 8 }}
+ {{- toYaml .Values.tolerations | nindent 8 }}
{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- if .tolerations }} | |
| tolerations: | |
| {{- toYaml .tolerations | nindent 8 }} | |
| {{- end }} | |
| {{- if .Values.tolerations }} | |
| tolerations: | |
| {{- toYaml .Values.tolerations | nindent 8 }} | |
| {{- end }} |
🤖 Prompt for AI Agents
In charts/ctrlplane/charts/event-queue/templates/deployment.yaml around lines 31
to 34, the template references `.tolerations` which is undefined; change it to
use `.Values.tolerations` (or the correct chart value path) and keep the
existing conditional and toYaml/nindent usage so the block renders only when the
value is set; ensure you reference `.Values` consistently (e.g., `{{- if
.Values.tolerations }}`) to avoid nil template errors.
| {{- include "ctrlplane.nodeSelector" . | nindent 6 }} | ||
| {{- include "ctrlplane.priorityClassName" . | nindent 6 }} | ||
| {{- include "ctrlplane.podSecurityContext" .Values.pod.securityContext | nindent 6 }} |
There was a problem hiding this comment.
Pass the correct context to podSecurityContext helper.
Most helpers expect the root (.). Passing the map likely breaks rendering.
- {{- include "ctrlplane.podSecurityContext" .Values.pod.securityContext | nindent 6 }}
+ {{- include "ctrlplane.podSecurityContext" . | nindent 6 }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- include "ctrlplane.nodeSelector" . | nindent 6 }} | |
| {{- include "ctrlplane.priorityClassName" . | nindent 6 }} | |
| {{- include "ctrlplane.podSecurityContext" .Values.pod.securityContext | nindent 6 }} | |
| {{- include "ctrlplane.nodeSelector" . | nindent 6 }} | |
| {{- include "ctrlplane.priorityClassName" . | nindent 6 }} | |
| {{- include "ctrlplane.podSecurityContext" . | nindent 6 }} |
🤖 Prompt for AI Agents
In charts/ctrlplane/charts/event-queue/templates/deployment.yaml around lines 35
to 37, the podSecurityContext helper is being called with the map
(.Values.pod.securityContext) instead of the full chart context (.), which can
break template rendering; call the helper with the root context (.) so the
helper can access .Values internally (replace the current include argument with
.) and keep the existing nindent 6 formatting.
Summary by CodeRabbit
New Features
Chores