Skip to content

fix(helm): wire envFrom secrets and configmaps into migrations job (closes #30311) - #30325

Open
tsushanth wants to merge 1 commit into
BerriAI:litellm_internal_stagingfrom
tsushanth:fix-helm-migration-job-envfrom
Open

fix(helm): wire envFrom secrets and configmaps into migrations job (closes #30311)#30325
tsushanth wants to merge 1 commit into
BerriAI:litellm_internal_stagingfrom
tsushanth:fix-helm-migration-job-envfrom

Conversation

@tsushanth

@tsushanth tsushanth commented Jun 13, 2026

Copy link
Copy Markdown

Relevant issues

Closes #30311

Linear ticket

n/a (community contribution)

Pre-Submission checklist

  • I have added meaningful tests
  • My PR's scope is as isolated as possible; it only solves 1 specific problem
  • Greptile review complete: 5/5 Confidence Score (review link), Greptile flagged no issues and notes the change is a minimal copy of the existing pattern with no behavioural side-effects

Screenshots / Proof of Fix

Render the migrations Job with two env secrets and one configmap set via the standard chart values

helm template test deploy/charts/litellm-helm \
  --set environmentSecrets='{litellm-env-secret,extra-secret}' \
  --set environmentConfigMaps='{litellm-env-configmap}' \
  --set 'migrationJob.enabled=true' \
  --set 'db.useExisting=true' \
  --set 'db.secret.name=litellm-db-secret' \
  --set 'db.secret.usernameKey=username' \
  --set 'db.secret.passwordKey=password' \
  --set 'db.secret.endpointKey=endpoint' \
  --set 'db.database=litellm' \
  --set 'db.url=postgresql://user:pass@host:5432/litellm' \
  --show-only templates/migrations-job.yaml

Output (relevant slice)

            - name: DISABLE_SCHEMA_UPDATE
              value: "false"
          envFrom:
            - secretRef:
                name: litellm-env-secret
            - secretRef:
                name: extra-secret
            - configMapRef:
                name: litellm-env-configmap
      restartPolicy: OnFailure

Empty case still parses cleanly (matches the existing deployment.yaml behavior)

helm template test deploy/charts/litellm-helm \
  --set 'migrationJob.enabled=true' \
  --show-only templates/migrations-job.yaml | grep -B1 -A1 envFrom

Output

              value: "false"
          envFrom:
      restartPolicy: OnFailure

Unit tests via helm-unittest

helm unittest -f 'tests/migrations-job_tests.yaml' deploy/charts/litellm-helm

Output

 PASS  test migrations job  tests/migrations-job_tests.yaml

Charts:      1 passed, 1 total
Test Suites: 1 passed, 1 total
Tests:       17 passed, 17 total

Three new regression cases cover the three failure modes of the bug (environmentSecrets only, environmentConfigMaps only, both together)

Type

Bug Fix

Changes

Adds the same envFrom: block already present in templates/deployment.yaml to templates/migrations-job.yaml, ranging over .Values.environmentSecrets and .Values.environmentConfigMaps. The migrations Job container now sees the same Secret and ConfigMap references the main Deployment does, so a DATABASE_* var that lives in a referenced Secret (the common production pattern) reaches the Prisma migration step. Mirroring keeps the two surfaces from drifting on future value-key additions

Test file adds three new cases following the existing style: secrets-only, configmaps-only, and both, each asserting the rendered envFrom list contains the expected refs

@CLAassistant

CLAassistant commented Jun 13, 2026

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@greptile-apps

greptile-apps Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

Wires environmentSecrets and environmentConfigMaps into the Prisma migrations Job container, which previously only received those env sources in the main Deployment. The fix is a direct copy of the identical envFrom block already present in deployment.yaml.

  • migrations-job.yaml: Adds the envFrom block (iterating environmentSecrets as secretRef entries and environmentConfigMaps as configMapRef entries) immediately after the env section, matching the deployment template line-for-line.
  • migrations-job_tests.yaml: Adds three regression cases (secrets-only, configmaps-only, both together) using contains assertions on spec.template.spec.containers[0].envFrom.

Confidence Score: 5/5

Safe to merge — the change is a minimal, focused copy of an existing pattern with no behavioural side-effects.

The template change is an exact copy of the envFrom block already proven in deployment.yaml. The explicit env entries (including DISABLE_SCHEMA_UPDATE=false) retain precedence over anything coming from the referenced Secrets/ConfigMaps. Three targeted regression tests cover all three failure modes described in the bug report, and the empty-values case continues to behave identically to deployment.yaml.

No files require special attention.

Important Files Changed

Filename Overview
deploy/charts/litellm-helm/templates/migrations-job.yaml Adds envFrom block (environmentSecrets + environmentConfigMaps) mirroring deployment.yaml exactly; change is minimal and correct.
deploy/charts/litellm-helm/tests/migrations-job_tests.yaml Three new helm-unittest cases cover secrets-only, configmaps-only, and both together; assertions use contains correctly for list membership.

Reviews (1): Last reviewed commit: "fix(helm): wire envFrom secrets and conf..." | Re-trigger Greptile

@codecov

codecov Bot commented Jun 13, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@tsushanth

Copy link
Copy Markdown
Author

Greptile gave this 5/5 (full review above). Tagging @yuneng-berri @yassin-berriai — based on recent merged helm PRs (#28710, #28712) you'd be the right folks to take a look when there's a window. Happy to address any review feedback quickly. Thanks

@tsushanth

Copy link
Copy Markdown
Author

Heads-up on the red X above. The test-server-root-path (/api/v1) job failed at the "Install UI deps and Chromium" step with npm error code ECONNRESET / network aborted during npx playwright install --with-deps chromium, while pulling chromium binaries from the npm/playwright registry. The earlier steps (Docker build, start container, healthcheck, UI endpoint test with root path) all passed, and the server e2e step itself was skipped because chromium never installed. It's an npm registry transient, not a regression from this PR (no UI or playwright files are touched here). A rerun should clear it

@tsushanth
tsushanth force-pushed the fix-helm-migration-job-envfrom branch from 2581dc3 to 64844d2 Compare August 7, 2026 17:22
@tsushanth

Copy link
Copy Markdown
Author

Rebased cleanly onto current litellm_internal_staging — the chart directory moved from deploy/charts/litellm-helm/ to helm/litellm-helm/ since this was opened, which is why it had drifted so far. helm lint passes clean on the rebased chart.

@tsushanth
tsushanth force-pushed the fix-helm-migration-job-envfrom branch from 64844d2 to 66c930a Compare August 8, 2026 13:05
@tsushanth

Copy link
Copy Markdown
Author

recheck

@tsushanth

Copy link
Copy Markdown
Author

The one red check (proxy-infra / Run tests) is unrelated to this PR — it's TestPriceDataReloadIntegration::test_distributed_reload_check_function, an assertion comparing model-pricing config data, nothing to do with the Helm chart. This PR only touches helm/litellm-helm/templates/migrations-job.yaml and its test. Would appreciate a re-run when you get a chance — I don't have permission to retrigger it myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Migration job in helm chart does not support envFrom secrets or configmaps

3 participants