fix(runtime): pass DAG level variables to executor config evaluation properly#1448
fix(runtime): pass DAG level variables to executor config evaluation properly#1448
Conversation
WalkthroughThe PR refactors environment variable handling in the runtime package by replacing the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/integration/base_dag_test.go (1)
26-35: Optionally assert concrete values and harden handler command quotingRight now you only check that
DAG_NAME,DAG_RUN_ID, andDAG_RUN_LOG_FILEare present and non‑empty. For a stronger regression guard, consider parsing the file and asserting they matchdag.Name,dagRunID, andlogFile. Also, sinceoutputFileis interpolated into a shell script, wrapping it in quotes (e.g.">> %q"viafmt.Sprintf) would make the test resilient to paths with spaces.Also applies to: 84-95
internal/runtime/eval_test.go (1)
7-26: Shared DAGContext/BaseEnv test setup is appropriate; update comment for clarity
setupTestContextgives the EvalObject-focused tests a properly initializedDAGContext(includingBaseEnv), which fits the newAllEnvs(AllEnvsMap)behavior, and its reuse across tests keeps setups consistent. The file-level note about “these tests useexecution.SetupDAGContext” is now partly outdated though; consider updating it to mentionsetupTestContext/WithDAGContextso future readers don’t misinterpret how the context is constructed.Also applies to: 171-207, 214-221, 323-329, 548-556
internal/runtime/env.go (1)
56-67: AllEnvs(/AllEnvsMap) aggregation looks correct; consider documenting precedenceThe new
Env.AllEnvs+AllEnvsMapcorrectly layer DAGContext envs, stepEnvs, andVariableswith “later overrides earlier”, matchingTestEnv_AllEnvsMap(e.g. variables winning overEnvs). To avoid confusion alongside the detailed precedence comment onEvalStringand theUserEnvsMapdoc, it would be helpful to add a brief precedence note here (and, if necessary, reconcile theUserEnvsMapcomment with the actual overwrite behavior).Also applies to: 379-396
internal/runtime/env_test.go (1)
13-19: AllEnvsMap tests match implementation; optional extra coverage for BaseEnv/DAG env
TestEnv_AllEnvsMapdoes a good job validating the merged map (variables + step envs + step name) and the intended precedence where variables overrideEnvs. If you want even stronger guarantees, you could add one case that seedsBaseEnv(e.g. viaconfig.NewBaseEnv([]string{"BASE_KEY=base"})) or DAGEnvand asserts those keys also appear inAllEnvsMap, but the current coverage is already reasonable.Also applies to: 21-129
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
internal/integration/base_dag_test.go(1 hunks)internal/runtime/env.go(2 hunks)internal/runtime/env_test.go(3 hunks)internal/runtime/eval.go(1 hunks)internal/runtime/eval_test.go(6 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Backend entrypoint incmd/orchestrates the scheduler and CLI; runtime, persistence, and service layers sit underinternal/*(for exampleinternal/runtime,internal/persistence)
Keep Go filesgofmt/goimportsclean; use tabs, PascalCase for exported symbols (SchedulerClient), lowerCamelCase for locals, andErr...names for package-level errors
Repository linting relies ongolangci-lint; prefer idiomatic Go patterns, minimal global state, and structured logging helpers ininternal/common
Files:
internal/runtime/eval.gointernal/integration/base_dag_test.gointernal/runtime/eval_test.gointernal/runtime/env.gointernal/runtime/env_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Co-locate Go tests as*_test.go; favour table-driven cases and cover failure paths
Usestretchr/testify/requireand shared fixtures frominternal/testinstead of duplicating mocks
Files:
internal/integration/base_dag_test.gointernal/runtime/eval_test.gointernal/runtime/env_test.go
🧠 Learnings (2)
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/integration/base_dag_test.gointernal/runtime/eval_test.gointernal/runtime/env_test.go
📚 Learning: 2025-12-04T10:34:17.051Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.051Z
Learning: Applies to **/*_test.go : Use `stretchr/testify/require` and shared fixtures from `internal/test` instead of duplicating mocks
Applied to files:
internal/runtime/env_test.go
🧬 Code graph analysis (5)
internal/runtime/eval.go (1)
internal/runtime/env.go (1)
AllEnvsMap(386-396)
internal/integration/base_dag_test.go (1)
internal/core/spec/loader.go (1)
WithBaseConfig(42-46)
internal/runtime/eval_test.go (2)
internal/common/config/env.go (2)
NewBaseEnv(15-17)BaseEnv(9-11)internal/core/execution/context.go (2)
DAGContext(16-26)WithDAGContext(173-175)
internal/runtime/env.go (2)
internal/core/execution/context.go (1)
DAGContext(16-26)internal/cmd/context.go (1)
Context(38-53)
internal/runtime/env_test.go (5)
internal/runtime/env.go (3)
Env(24-54)WithEnv(366-368)AllEnvsMap(386-396)internal/core/dag.go (1)
DAG(33-135)internal/common/config/env.go (2)
NewBaseEnv(15-17)BaseEnv(9-11)internal/core/execution/context.go (2)
DAGContext(16-26)WithDAGContext(173-175)internal/core/step.go (1)
Step(13-77)
🪛 Gitleaks (8.30.0)
internal/runtime/env_test.go
[high] 49-49: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test on ubuntu-latest
🔇 Additional comments (2)
internal/integration/base_dag_test.go (1)
18-82: Integration coverage for handler env vars looks solidThe test setup and assertions correctly exercise a failing DAG with a base-config failure handler and confirm the handler runs and sees the special env vars; structure and use of
test.Setup/agent wiring look good.internal/runtime/eval.go (1)
24-28: EvalObject now correctly uses unified env aggregationSwitching
EvalObjectto passAllEnvsMap(ctx)aligns object evaluation with the new combined env model (DAG context + step envs + variables) and matches the dedicated tests you added.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1448 +/- ##
==========================================
- Coverage 59.75% 59.74% -0.01%
==========================================
Files 187 187
Lines 21093 21095 +2
==========================================
Hits 12604 12604
- Misses 7175 7176 +1
- Partials 1314 1315 +1
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
Bug Fixes
DAG_NAME,DAG_RUN_ID,DAG_RUN_LOG_FILE) are now properly accessible in failure handlers during DAG execution.Tests
✏️ Tip: You can customize this high-level summary in your review settings.