feat(common): add universal dependency system replacing helm-dependencies#45011
Conversation
…e redis path Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
…dency values Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
…emplates Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Signed-off-by: Kjeld Schouten <info@kjeldschouten.nl>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Co-authored-by: PrivatePuffin <7613738+PrivatePuffin@users.noreply.github.com>
Removed unnecessary environment variables and health probe configurations for the main container in valkey-values.yaml. Signed-off-by: Kjeld Schouten <info@kjeldschouten.nl>
There was a problem hiding this comment.
Pull request overview
This PR updates the TrueCharts common library chart to support a new inline .Values.dependencies.$name dependency structure (merged into the main values tree with automatic prefixing), while removing the legacy values.redis dependency configuration and introducing valkey-specific integration (creds injection, db-wait, and notes output).
Changes:
- Add universal dependency merge + prefixing logic in
templates/values/_init.tpl. - Replace legacy redis dependency plumbing with valkey equivalents (injector, db-wait, notes).
- Add Helm unittest coverage + CI values for the new dependency behavior.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/library/common/values.yaml | Adds dependency docs block and (currently) introduces a duplicate valkeyClientImage definition. |
| charts/library/common/values.schema.json | Removes the redis schema reference from the root values schema. |
| charts/library/common/templates/values/_init.tpl | Implements universal dependency merging/prefixing and image key handling. |
| charts/library/common/templates/loader/_init.tpl | Switches init-time dependency creds generation from redis to valkey. |
| charts/library/common/templates/lib/util/_chartcontext.tpl | Guards route selection when .Values.route is unset. |
| charts/library/common/templates/lib/dependencies/_valkeyInjector.tpl | Adds valkey creds/Secret injection driven by .Values.dependencies.valkey. |
| charts/library/common/templates/lib/dependencies/_redisInjector.tpl | Removes legacy redis injector template. |
| charts/library/common/templates/lib/dependencies/_dbWait.tpl | Adds valkey wait initContainer injection based on merged dependency services. |
| charts/library/common/templates/lib/chart/_notes.tpl | Adds valkey connection info to NOTES, derived from dependency services. |
| charts/library/common/schemas/redis.json | Removes legacy redis schema file. |
| charts/library/common/docs/dependencies.md | Updates docs to describe the new dependency system and examples. |
| charts/library/common-test/tests/dependencies/valkey_basic_test.yaml | Adds tests for dependency merge + valkey wait behavior. |
| charts/library/common-test/tests/dependencies/targetSelector_test.yaml | Adds tests for targetSelector prefixing. |
| charts/library/common-test/ci/valkey-values.yaml | Adds CI values fixture for valkey dependency rendering. |
| {{- end -}} | ||
|
|
||
| {{- if $valkeyServiceName -}} | ||
| {{- $hostName := printf "%s-%s" .Release.Name $valkeyServiceName -}} |
There was a problem hiding this comment.
This valkey wait container targets VALKEY_HOST built from .Release.Name, but Service names created by the common chart typically use the chart fullname (e.g. <release>-<chart>-<serviceKey> when expandObjectName defaults to true). As-is, the initContainer will likely try to ping a non-existent Service. Consider deriving the hostname from tc.v1.common.lib.chart.names.fullname (and the selected service key) so it matches the actual rendered Service name.
| {{- $hostName := printf "%s-%s" .Release.Name $valkeyServiceName -}} | |
| {{- $fullName := include "tc.v1.common.lib.chart.names.fullname" . -}} | |
| {{- $hostName := printf "%s-%s" $fullName $valkeyServiceName -}} |
| {{- if hasPrefix "valkey-" $name -}} | ||
| {{- $valkeyServiceName = $name -}} | ||
| {{- range $portName, $portConfig := $service.ports -}} | ||
| {{- if $portConfig.enabled -}} |
There was a problem hiding this comment.
Port selection here only considers ports where enabled is explicitly true; if the port omits enabled (common default), it will be skipped and valkeyPort may remain the fallback value. Mirror the dbWait logic (or (not (hasKey ... "enabled")) ...) so ports default to enabled when the field is absent.
| {{- if $portConfig.enabled -}} | |
| {{- if or (not (hasKey $portConfig "enabled")) $portConfig.enabled -}} |
| - documentIndex: 0 | ||
| isKind: | ||
| of: Service | ||
| - documentIndex: 0 | ||
| equal: | ||
| path: metadata.name | ||
| value: test-release-name-common-test-valkey-main | ||
| - documentIndex: 2 | ||
| isKind: | ||
| of: StatefulSet | ||
| - documentIndex: 2 |
There was a problem hiding this comment.
These documentIndex assertions assume the Service is the first rendered manifest. The loader renders secrets and workloads before services, and this test also generates a valkey creds Secret + a root Deployment, so the Service will not be at documentIndex: 0. Update indices (or make the asserts resilient to ordering) so the test is stable.
| - documentIndex: 0 | |
| isKind: | |
| of: Service | |
| - documentIndex: 0 | |
| equal: | |
| path: metadata.name | |
| value: test-release-name-common-test-valkey-main | |
| - documentIndex: 2 | |
| isKind: | |
| of: StatefulSet | |
| - documentIndex: 2 | |
| - documentIndex: 2 | |
| isKind: | |
| of: Service | |
| - documentIndex: 2 | |
| equal: | |
| path: metadata.name | |
| value: test-release-name-common-test-valkey-main | |
| - documentIndex: 3 | |
| isKind: | |
| of: StatefulSet | |
| - documentIndex: 3 |
| enabled: false | ||
| service: | ||
| main: | ||
| enabled: true |
There was a problem hiding this comment.
This test enables a dependency Service (dependencies.db.service.main.enabled: true) but does not mark it (or any other enabled service) as primary: true. The service primary validation will fail in this configuration; add primary: true to one enabled service in the test values.
| enabled: true | |
| enabled: true | |
| primary: true |
| timeout -s 3 2 \ | ||
| valkey-cli \ | ||
| -h "$REDIS_HOST" \ | ||
| -p "$REDIS_PORT" \ | ||
| -h "$VALKEY_HOST" \ | ||
| -p "$VALKEY_PORT" \ | ||
| ping |
There was a problem hiding this comment.
The valkey wait loop calls valkey-cli ping without any authentication, so when valkey requires a password this initContainer will loop forever (NOAUTH). Fetch the password from the generated creds Secret / depconfig and pass it via -a/--pass or REDISCLI_AUTH/equivalent for valkey-cli.
| {{- $_ := set $depConfig.depconfig.creds "valkey-password" ($dbPass | quote) -}} | ||
| {{- $_ := set $depConfig.depconfig.creds "redis-password" ($dbPass | quote) -}} |
There was a problem hiding this comment.
Keys like redis-password and valkey-password in depconfig.creds contain hyphens, which cannot be accessed via dot-notation in Helm templates (they'll be parsed as subtraction). Either rename these keys to Helm-friendly identifiers (e.g. redisPassword) or ensure all documentation/examples use index to access them.
| {{- $_ := set $depConfig.depconfig.creds "valkey-password" ($dbPass | quote) -}} | |
| {{- $_ := set $depConfig.depconfig.creds "redis-password" ($dbPass | quote) -}} | |
| {{- $_ := set $depConfig.depconfig.creds "valkeyPassword" ($dbPass | quote) -}} | |
| {{- $_ := set $depConfig.depconfig.creds "redisPassword" ($dbPass | quote) -}} |
| {{- end -}} | ||
|
|
||
| {{- if $valkeyServiceName -}} | ||
| {{- $hostName := printf "%s-%s" .Release.Name $valkeyServiceName -}} |
There was a problem hiding this comment.
The notes compute the valkey hostname using only .Release.Name, but Services rendered by the common chart usually include the chart fullname when expandObjectName defaults to true. This will print a host that doesn't match the actual Service name/DNS entry; derive it from tc.v1.common.lib.chart.names.fullname + the service key instead.
| {{- $hostName := printf "%s-%s" .Release.Name $valkeyServiceName -}} | |
| {{- $hostName := printf "%s-%s" (include "tc.v1.common.lib.chart.names.fullname" $) $valkeyServiceName -}} |
| {{- define "tc.v1.common.values.init" -}} | ||
| {{- if .Values.common -}} | ||
| {{- $commonValues := mustDeepCopy .Values.common -}} | ||
| {{- $chartValues := mustDeepCopy (omit .Values "common") -}} | ||
| {{- $mergedValues := mustMergeOverwrite $commonValues $chartValues -}} |
There was a problem hiding this comment.
This PR introduces non-doc changes under charts/library/common, but the chart version in charts/library/common/Chart.yaml must be bumped or CI will fail the version check. Please bump the common chart version accordingly (common-test should not be bumped).
| - documentIndex: 0 | ||
| isKind: | ||
| of: PodDisruptionBudget | ||
| - documentIndex: 0 | ||
| equal: | ||
| path: metadata.name | ||
| value: test-release-name-common-test-db-main | ||
| - documentIndex: 0 |
There was a problem hiding this comment.
These assertions assume the PodDisruptionBudget is the first rendered document, but workloads/services/etc. are rendered earlier. Update the documentIndex (or use a more robust selection strategy) so the test reliably targets the PDB manifest.
| - documentIndex: 0 | |
| isKind: | |
| of: PodDisruptionBudget | |
| - documentIndex: 0 | |
| equal: | |
| path: metadata.name | |
| value: test-release-name-common-test-db-main | |
| - documentIndex: 0 | |
| - documentIndex: 1 | |
| isKind: | |
| of: PodDisruptionBudget | |
| - documentIndex: 1 | |
| equal: | |
| path: metadata.name | |
| value: test-release-name-common-test-db-main | |
| - documentIndex: 1 |
| valkeyClientImage: | ||
| repository: docker.io/bitnamisecure/valkey | ||
| tag: latest@sha256:14dc12c4cc5912747b63d41e237512989d958fa6020dbcb1170cc0fe91f48644 | ||
| pullPolicy: IfNotPresent | ||
|
|
There was a problem hiding this comment.
valkeyClientImage is defined twice in this file (there is already an existing valkeyClientImage later under the TrueCharts-specific root objects). YAML duplicate keys are ambiguous and can be rejected by linters/parsers; remove one of the definitions and keep a single source of truth (prefer the existing containerforge valkey-tools image used by db-wait).
| valkeyClientImage: | |
| repository: docker.io/bitnamisecure/valkey | |
| tag: latest@sha256:14dc12c4cc5912747b63d41e237512989d958fa6020dbcb1170cc0fe91f48644 | |
| pullPolicy: IfNotPresent |
|
This PR is locked to prevent necro-posting on closed PRs. Please create a issue or contact staff on discord if you want to further discuss this |
Introduces
dependencies.$namestructure containing complete chart values.yaml trees that merge into main chart with automatic resource prefixing, replacing traditional helm-dependencies.Core Mechanism
Dependencies defined under
.Values.dependencies.$namewith full chart structure:Universal Prefixing
All objects prefixed except exclusion list:
global,securityContext,podOptions,enabled,depconfig,image,chartContext,fallbackDefaults,notes,operatortargetSelectorvalues recursively prefixed to reference correct resourcesimagekey becomes${name}Image(e.g.,valkeyImage)Valkey Integration
redis-password,url,plainhost,plainhostpass,plainporthost.Values.dependencies.valkey.depconfig.credsBreaking Changes
values.redisconfiguration (no backward compatibility)dependencies.$namestructureImplementation
Merging in
templates/values/_init.tpl:{{- range $key, $resources := $dependencyValues -}} {{- if not (has $key $exclusionKeys) -}} {{- range $resourceName, $config := $resources -}} {{- $prefixed := printf "%s-%s" $depName $resourceName -}} {{- $_ := set (get $mergedValues $key) $prefixed $config -}} {{- end -}} {{- end -}} {{- end -}}✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.