chore: remove experimental gha (GitHub Actions) executor#1983
Merged
Conversation
Drops the experimental `gha` step executor and its `github_action` / `github-action` aliases, the nektos/act integration, associated JSON schema definitions, and all related documentation and skill references. `go mod tidy` prunes nektos/act and ~20 transitive dependencies.
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe GitHub Actions (gha) executor has been completely removed from the codebase, including its runtime implementation, configuration schema, validation rules, tests, and documentation. This affects the dependency graph, core specifications, schema validation, and related test suites. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/cmn/schema/dag.schema.json (1)
1410-1437:⚠️ Potential issue | 🟡 MinorAlign this
step.paramsdescription with the current loader.Line 1437 still mentions a
runfield that this step schema doesn't define, andbuildStepParamsFieldininternal/core/spec/step.gocurrently converts parsed step params intomap[string]stringbefore storing them oncore.Step. The “rich types are preserved” sentence therefore overstates the current behavior.📝 Suggested wording
- "description": "Parameters for the step. Supports three formats: 1) String format: space-separated positional parameters (e.g., 'arg1 arg2') accessible as $1, $2. 2) Array format: named parameters as key-value objects preserving rich types (numbers, booleans, nested objects). 3) Object format: named parameters as simple key-value pairs. For sub-DAG execution (when 'call' or 'run' is specified), these are passed to the sub DAG and accessible as ${KEY}. Rich types are preserved when passing to executors that support them." + "description": "Parameters for the step. Supports three formats: 1) String format: space-separated positional parameters (e.g., 'arg1 arg2') accessible as $1, $2. 2) Array format: named parameters as key-value objects. 3) Object format: named parameters as key-value pairs. For sub-DAG execution (when 'call' is specified), these are passed to the sub DAG and accessible as ${KEY}."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmn/schema/dag.schema.json` around lines 1410 - 1437, Update the "params" description in the step schema to match the loader behavior: remove any mention of a non-existent "run" field, and change the sentence claiming "Rich types are preserved" to state that parsed params are converted to string map form (via buildStepParamsField in internal/core/spec/step.go) and stored on core.Step as map[string]string, so rich types are not preserved when the loader processes step params. Reference the "params" schema entry and the buildStepParamsField/core.Step symbols when making the wording change.
🧹 Nitpick comments (1)
internal/core/spec/step.go (1)
96-97: Make this executor list explicitly non-exhaustive.Line 96 still reads like a full set, but the schema allows many more executor types. Adding
e.g.or pointing to the canonical schema/registry will keep this comment from drifting again.📝 Suggested wording
- // Type specifies the executor type (ssh, http, jq, mail, docker, archive). + // Type specifies the executor type (e.g. ssh, http, jq, mail, docker, archive). + // See internal/cmn/schema/dag.schema.json#/definitions/executorType for the full set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/spec/step.go` around lines 96 - 97, Update the comment on the Step.Type field to indicate the executor list is non-exhaustive (e.g. replace "ssh, http, jq, mail, docker, archive" with "e.g. ssh, http, jq, mail, docker, archive" or add "and others" and/or a pointer to the canonical schema/registry) so the Type string field's doc clarifies that additional executor types are allowed; modify the comment above the Type field (symbol: Type in the Step struct) to include "e.g." or a registry link and/or the phrase "non-exhaustive" to prevent future drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@internal/cmn/schema/dag.schema.json`:
- Around line 1410-1437: Update the "params" description in the step schema to
match the loader behavior: remove any mention of a non-existent "run" field, and
change the sentence claiming "Rich types are preserved" to state that parsed
params are converted to string map form (via buildStepParamsField in
internal/core/spec/step.go) and stored on core.Step as map[string]string, so
rich types are not preserved when the loader processes step params. Reference
the "params" schema entry and the buildStepParamsField/core.Step symbols when
making the wording change.
---
Nitpick comments:
In `@internal/core/spec/step.go`:
- Around line 96-97: Update the comment on the Step.Type field to indicate the
executor list is non-exhaustive (e.g. replace "ssh, http, jq, mail, docker,
archive" with "e.g. ssh, http, jq, mail, docker, archive" or add "and others"
and/or a pointer to the canonical schema/registry) so the Type string field's
doc clarifies that additional executor types are allowed; modify the comment
above the Type field (symbol: Type in the Step struct) to include "e.g." or a
registry link and/or the phrase "non-exhaustive" to prevent future drift.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0b56ee9-5e28-4fe5-962b-1063a5df6857
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
go.modinternal/cmn/schema/dag.schema.jsoninternal/core/spec/step.gointernal/core/spec/step_test.gointernal/core/step.gointernal/intg/gha_test.gointernal/runtime/builtin/builtin.gointernal/runtime/builtin/gha/config.gointernal/runtime/builtin/gha/executor.gointernal/runtime/builtin/gha/executor_test.goskills/dagu/references/executors.md
💤 Files with no reviewable changes (6)
- internal/runtime/builtin/builtin.go
- skills/dagu/references/executors.md
- internal/runtime/builtin/gha/config.go
- internal/intg/gha_test.go
- internal/runtime/builtin/gha/executor.go
- internal/runtime/builtin/gha/executor_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ghastep executor (aliases:github_action,github-action) and its nektos/act integrationgo mod tidydropsgithub.com/nektos/actand ~20 transitive deps (docker/cli, moby/go-archive, shellquote, etc.)Files changed
internal/runtime/builtin/gha/(package) andinternal/intg/gha_test.gointernal/runtime/builtin/builtin.go: removed blank importinternal/core/spec/{step.go,step_test.go},internal/core/step.go: removedgha/github_action/github-actionfrom capability registration, test cases, and doc commentsinternal/cmn/schema/dag.schema.json: removedghaExecutorConfigdefinition, theif/thenbranches that wired it to the three type aliases, and the enum entriesskills/dagu/references/executors.md: removed## ghasectiongo.mod/go.sum: pruned unused depsTest plan
go build ./...go test ./internal/core/spec/... ./internal/cmn/schema/... ./internal/runtime/builtin/...Summary by CodeRabbit
Release Notes
Removed Features
ghaexecutor type will no longer execute.Chores