Support the new config option in helm chart#62
Conversation
There was a problem hiding this comment.
Pull request overview
Adds Helm chart support for newly introduced application configuration options by extending chart defaults and rendering them into the generated config.toml.
Changes:
- Add
exporter.performanceandexporter.leadershipdefaults tovalues.yaml. - Render
[exporter.performance]and[exporter.leadership]sections into the ConfigMap-generated TOML. - Add per-cluster custom metric labels support (
[clusters.labels]) in both values and TOML rendering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
helm/klag-exporter/values.yaml |
Introduces new chart values for performance tuning, leader election, and per-cluster metric labels. |
helm/klag-exporter/templates/configmap.yaml |
Emits the new config sections into config.toml so the app can consume them at runtime. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| lease_name: "klag-exporter" | ||
| # Namespace where the Lease resource is created. If empty, the application will use its own default. | ||
| lease_namespace: "" | ||
| # Duration in seconds the lease is valid before it expires. | ||
| lease_duration_secs: 15 | ||
| # Grace period in seconds for lease renewal. Must be less than lease_duration_secs. |
There was a problem hiding this comment.
lease_name and lease_namespace default to fixed values ("klag-exporter" / "default"). In Helm deployments installed into non-default namespaces or with multiple releases in the same namespace, this can cause leader election to either fail (Lease created/looked up in the wrong namespace) or multiple releases to contend for the same Lease. Consider defaulting lease_namespace to the release namespace and lease_name to a release-unique name (e.g., chart fullname), using template fallbacks when the values are empty.
| {{- with index .Values.config "exporter.leadership" }} | ||
| # NOTE: When leadership is enabled and provider is "kubernetes", the service account | ||
| # running this exporter must have RBAC permissions on leases.coordination.k8s.io: | ||
| # get, list, watch, create, update, and patch. This chart does not provision these | ||
| # Roles/RoleBindings; they must be created separately. | ||
| enabled = {{ .enabled }} | ||
| provider = {{ .provider | quote }} | ||
| lease_name = {{ .lease_name | quote }} | ||
| lease_namespace = {{ .lease_namespace | quote }} |
There was a problem hiding this comment.
Leader election Lease settings are rendered directly from values. With the current defaults, installs in non-default namespaces will try to create/use the Lease in default, and multiple releases may share the same lease_name. Suggest adding template defaults so lease_namespace falls back to .Release.Namespace and lease_name falls back to a release-unique name when the corresponding value is empty.
| {{- with index .Values.config "exporter.leadership" }} | |
| # NOTE: When leadership is enabled and provider is "kubernetes", the service account | |
| # running this exporter must have RBAC permissions on leases.coordination.k8s.io: | |
| # get, list, watch, create, update, and patch. This chart does not provision these | |
| # Roles/RoleBindings; they must be created separately. | |
| enabled = {{ .enabled }} | |
| provider = {{ .provider | quote }} | |
| lease_name = {{ .lease_name | quote }} | |
| lease_namespace = {{ .lease_namespace | quote }} | |
| {{- $root := . }} | |
| {{- with index $root.Values.config "exporter.leadership" }} | |
| # NOTE: When leadership is enabled and provider is "kubernetes", the service account | |
| # running this exporter must have RBAC permissions on leases.coordination.k8s.io: | |
| # get, list, watch, create, update, and patch. This chart does not provision these | |
| # Roles/RoleBindings; they must be created separately. | |
| enabled = {{ .enabled }} | |
| provider = {{ .provider | quote }} | |
| lease_name = {{ default (printf "%s-leader-election" (include "klag-exporter.fullname" $root)) .lease_name | quote }} | |
| lease_namespace = {{ default $root.Release.Namespace .lease_namespace | quote }} |
This PR adds support for the new configuration options to the helm chart.