Skip to content

Fix celery docker env var parsing#34007

Open
mattfaltyn wants to merge 1 commit into
dagster-io:masterfrom
mattfaltyn:agent/fix-celery-docker-env-equals
Open

Fix celery docker env var parsing#34007
mattfaltyn wants to merge 1 commit into
dagster-io:masterfrom
mattfaltyn:agent/fix-celery-docker-env-equals

Conversation

@mattfaltyn

@mattfaltyn mattfaltyn commented Jul 15, 2026

Copy link
Copy Markdown

Summary & Motivation

Fixes #34006.

dagster-celery-docker manually split list-form container_kwargs.environment entries on every =, so valid values like TOKEN=a=b crashed with ValueError: too many values to unpack. This reuses Dagster's existing parse_env_var() helper so only the first = separates the key from the value.

Test Plan

  • UV_CACHE_DIR=/tmp/uv-cache uv run --frozen --project python_modules/libraries/dagster-celery-docker --group test pytest python_modules/libraries/dagster-celery-docker/dagster_celery_docker_tests/test_executor.py -q — passed, 1 passed in 0.06s
  • UV_CACHE_DIR=/tmp/uv-cache UV_TOOL_DIR=/tmp/uv-tools uvx ruff==0.15.15 check python_modules/libraries/dagster-celery-docker/dagster_celery_docker/executor.py python_modules/libraries/dagster-celery-docker/dagster_celery_docker_tests/test_executor.py — passed
  • UV_CACHE_DIR=/tmp/uv-cache UV_TOOL_DIR=/tmp/uv-tools uvx ruff==0.15.15 format --check python_modules/libraries/dagster-celery-docker/dagster_celery_docker/executor.py python_modules/libraries/dagster-celery-docker/dagster_celery_docker_tests/test_executor.py — passed, 2 files already formatted
  • UV_CACHE_DIR=/tmp/uv-cache uv run --frozen --project python_modules/libraries/dagster-celery-docker --group test pytest python_modules/libraries/dagster-celery-docker/dagster_celery_docker_tests -q — attempted; local run reached 3 passed, 2 errors, with both errors in Docker Compose fixture setup because the docker executable is unavailable locally (FileNotFoundError: [Errno 2] No such file or directory: 'docker')

Changelog

Fixed dagster-celery-docker handling of list-form Docker environment values that contain additional equals signs.

@mattfaltyn
mattfaltyn force-pushed the agent/fix-celery-docker-env-equals branch from 073e5bc to 05614de Compare July 15, 2026 08:01
@mattfaltyn
mattfaltyn marked this pull request as ready for review July 15, 2026 08:08
@greptile-apps

greptile-apps Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a ValueError: too many values to unpack crash in dagster-celery-docker when Docker container environment entries in list form (e.g., TOKEN=a=b) contain extra = characters. The fix replaces a bare v.split("=") with the existing parse_env_var() helper that uses split("=", maxsplit=1).

  • executor.py: Imports and uses parse_env_var() instead of v.split("=") in the list-form environment variable processing path.
  • test_executor.py: Adds a focused unit test that stubs out all Docker and Dagster I/O and asserts that TOKEN=a=b is correctly parsed to key TOKEN and value a=b.

Confidence Score: 5/5

The change is minimal, targeted, and replaces a proven-broken code path with a well-tested utility already used elsewhere in the codebase.

The one-line change in executor.py is straightforwardly correct: parse_env_var uses maxsplit=1 to isolate only the first =, fixing the crash without touching any other logic. The new test exercises the exact failure scenario end-to-end, monkeypatching cleanly without leaving real side-effects. No other code paths are affected.

No files require special attention; both changed files are clean.

Important Files Changed

Filename Overview
python_modules/libraries/dagster-celery-docker/dagster_celery_docker/executor.py Single-line fix replacing v.split('=') with parse_env_var(v) to correctly handle env values containing =; import added correctly.
python_modules/libraries/dagster-celery-docker/dagster_celery_docker_tests/test_executor.py New regression test that monkeypatches Docker and Dagster dependencies to verify the env var parsing fix end-to-end without requiring a Docker daemon.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["container_kwargs.environment provided"] --> B{isinstance dict?}
    B -- Yes --> C["docker_env.update(e_vars)"]
    B -- No --> D["for v in e_vars (list)"]
    D --> E["parse_env_var(v)"]
    E --> F{Contains '='?}
    F -- Yes --> G["split on first '=' only\ne.g. TOKEN=a=b → ('TOKEN','a=b')"]
    F -- No --> H["os.getenv(v)\n(inherit from worker env)"]
    G --> I["docker_env[key] = val"]
    H --> I
    I --> J["del container_kwargs['environment']"]
    J --> K["client.containers.run(..., environment=docker_env)"]

    style E fill:#90EE90
    style G fill:#90EE90
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["container_kwargs.environment provided"] --> B{isinstance dict?}
    B -- Yes --> C["docker_env.update(e_vars)"]
    B -- No --> D["for v in e_vars (list)"]
    D --> E["parse_env_var(v)"]
    E --> F{Contains '='?}
    F -- Yes --> G["split on first '=' only\ne.g. TOKEN=a=b → ('TOKEN','a=b')"]
    F -- No --> H["os.getenv(v)\n(inherit from worker env)"]
    G --> I["docker_env[key] = val"]
    H --> I
    I --> J["del container_kwargs['environment']"]
    J --> K["client.containers.run(..., environment=docker_env)"]

    style E fill:#90EE90
    style G fill:#90EE90
Loading

Reviews (1): Last reviewed commit: "Fix celery docker env var parsing" | Re-trigger Greptile

@mattfaltyn

Copy link
Copy Markdown
Author

Buildkite unit-tests is currently blocked before jobs start. The Buildkite build reports creator_verified: false, started_at: null, jobs_started_count: 0, and jobs_failed_count: 0, so there are no unit-test failure logs available yet.

Local scoped validation passed as listed in the Test Plan: focused regression test, Ruff check, and Ruff format check. The broader dagster-celery-docker package test command was attempted locally, but the two Docker Compose integration tests could not run because the local docker executable is unavailable.

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.

[dagster-celery-docker] environment values containing '=' crash in container_kwargs

1 participant