feat(spec): simplified executor field syntax#1525
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new executor specification format (type and config fields) for steps alongside maintaining backward compatibility with the legacy executor field. It includes conflict detection, type inference logic, and schema updates to centralize executor type and config definitions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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 |
![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 (2)
schemas/dag.schema.json (1)
556-563: Consider adding schema-level mutual exclusivity validation.The schema allows both
type/configandexecutorfields to be present simultaneously. While the Go code validates this conflict, adding schema-level validation usingoneOfornotwould provide earlier feedback to users with schema-aware editors.Example approach using allOf/not
// In step definition, add to allOf: { "if": { "anyOf": [ { "required": ["type"] }, { "required": ["config"] } ] }, "then": { "not": { "required": ["executor"] } } }Also applies to: 1709-1718
internal/core/spec/step.go (1)
1007-1011: Both "docker" and "container" are registered aliases with identical behavior; consider standardizing on one for consistency.The inconsistency is confirmed: step-level container sets the executor type to "docker" (line 1009), while DAG-level container sets it to "container" (line 1016). Both are registered aliases in
internal/runtime/builtin/docker/executor.go(lines 501–502) pointing to the same executor implementation, making them functionally equivalent. Using the same type in both cases would improve clarity and prevent potential confusion.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/core/spec/step.gointernal/core/spec/step_test.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/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/core/spec/step_test.go
🧬 Code graph analysis (2)
internal/core/spec/step_test.go (3)
internal/core/spec/builder.go (2)
StepBuildContext(25-28)BuildContext(13-22)internal/core/step.go (2)
ExecutorConfig(189-197)Step(13-97)internal/core/spec/loader.go (1)
Load(150-171)
internal/core/spec/step.go (3)
internal/core/spec/builder.go (1)
StepBuildContext(25-28)internal/core/step.go (2)
Step(13-97)ExecutorConfig(189-197)internal/core/spec/errors.go (2)
ErrExecutorTypeMustBeString(23-23)ErrExecutorConfigValueMustBeMap(24-24)
⏰ 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 (9)
schemas/dag.schema.json (2)
1692-1708: LGTM! Clean centralization of executor type and config definitions.The new
executorTypeandexecutorConfigdefinitions provide a single source of truth for executor validation. The enum includes all supported executor types, andexecutorConfigcorrectly usesadditionalProperties: trueto allow flexible executor-specific configuration.
1023-1031: LGTM! New simplified executor syntax fields.The new
typeandconfigfields at the step level properly reference the centralized definitions and include clear documentation that they cannot be used together with the legacyexecutorfield.internal/core/spec/step_test.go (3)
3096-3179: LGTM! Comprehensive unit tests for new executor format.The test cases cover key scenarios:
- Type-only specification
- Type with config
- Config-only (no type)
- New format taking precedence over DAG-level container inference
Good use of table-driven tests following Go idioms.
3181-3237: LGTM! Conflict validation tests are thorough.The tests correctly verify that:
- New format only (type + config) is valid
- Legacy format only (executor) is valid
- Mixing type with executor produces an error
- Mixing config with executor produces an error
- Mixing both type and config with executor produces an error
3239-3357: LGTM! Integration tests validate end-to-end YAML loading.The integration tests effectively verify:
- SSH executor with new format
- HTTP executor with headers config
- jq executor with raw option
- Legacy format backward compatibility
- Conflict detection at load time
The tests use temporary files and the
Loadfunction, providing realistic coverage.internal/core/spec/step.go (4)
97-103: LGTM! New fields for simplified executor syntax.The new
TypeandConfigfields are properly defined with appropriate YAML tags and align with the schema changes. The field placement afterContaineris logical as they represent alternative ways to configure executor behavior.
924-936: LGTM! Conflict validation between new and legacy formats.The validation correctly:
- Detects when new format (
typeorconfig) conflicts with legacy format (executor)- Provides clear error messages guiding users to use the new format
- Prioritizes reporting
typeconflict overconfigconflict when both are present
990-1023: LGTM! Clean executor resolution with correct precedence.The refactored
buildStepExecutorimplements a clear priority chain:
- Step-level
typeandconfig(new format - highest priority)- Legacy
executorfield (backward compatibility)- Step-level
container→ infer "docker"- DAG-level
container→ infer "container"- DAG-level
ssh→ infer "ssh"The early return on line 1010 after setting "docker" for container is correct.
1025-1066: Correct merge semantics in legacy executor parsing.The
parseLegacyExecutorfunction properly:
- Only sets
Typeif not already set (line 1029-1031, 1041-1043)- Only adds config keys if not already present (line 1051-1053)
This ensures the new format fields take precedence when both are technically present (though validateConflicts should prevent this in practice).
However, note that the condition on line 1051 checks
result.ExecutorConfig.Config[k] == nilwhich would allow overwriting zero-value entries but not nil entries. Since Config is initialized asmake(map[string]any), missing keys will correctly trigger the assignment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1525 +/- ##
==========================================
+ Coverage 62.98% 63.00% +0.01%
==========================================
Files 211 211
Lines 23885 23897 +12
==========================================
+ Hits 15045 15057 +12
- Misses 7394 7396 +2
+ Partials 1446 1444 -2
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.