feat: support template functions in custom step types#2020
feat: support template functions in custom step types#2020
Conversation
|
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:
📝 WalkthroughWalkthroughCentralized hermetic Go text-template function map into Changes
Sequence Diagram(s)sequenceDiagram
participant StepLoader as Step Loader
participant TemplateFuncs as TemplateFuncs (package)
participant TemplateBuilder as Go Template
participant Runtime as Runtime Executor
StepLoader->>TemplateFuncs: call FuncMap()
TemplateFuncs-->>StepLoader: return hermetic FuncMap (with overrides)
StepLoader->>TemplateBuilder: Parse template with FuncMap (+ json helper)
TemplateBuilder-->>StepLoader: compiled template
StepLoader->>Runtime: provide compiled template + input
Runtime->>TemplateBuilder: Execute(template, data)
TemplateBuilder-->>Runtime: rendered string/args
Runtime->>Runtime: execute task with rendered command/args
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 |
yottahmd
force-pushed
the
custom-step-template-functions
branch
from
d5f2720 to
c214ad2
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/cmn/templatefuncs/functions.go (2)
39-63:joinsilently stringifies non-slice inputs (e.g. maps).The
defaultbranch callsfmt.Sprint(v)for anything that isn't a slice/array, which means passing a map or scalar returns itsfmtrepresentation rather than an error or empty string. That's easy to miss in templates and produces output likemap[a:1 b:2]. Consider returning""(or an error) for unsupported kinds so misuse is visible rather than silently rendering Go's default formatting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmn/templatefuncs/functions.go` around lines 39 - 63, The join template func silently stringifies non-slice/array inputs (e.g. maps) by falling through to fmt.Sprint; update the join function so that after the reflect-based slice/array check (using rv := reflect.ValueOf(v) and rv.Kind()), unsupported kinds (anything not slice/array) return an empty string (or propagate an error) instead of fmt.Sprint(v) so maps and scalars don't silently render their Go fmt representation.
64-74: Consider handlingnilinput incountconsistently withempty/default.
emptyanddefaultexplicitly short-circuit onv == nil, butcount(nil)falls through toreflect.ValueOf(nil).Kind() == Invalidand returns an error. In templates where a pipeline value may be nil (e.g. missing map key), this will surface as a template execution error rather than0. Consider treatingnilas length 0 for symmetry.Proposed tweak
m["count"] = func(v any) (int, error) { + if v == nil { + return 0, nil + } rv := reflect.ValueOf(v) switch rv.Kind() { //nolint:exhaustive // unsupported kinds return an error below🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmn/templatefuncs/functions.go` around lines 64 - 74, The count template func currently errors for nil because reflect.ValueOf(nil).Kind() is Invalid; update the anonymous function stored in m["count"] (the count func) to short-circuit when v == nil (or when reflect.ValueOf(v).IsValid() is false) and return 0, nil so nil is treated as length 0, otherwise proceed to the existing switch on rv.Kind() and keep the default unsupported-type error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/cmn/templatefuncs/functions.go`:
- Around line 126-146: The blockedFuncs list includes entries that do not exist
in slim-sprig v3 (notably "randString" and the crypto-related names
"genPrivateKey", "derivePassword", "buildCustomCert", "genCA",
"genSelfSignedCert", "genSignedCert"); either remove these nonexistent names
from the blockedFuncs slice in functions.go or add a brief comment explaining
they are intentionally retained for compatibility with older/forked sprig
versions, and verify the final list against slim-sprig's current
genericMap/nonhermeticFunctions to ensure you're only blocking real,
non-hermetic functions.
---
Nitpick comments:
In `@internal/cmn/templatefuncs/functions.go`:
- Around line 39-63: The join template func silently stringifies non-slice/array
inputs (e.g. maps) by falling through to fmt.Sprint; update the join function so
that after the reflect-based slice/array check (using rv := reflect.ValueOf(v)
and rv.Kind()), unsupported kinds (anything not slice/array) return an empty
string (or propagate an error) instead of fmt.Sprint(v) so maps and scalars
don't silently render their Go fmt representation.
- Around line 64-74: The count template func currently errors for nil because
reflect.ValueOf(nil).Kind() is Invalid; update the anonymous function stored in
m["count"] (the count func) to short-circuit when v == nil (or when
reflect.ValueOf(v).IsValid() is false) and return 0, nil so nil is treated as
length 0, otherwise proceed to the existing switch on rv.Kind() and keep the
default unsupported-type error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b27e34d-f4bb-4938-a300-d0df05f001f5
📒 Files selected for processing (4)
internal/cmn/templatefuncs/functions.gointernal/core/spec/step_types.gointernal/core/spec/step_types_test.gointernal/runtime/builtin/template/template.go
✅ Files skipped from review due to trivial changes (2)
- internal/core/spec/step_types.go
- internal/core/spec/step_types_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/runtime/builtin/template/template.go
yottahmd
force-pushed
the
custom-step-template-functions
branch
from
c214ad2 to
47ac28b
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/runtime/builtin/template/template_test.go (1)
606-608: Consider two-value type assertion for clearer test failure.The single-value assertion on line 606 will panic with a raw
interface conversionmessage if thejoinsignature ever drifts again (e.g., from a future refactor). Using the two-value form produces a proper test failure with context.♻️ Optional tweak
- fn := funcMap["join"].(func(string, any) (string, error)) + fn, ok := funcMap["join"].(func(string, any) (string, error)) + require.True(t, ok, "join has unexpected signature: %T", funcMap["join"]) result, err := fn(tt.sep, tt.input) require.NoError(t, err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/runtime/builtin/template/template_test.go` around lines 606 - 608, The test currently uses a single-value type assertion for fn := funcMap["join"].(func(string, any) (string, error)) which will panic on mismatch; change this to the two-value form (fn, ok := funcMap["join"].(func(string, any) (string, error))) and add an assertion (e.g., require.True(t, ok, "funcMap[\"join\"] has unexpected type") or require.Truef to include the actual type) before calling fn so the test fails cleanly with context instead of panicking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/runtime/builtin/template/template_test.go`:
- Around line 606-608: The test currently uses a single-value type assertion for
fn := funcMap["join"].(func(string, any) (string, error)) which will panic on
mismatch; change this to the two-value form (fn, ok :=
funcMap["join"].(func(string, any) (string, error))) and add an assertion (e.g.,
require.True(t, ok, "funcMap[\"join\"] has unexpected type") or require.Truef to
include the actual type) before calling fn so the test fails cleanly with
context instead of panicking.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c4acc65-1f48-4985-b22c-d03d03aaa37d
📒 Files selected for processing (6)
internal/cmn/templatefuncs/functions.gointernal/cmn/templatefuncs/functions_test.gointernal/core/spec/step_types.gointernal/core/spec/step_types_test.gointernal/runtime/builtin/template/template.gointernal/runtime/builtin/template/template_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/cmn/templatefuncs/functions.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/spec/step_types.go
- internal/core/spec/step_types_test.go
yottahmd
force-pushed
the
custom-step-template-functions
branch
from
47ac28b to
b45d6db
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
yottahmd
force-pushed
the
custom-step-template-functions
branch
from
625d938 to
16ee167
Compare
|
@coderabbitai review |
1 participant
Summary
jsonhelper while exposing the same pipeline-friendly functions and slim-sprig subsetDocumentation
Testing
Summary by CodeRabbit