[INFRA-437] - feat(plane-enterprise): configurable labels & annotations for all resources, pods, and one-shot jobs#262
Conversation
…rces, pods, and one-shot jobs Adds a three-tier (plus job-override) configurable metadata scheme on top of the standard recommended labels from #256, following the Grafana/GitLab chart conventions: - commonLabels / commonAnnotations: chart-wide, applied to every object the chart renders (Deployments, StatefulSets, Jobs, Services, ConfigMaps, Secrets, certs, Ingress/IngressRoute, ServiceAccount, Middleware) and to every pod template. - services.<svc>.labels / annotations: one workload's object metadata (pre-existing keys, now merged with the common tier). - services.<svc>.podLabels / podAnnotations: one workload's pod template only - what NetworkPolicies, service meshes and Prometheus match on. Added for all 22 services. - services.api.migrator / services.pi.migrator / services.minio.bucketJob: override tier for the one-shot Jobs. Jobs inherit their parent service's maps; keys set here win on conflict (e.g. sidecar.istio.io/inject "false" so migration Jobs complete on meshed clusters, or ArgoCD hook annotations that belong on the Job object only). Precedence: job override > per-service > common > standard labels. Nothing is ever injected into spec.selector/matchLabels, so helm upgrade of an existing release never trips the immutable-selector error. Helpers: plane.commonLabels (from #256) renamed to plane.standardLabels to free the "common" name for the user-facing values key (GitLab's naming); new plane.resourceLabels / plane.resourceAnnotations / plane.podLabels / plane.podAnnotations compose the tiers; plane.labelsAndAnnotations extended with the common + job tiers. Also fixes, found while testing: - MinIO bucket Job pod template had no labels at all (missed in #256 too). - pi-migrator Job object metadata skipped services.pi.labels while its pods used services.pi.podLabels - now consistent with the other Jobs. - Traefik Middleware maxRequestBodyBytes rendered as scientific notation (2.097152e+07), which the CRD's integer field can reject - now | int64. Validated with helm lint and a 6-scenario render-assertion suite (~2,500 assertions: precedence conflicts, selector immutability across labeled/unlabeled renders, nginx/traefik ingress annotation merging, standard-label override escape hatch, special-character annotation values). Chart version: 2.7.0 -> 2.8.0.
WalkthroughThis PR adds chart-wide ChangesConfigurable common/pod labels and annotations
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
Linked to Plane Work Item(s) References This comment was auto-generated by Plane |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
charts/plane-enterprise/templates/_helpers.tpl (1)
120-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstrings for
plane.podLabels/plane.podAnnotationsomit thejoboverride usage.Both helpers implement a
$job.podLabels/$job.podAnnotationstier (Lines 135-137, 152-153), but their doc comments only show the two-argument (context,values) call form, unlikeplane.labelsAndAnnotationsabove which explicitly documents thejobdict for one-shot Jobs (Lines 104-107). This omission plausibly contributed to the PR's own noted fix ("MinIO bucket job pod labels were missing") — callers may not realize pod-level job overrides are supported. Consider adding a job-override example to both docstrings for consistency and to prevent the same class of oversight recurring for future one-shot Jobs.📝 Suggested docstring addition
{{/* Pod-template labels as a bare map (no `labels:` key): the standard recommended labels + chart-wide `.Values.commonLabels` + the per-workload `podLabels` map, merged into one block (podLabels wins over commonLabels wins over the standard labels). Always non-empty, so callers append it under the pod template's `labels:` key (which already carries the static `app.name` selector label) without a `with` guard. The `app.name` label is emitted by the template itself and never included here, so the selector is never duplicated. Call with a dict carrying the root context and the component values: labels: app.name: ... {{- include "plane.podLabels" (dict "context" $ "values" .Values.services.api) | nindent 8 }} +One-shot Jobs that inherit a parent service's pod values may pass an optional job-override +map whose podLabels win over the parent's: + {{- include "plane.podLabels" (dict "context" $ "values" .Values.services.minio "job" .Values.services.minio.bucketJob) | nindent 8 }} */}}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/plane-enterprise/templates/_helpers.tpl` around lines 120 - 156, The docstrings for plane.podLabels and plane.podAnnotations only show the context/values call pattern, but both helpers also support a job override via the .job dict. Update the comments above these templates to document the $job.podLabels and $job.podAnnotations tier, and add an example call shape similar to plane.labelsAndAnnotations so users know one-shot Jobs can supply job-specific pod metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/plane-enterprise/templates/workloads/admin.deployment.yaml`:
- Around line 6-13: The merged labels in the admin workload template allow a
user-supplied app.name to conflict with the chart’s fixed app.name value, which
can break selector.matchLabels consistency. Update the workload label rendering
in the admin deployment template and the other similar workload templates so
app.name is either excluded from the included resource labels or always emitted
after the include as the authoritative fixed label, using the existing
plane.resourceLabels helper and the pod template/selector label blocks.
---
Nitpick comments:
In `@charts/plane-enterprise/templates/_helpers.tpl`:
- Around line 120-156: The docstrings for plane.podLabels and
plane.podAnnotations only show the context/values call pattern, but both helpers
also support a job override via the .job dict. Update the comments above these
templates to document the $job.podLabels and $job.podAnnotations tier, and add
an example call shape similar to plane.labelsAndAnnotations so users know
one-shot Jobs can supply job-specific pod metadata.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76806a9f-869b-4b51-a875-607a50379e15
📒 Files selected for processing (49)
charts/plane-enterprise/Chart.yamlcharts/plane-enterprise/README.mdcharts/plane-enterprise/templates/_helpers.tplcharts/plane-enterprise/templates/certs/cert-issuers.yamlcharts/plane-enterprise/templates/certs/certs.yamlcharts/plane-enterprise/templates/certs/email-certs.yamlcharts/plane-enterprise/templates/config-secrets/app-env.yamlcharts/plane-enterprise/templates/config-secrets/automations-consumer.yamlcharts/plane-enterprise/templates/config-secrets/doc-store.yamlcharts/plane-enterprise/templates/config-secrets/docker-registry.yamlcharts/plane-enterprise/templates/config-secrets/email-env.yamlcharts/plane-enterprise/templates/config-secrets/live-env.yamlcharts/plane-enterprise/templates/config-secrets/monitor.yamlcharts/plane-enterprise/templates/config-secrets/opensearchdb.yamlcharts/plane-enterprise/templates/config-secrets/outbox-poller.yamlcharts/plane-enterprise/templates/config-secrets/pgdb.yamlcharts/plane-enterprise/templates/config-secrets/pi-api-env.yamlcharts/plane-enterprise/templates/config-secrets/rabbitmqdb.yamlcharts/plane-enterprise/templates/config-secrets/runner-env.yamlcharts/plane-enterprise/templates/config-secrets/silo.yamlcharts/plane-enterprise/templates/ingress-traefik.yamlcharts/plane-enterprise/templates/ingress.yamlcharts/plane-enterprise/templates/service-account.yamlcharts/plane-enterprise/templates/traefik-middleware.yamlcharts/plane-enterprise/templates/workloads/admin.deployment.yamlcharts/plane-enterprise/templates/workloads/api.deployment.yamlcharts/plane-enterprise/templates/workloads/automation-consumer.deployment.yamlcharts/plane-enterprise/templates/workloads/beat-worker.deployment.yamlcharts/plane-enterprise/templates/workloads/email.deployment.yamlcharts/plane-enterprise/templates/workloads/iframely.deployment.yamlcharts/plane-enterprise/templates/workloads/live.deployment.yamlcharts/plane-enterprise/templates/workloads/migrator.job.yamlcharts/plane-enterprise/templates/workloads/minio.stateful.yamlcharts/plane-enterprise/templates/workloads/monitor.stateful.yamlcharts/plane-enterprise/templates/workloads/opensearch.stateful.yamlcharts/plane-enterprise/templates/workloads/outbox-poller.deployment.yamlcharts/plane-enterprise/templates/workloads/pi-api.deployment.yamlcharts/plane-enterprise/templates/workloads/pi-beat.deployment.yamlcharts/plane-enterprise/templates/workloads/pi-migrator.job.yamlcharts/plane-enterprise/templates/workloads/pi-worker.deployment.yamlcharts/plane-enterprise/templates/workloads/postgres.stateful.yamlcharts/plane-enterprise/templates/workloads/rabbitmq.stateful.yamlcharts/plane-enterprise/templates/workloads/redis.stateful.yamlcharts/plane-enterprise/templates/workloads/runner.deployment.yamlcharts/plane-enterprise/templates/workloads/silo.deployment.yamlcharts/plane-enterprise/templates/workloads/space.deployment.yamlcharts/plane-enterprise/templates/workloads/web.deployment.yamlcharts/plane-enterprise/templates/workloads/worker.deployment.yamlcharts/plane-enterprise/values.yaml
| name: {{ .Release.Name }}-admin | ||
| labels: | ||
| app.name: {{ .Release.Namespace }}-{{ .Release.Name }}-admin | ||
| {{- include "plane.commonLabels" $ | nindent 4 }} | ||
| {{- include "plane.resourceLabels" $ | nindent 4 }} | ||
| {{- with (include "plane.resourceAnnotations" $) }} | ||
| annotations: | ||
| {{- . | nindent 4 }} | ||
| {{- end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== files ==\n'
git ls-files 'charts/plane-enterprise/templates/**' 'charts/plane-enterprise/templates/_helpers.tpl' 'charts/plane-enterprise/values.yaml' | sed -n '1,200p'
printf '\n== helper outlines ==\n'
ast-grep outline charts/plane-enterprise/templates/_helpers.tpl --view expanded || true
printf '\n== workload outlines ==\n'
for f in \
charts/plane-enterprise/templates/workloads/admin.deployment.yaml \
charts/plane-enterprise/templates/workloads/api.deployment.yaml
do
echo "--- $f"
ast-grep outline "$f" --view expanded || true
done
printf '\n== search for label helpers ==\n'
rg -n 'define "plane\.(resourceLabels|podLabels|standardLabels)"|app\.name|commonLabels|podLabels|labels:' charts/plane-enterprise/templates charts/plane-enterprise/values.yaml
printf '\n== helpers snippet ==\n'
sed -n '1,260p' charts/plane-enterprise/templates/_helpers.tpl
printf '\n== admin deployment snippet ==\n'
sed -n '1,140p' charts/plane-enterprise/templates/workloads/admin.deployment.yaml
printf '\n== api deployment snippet ==\n'
sed -n '1,160p' charts/plane-enterprise/templates/workloads/api.deployment.yamlRepository: makeplane/helm-charts
Length of output: 47884
🌐 Web query:
Sprig merge precedence left-to-right Helm chart merge function key conflicts user map wins standard labels
💡 Result:
In Helm and Sprig template functions, the precedence of key conflicts depends on which merge function you use [1][2][3]. The standard merge function (merge) gives precedence to the leftmost dictionary (the destination dictionary) [2][4][3]. When a key exists in both the destination and the source, the value in the destination is preserved [4][3]. This makes it useful for applying default values while allowing user-provided maps to take priority (by placing the user map first) [3]. In contrast, the mergeOverwrite function gives precedence from right to left [1][2][3]. When a key exists in multiple dictionaries, the rightmost dictionary wins, effectively overwriting values in the destination [2][4][3]. Summary of Precedence: - merge: Left-to-right (First/Destination dictionary wins) [2][4][3] - mergeOverwrite: Right-to-left (Last/Source dictionary wins) [1][2][3] Important Considerations: - Mutability: Both functions perform deep merges but are not deep copies [2][4][5]. They mutate the leftmost (destination) dictionary [5]. If you need to avoid modifying the original dictionary, use deepCopy to clone it before passing it to the merge function [2][4][5]. - Falsy Values: Be aware that in some contexts, Sprig's merge behavior regarding falsy values (like empty strings or false booleans) can be counter-intuitive depending on the specific implementation version [6][7]. - Use Case: If you want user-defined labels or configurations to override defaults, you should typically use merge with the user dictionary as the first argument, or use mergeOverwrite with the user dictionary as the last argument [3].
Citations:
- 1: https://helm.sh/docs/chart_template_guide/function_list/
- 2: https://github.com/Masterminds/sprig/blob/bd7cb37b66a3bf86016d09188a371c36e5a63f21/docs/dicts.md
- 3: https://kubernetes.recipes/recipes/helm/helm-sprig-add1-trim-merge-functions/
- 4: https://github.com/helm/helm-www/blob/main/versioned_docs/version-3/chart_template_guide/function_list.md
- 5: Helm
mergemutates the leftmost dict helm/helm#13308 - 6: Merge function does not overwrite if new value is falsy Masterminds/sprig#134
- 7: https://blog.nuvotex.de/helms-hidden-quirks-merging-boolean-values/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== values docs around labels ==\n'
sed -n '1,80p' charts/plane-enterprise/values.yaml
printf '\n== service-specific label sections ==\n'
for f in \
charts/plane-enterprise/values.yaml
do
echo "--- $f"
rg -n -C 3 'commonLabels:|podLabels:|labels: \{\}|app\.name|reserved|selector|wins over|override' "$f"
done
printf '\n== any mention of app.name in values or templates comments ==\n'
rg -n -C 2 'app\.name|reserved keys|omit|exclude' charts/plane-enterprise/templates charts/plane-enterprise/values.yamlRepository: makeplane/helm-charts
Length of output: 50377
charts/plane-enterprise/templates/workloads/admin.deployment.yaml: protect app.name in merged labels.
commonLabels/podLabels are merged straight into the metadata labels, and app.name isn’t excluded. A user-supplied app.name can produce a duplicate key and make the pod template labels diverge from selector.matchLabels. Omit app.name from the merged maps or render the fixed label after the include. Same pattern exists in the other workload templates.
🧰 Tools
🪛 YAMLlint (1.37.1)
[error] 6-6: syntax error: expected , but found ''
(syntax)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/plane-enterprise/templates/workloads/admin.deployment.yaml` around
lines 6 - 13, The merged labels in the admin workload template allow a
user-supplied app.name to conflict with the chart’s fixed app.name value, which
can break selector.matchLabels consistency. Update the workload label rendering
in the admin deployment template and the other similar workload templates so
app.name is either excluded from the included resource labels or always emitted
after the include as the authoritative fixed label, using the existing
plane.resourceLabels helper and the pod template/selector label blocks.
1 participant
What
Makes labels/annotations user-configurable across the entire
plane-enterprisechart, layered on top of the standard recommended labels from #256. Adds tovalues.yaml:Precedence on key conflict: job override > per-service > common > standard labels. All 46 chart objects (Deployments, StatefulSets, Jobs, Services, ConfigMaps, Secrets, certs, Ingress/IngressRoute, ServiceAccount, Middleware) and all 24 pod templates are wired through shared helpers in
templates/_helpers.tpl.Why
podSelector, service-mesh injection (sidecar.istio.io/inject), Prometheus scrape annotations, Vault agent injection. The chart previously exposed only object-levelservices.<svc>.labels, so none of these integrations could target Plane pods.commonLabels/commonAnnotations(Grafana/GitLab convention) enable cost-allocation, ownership, and GitOps grouping without repeating maps per service.sidecar.istio.io/inject: "true"on api pods would be inherited by the migration Job, whose sidecar never exits, so the Job never completes. Jobs inherit the parent service's maps and can selectively override (inject: "false"), and take Job-only object annotations likeargocd.argoproj.io/hook: PreSync.Scope / behavior
{}; with defaults the rendered output is identical to master except the three bug-fixes below. Verified by rendering both.helm upgradesafe: nothing is ever injected intospec.selector/matchLabels— selectors stay on the immutableapp.namelabel. Verified selectors are byte-identical between unlabeled and fully-labeled renders (24 objects).IngressmergescommonAnnotationswith the existingingress.ingress_annotations(ingress-specific wins). TraefikIngressRoute/Middlewareget the common maps too.plane.commonLabels([INFRA-424] - feat(plane-enterprise): add standard Kubernetes recommended labels #256) →plane.standardLabels, freeing the "common" name for the user-facing values key (GitLab's naming); newplane.resourceLabels/plane.resourceAnnotations/plane.podLabels/plane.podAnnotationscompose the tiers. Rename verified byte-identical on rendered output.Bug fixes (found while testing)
services.pi.labelswhile its pods usedservices.pi.podLabels— now consistent with the api-migrator and bucket Jobs.MiddlewarerenderedmaxRequestBodyBytes: 2.097152e+07(Helm float64 scientific notation), which the CRD's integer field can reject — now| int64, renders20971520.Testing
helm lintclean; chart version bumped 2.7.0 → 2.8.0.helm template+ YAML parsing), covering:labels:/annotations:blocks;services.api.labels.teamwins on the api object;services.postgres.podLabels.teamwins on postgres pods);sidecar.istio.io/inject: "true"while migrator pods render"false"yet still inherit api's prometheus annotations; ArgoCD hook lands on the Job object only;ingress_annotationswins) and traefik IngressRoute/Middleware coverage;commonLabels."app.kubernetes.io/version"wins, documented);Upgrade notes
None required. Existing releases upgrade in place: selectors are untouched; new labels/annotations only appear if the operator sets the new values.
Related
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores