feat(core): Add step-level container field#1501
Conversation
WalkthroughAdds per-step container support: introduces step-level Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/core/spec/step_test.go (1)
1001-1038: Consider refactoring to table-driven tests for consistency.These three conflict tests follow identical patterns and could be consolidated into a table-driven test case within the existing
TestBuildStepExecutortest suite (lines 927-1055). This would improve consistency with the rest of the file and reduce code duplication.💡 Example refactor
Add these cases to the
testsslice inTestBuildStepExecutor:+ { + name: "ContainerAndExecutorConflict_String", + step: &step{Executor: "docker"}, + ctx: testStepBuildContext(), + wantErr: true, + setupResult: func() *core.Step { + return &core.Step{ + Container: &core.Container{Image: "alpine:latest"}, + ExecutorConfig: core.ExecutorConfig{Config: make(map[string]any)}, + } + }, + }, + { + name: "ContainerAndExecutorConflict_Map", + step: &step{Executor: map[string]any{"type": "http"}}, + ctx: testStepBuildContext(), + wantErr: true, + setupResult: func() *core.Step { + return &core.Step{ + Container: &core.Container{Image: "alpine:latest"}, + ExecutorConfig: core.ExecutorConfig{Config: make(map[string]any)}, + } + }, + },Then update the test loop to handle the
setupResultfunction if present.internal/runtime/builtin/docker/executor.go (1)
294-327: Non-deterministic env var ordering due to map iteration.The function converts the merged environment map back to a slice using
range, but Go map iteration order is intentionally randomized. This results in non-deterministic ordering of the returned env vars, which can complicate testing, debugging, and potentially cause issues if any downstream code unexpectedly depends on env var order.🔎 Proposed fix to ensure deterministic ordering
// Convert back to slice result := make([]string, 0, len(envMap)) +keys := make([]string, 0, len(envMap)) for key, value := range envMap { + keys = append(keys, key) +} +sort.Strings(keys) +for _, key := range keys { + value := envMap[key] result = append(result, key+"="+value) } return resultDon't forget to add
"sort"to the imports at the top of the file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
internal/core/spec/dag.gointernal/core/spec/errors.gointernal/core/spec/step.gointernal/core/spec/step_test.gointernal/core/step.gointernal/integration/container_test.gointernal/runtime/builtin/docker/client.gointernal/runtime/builtin/docker/client_test.gointernal/runtime/builtin/docker/config.gointernal/runtime/builtin/docker/executor.goschemas/dag.schema.json
🧰 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/core/step.gointernal/runtime/builtin/docker/executor.gointernal/core/spec/dag.gointernal/runtime/builtin/docker/client_test.gointernal/runtime/builtin/docker/client.gointernal/runtime/builtin/docker/config.gointernal/core/spec/errors.gointernal/integration/container_test.gointernal/core/spec/step_test.gointernal/core/spec/step.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/runtime/builtin/docker/client_test.gointernal/integration/container_test.gointernal/core/spec/step_test.go
🧠 Learnings (1)
📚 Learning: 2025-12-04T10:34:17.062Z
Learnt from: CR
Repo: dagu-org/dagu PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-04T10:34:17.062Z
Learning: Applies to **/*_test.go : Co-locate Go tests as `*_test.go`; favour table-driven cases and cover failure paths
Applied to files:
internal/runtime/builtin/docker/client_test.go
🧬 Code graph analysis (7)
internal/core/spec/dag.go (1)
internal/core/container.go (3)
ParsePullPolicy(110-137)ContainerStartup(49-49)ContainerWaitFor(58-58)
internal/runtime/builtin/docker/client_test.go (3)
internal/runtime/builtin/docker/config.go (1)
Config(15-51)internal/core/container.go (1)
Container(9-46)internal/runtime/builtin/docker/client.go (1)
ExecOptions(89-92)
internal/runtime/builtin/docker/client.go (2)
internal/common/logger/context.go (2)
Debug(35-37)Error(50-52)internal/common/logger/tag/tag.go (6)
String(15-17)Image(315-317)Error(20-22)Status(89-91)Host(180-182)ID(276-278)
internal/runtime/builtin/docker/config.go (1)
internal/core/container.go (1)
Container(9-46)
internal/integration/container_test.go (3)
internal/runtime/subcmd.go (1)
Run(247-269)internal/test/helper.go (2)
Setup(81-225)DAG(426-429)internal/core/status.go (1)
Succeeded(11-11)
internal/core/spec/step_test.go (2)
internal/core/step.go (2)
Step(13-81)ExecutorConfig(111-119)internal/core/container.go (5)
PullPolicyMissing(92-92)PullPolicyAlways(90-90)StartupEntrypoint(53-53)WaitForRunning(61-61)PullPolicyNever(91-91)
internal/core/spec/step.go (4)
internal/core/spec/errors.go (2)
ErrContainerAndExecutorConflict(27-27)ErrContainerAndScriptConflict(28-28)internal/core/step.go (2)
ExecutorConfig(111-119)Step(13-81)internal/common/logger/tag/tag.go (1)
Type(266-268)internal/core/spec/builder.go (2)
StepBuildContext(25-28)BuildContext(13-22)
⏰ 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 (23)
internal/core/step.go (1)
77-80: LGTM: Well-documented field addition.The new
Containerfield is properly structured as an optional pointer with clear documentation explaining that it enables per-step container configuration. The implementation follows existing patterns in the struct.internal/runtime/builtin/docker/client_test.go (2)
594-687: LGTM: Comprehensive test coverage for shortcut fields.The new test cases thoroughly validate the workingDir and volumes shortcut functionality, including:
- Basic usage of individual shortcuts
- Combined usage of multiple shortcuts
- Correct precedence (explicit container.WorkingDir overrides shortcut)
- Append semantics for volumes (shortcut volumes append to existing binds)
The tests follow the established table-driven pattern and provide good coverage.
1291-1363: LGTM: Thorough testing of environment variable merging.The test function comprehensively validates
mergeEnvVarsbehavior with appropriate edge case coverage:
- Empty/nil inputs
- Override precedence (override values take precedence over base)
- Values containing equals signs (URLs with query params)
- Empty values
The sorting on lines 1352-1358 is essential for deterministic comparison given map iteration order. Good testing approach.
internal/runtime/builtin/docker/config.go (2)
65-67: LGTM: Clean shortcut field declarations.The new fields provide user-friendly shortcuts for common container configuration, clearly commented and properly tagged for mapstructure parsing.
135-144: LGTM: Correct precedence and append semantics.The shortcut application logic correctly implements:
- Explicit
container.WorkingDirtakes precedence over the shortcut (line 137 condition)- Volumes from shortcuts append to existing
Host.Bindsrather than replacing themThis ensures user intent is preserved when both shortcut and explicit configuration are provided.
internal/core/spec/errors.go (1)
27-28: LGTM: Clear validation error definitions.The new error variables provide clear, actionable messages for configuration conflicts:
ErrContainerAndExecutorConflictexplains that container and executor are mutually exclusiveErrContainerAndScriptConflicthelpfully suggests using 'command' instead of 'script'These support proper validation of step-level container usage.
internal/runtime/builtin/docker/client.go (4)
96-138: LGTM: Enhanced observability in InitializeClient.The added logging provides valuable visibility into Docker client initialization without changing functional behavior. Debug level is appropriate for these operations.
389-467: LGTM: Comprehensive logging in Run method.The debug logging additions trace the execution flow through container checks, startup, and wait operations. This will aid in troubleshooting container runtime issues without impacting performance.
507-603: LGTM: Detailed logging for container startup.The logging enhancements provide visibility into image pull decisions, pull operations, container creation, and startup. The debug level is appropriate for these operations.
691-747: LGTM: Logging for attach and wait operations.The added logging traces log attachment, stdcopy operations, and container wait status. This completes the observability improvements for the Docker client operations.
internal/integration/container_test.go (1)
517-743: LGTM: Comprehensive integration tests for step-level containers.The new test suite thoroughly validates step-level container functionality with excellent coverage:
- Basic container usage and configuration options (workingDir, user, pullPolicy)
- Volume mounting and environment variable handling
- Multiple steps with different containers
- Override behavior (step container overrides DAG container)
- Environment variable merging precedence (container.env takes precedence over step.env)
The tests follow established patterns with proper parallelization and cleanup. This provides strong confidence in the new feature.
internal/core/spec/dag.go (2)
747-752: LGTM: Clean delegation to extracted helper.The refactoring of
buildContainerto delegate tobuildContainerFromSpecis clean and maintains the existing behavior while enabling reuse for step-level containers.
754-803: LGTM: Well-structured extraction of container building logic.The new
buildContainerFromSpecfunction provides excellent reusability for both DAG-level and step-level container configuration. Key strengths:
- Clear parameter naming (
cfor container spec)- Proper validation with specific error messages (lines 758-760, 763-765, 768-770)
- Consistent field mapping with appropriate type conversions (lines 788-792)
- Backward compatibility handling for deprecated
WorkDirfield (lines 796-800)The function follows the same pattern as other builders in this file.
schemas/dag.schema.json (3)
597-597: Minor formatting consolidation.The single-line array format is more concise while maintaining the same semantics. No functional change.
1008-1009: LGTM: Important documentation of env merging behavior.The enhanced description clearly documents that step.env is merged with container.env when the container field is used. This is crucial information for users to understand the precedence rules.
1010-1013: LGTM: Well-documented step-level container field.The new schema entry properly:
- References the shared container definition
- Explains when it applies (runs in own container instead of DAG-level)
- Documents the mutual exclusivity with executor field
- Uses the same configuration format as DAG-level container
The schema accurately reflects the code changes.
internal/core/spec/step_test.go (1)
1435-1587: Excellent test coverage for step-level container configuration!The test suite comprehensively validates all aspects of step-level container building:
- Field mapping and defaults (pull policy, startup, waitFor)
- Environment variable handling
- Volume configuration
- Backward compatibility (WorkDir)
- Error conditions (missing image, script conflict)
This thorough coverage will help catch regressions as the container feature evolves.
internal/runtime/builtin/docker/executor.go (3)
153-159: Correct isolation between DAG-level and step-level containers.The additional
e.cfg == nilcheck ensures that steps with their own container configuration don't inadvertently use the DAG-level shared container client. This maintains proper isolation between step-level and DAG-level container execution.
253-272: Well-structured step-level container initialization.The priority-based approach correctly handles step-level containers (Priority 1) over the legacy executor config syntax (Priority 2). The runtime evaluation via
evalContainerFieldsand theShouldStart = truesetting ensure proper container lifecycle management. Error wrapping provides clear context for troubleshooting.
329-386: Clean runtime evaluation helpers with appropriate scope.The
evalContainerFieldsfunction correctly evaluates only the fields that commonly contain runtime variables (strings and slices) while preserving enum/boolean fields as-is. The granular error wrapping per field aids debugging, and theevalStringSlicehelper maintains clean separation of concerns.internal/core/spec/step.go (3)
196-201: Clear ordering dependency documentation.The explicit comment documenting why
buildStepContainermust precedebuildStepExecutoris excellent practice. This prevents future refactoring errors and makes the build pipeline's data dependencies clear.
690-708: Well-structured executor inference with clear priority hierarchy.The three-level priority system is correctly implemented and clearly documented:
- Step-level container (most specific) →
dockerexecutor- DAG-level container →
containerexecutor- DAG-level SSH →
sshexecutorThis ensures the most specific configuration takes precedence, which aligns with user expectations.
815-838: Clean container validation with appropriate conflict detection.The
buildStepContainerfunction correctly validates that scripts cannot be used with containers (returningErrContainerAndScriptConflict) and delegates tobuildContainerFromSpecfor the actual parsing logic. This maintains separation of concerns and reuses existing container parsing infrastructure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/runtime/builtin/docker/eval_test.go (4)
13-213: Consider adding error path test coverage.The test suite thoroughly covers successful evaluation scenarios but lacks tests for error conditions. Consider adding test cases for failure scenarios, such as:
- Evaluation errors from malformed variable syntax
- Missing required variables
- Invalid container field values after substitution
This would align with the coding guideline to "cover failure paths."
Example error test case
{ name: "EvaluationError", setup: func(ctx context.Context) context.Context { env := runtime.NewEnv(ctx, core.Step{Name: "test"}) return runtime.WithEnv(ctx, env) }, input: core.Container{ Image: "${MISSING_VAR}", }, expectError: true, },
205-211: Optional: Enable parallel execution for subtests.Consider adding
t.Parallel()within each subtest to enable concurrent test execution and improve performance.Suggested change
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() ctx := tt.setup(context.Background()) result, err := evalContainerFields(ctx, tt.input)
215-273: Consider adding error path test coverage.Similar to
TestEvalContainerFields, this test suite covers happy paths well but lacks error scenario testing. Consider adding test cases for:
- Invalid variable syntax
- Evaluation errors during substitution
This would provide more comprehensive coverage and align with testing best practices.
265-271: Optional: Enable parallel execution for subtests.Consider adding
t.Parallel()within each subtest for improved test performance.Suggested change
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() ctx := tt.setup(context.Background()) result, err := evalStringSlice(ctx, tt.input)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/runtime/builtin/docker/eval_test.go
🧰 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/builtin/docker/eval_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/runtime/builtin/docker/eval_test.go
🧬 Code graph analysis (1)
internal/runtime/builtin/docker/eval_test.go (2)
internal/runtime/env.go (3)
NewEnv(114-141)WithEnv(375-377)Env(24-54)internal/core/container.go (3)
PullPolicyAlways(90-90)StartupCommand(54-54)WaitForHealthy(62-62)
⏰ 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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1501 +/- ##
==========================================
- Coverage 61.74% 61.46% -0.29%
==========================================
Files 203 203
Lines 22146 22339 +193
==========================================
+ Hits 13675 13730 +55
- Misses 7096 7222 +126
- Partials 1375 1387 +12
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Summary by CodeRabbit
New Features
Bug Fixes
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.