[steps][3/4] Interpolate composite function inputs - #4014
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4014 +/- ##
==========================================
+ Coverage 61.29% 61.45% +0.16%
==========================================
Files 977 978 +1
Lines 43687 43829 +142
Branches 9186 9216 +30
==========================================
+ Hits 26774 26931 +157
+ Misses 15465 15450 -15
Partials 1448 1448 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
66faed4 to
6ac7f96
Compare
c5be755 to
68b6cdf
Compare
68b6cdf to
e31b8bc
Compare
6ac7f96 to
42ee777
Compare
e31b8bc to
7937bd7
Compare
30592bd to
ff1c31a
Compare
709a789 to
1b037d3
Compare
4ed949b to
7428b06
Compare
1b037d3 to
051d1fa
Compare
7428b06 to
83c2e00
Compare
051d1fa to
515cccd
Compare
83c2e00 to
7082ec5
Compare
515cccd to
901e44a
Compare
7082ec5 to
d5d55e3
Compare
8c24784 to
6613344
Compare
a41b6a3 to
26aa626
Compare
6613344 to
c92328e
Compare
26aa626 to
73e1e7e
Compare
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
hSATAC
left a comment
There was a problem hiding this comment.
Looks great — the scoping (lazy inputs, caller/callee resolution, env boundary, the if memoization) is subtle but holds together, and the tests are really thorough. One naming nit inline, not blocking.
|
|
||
| const compositeFunction = this.lookupCompositeFunction(compositeFunctionPath); | ||
| const innerSteps = compositeFunction.runs.steps; | ||
| const action = this.lookupCompositeFunction(compositeFunctionPath); |
There was a problem hiding this comment.
Small naming thing: this renames the compositeFunction local to action (also in lookupCompositeFunction) and adds buildActionInputs. Everything else sticks to "composite function", so "action" stands out a bit — mind keeping these as compositeFunction / buildCompositeFunctionInputs?
There was a problem hiding this comment.
Yes yes thank you for catching this! I originally implememented everything with the "actions" naming but then decided that "composite functions" fits this better and renamed. This one got away.
73e1e7e to
7a0d089
Compare
672cc04 to
7e50713
Compare
7a0d089 to
c50f0a3
Compare
7e50713 to
c88f637
Compare
| /** Merge call-site env from each scope level, outer to inner; inner keys win. */ | ||
| public resolveInheritedEnv(base: JobInterpolationContext): BuildStepEnv { | ||
| const parentEnv = this.parent?.resolveInheritedEnv(base) ?? {}; | ||
| return { ...parentEnv, ...this.resolveScopeEnv(base) }; |
There was a problem hiding this comment.
codex tells me
# Workflow
- uses: ./outer
env:
OUTER: visible
and
# outer/function.yml
runs:
steps:
- uses: ./inner
env:
COPIED: ${{ env.OUTER }}
won't work if we don't add parentEnv to resolveScopeEnv call
| expect(error.message).toMatch(/Invalid identifier "name"/); | ||
| }); | ||
|
|
||
| it('skips the allowed_values check for a reference-valued constrained input', async () => { |
There was a problem hiding this comment.
is this right? should we skip the allowed values check for interpolated values?
There was a problem hiding this comment.
This test documents a deliberate decision to not implement this check.
The reason behind this decision is to stay consistent with how things work currently in workflows (and custom builds), e.g. this
jobs:
build:
steps:
- id: pick
run: set-output artifact_type application-archive
outputs: [artifact_type]
- uses: eas/upload_artifact
with:
type: ${{ steps.pick.outputs.artifact_type }}
Even though type input in upload_artifact has a set of "allowed values", it's not validated for interpolated values.
We could add the runtime check globally in BuildStepInput, but I worry we would break things for users that depend on this behavior.
Alternatively, we can add this check just for reusable functions, so that there's validation from day one in this new feature.
There was a problem hiding this comment.
of course it's "already broken", what was i thinking
| ).toBe(''); | ||
| }); | ||
|
|
||
| it('does not error for a missing required input that is never referenced', async () => { |
There was a problem hiding this comment.
Actually let's harden it. Made it eagerly require something present when required: true in parse time. But not adding validations that the interpolation resolves to something (not undefined) for the same reasons as in the previous comment, documented it in a test too.
Replace the short-id alias map with childrenByLocalId so the scope reads outputs from the call's own children instead of a hidden global steps view.
|
⏩ The changelog entry check has been skipped since the "no changelog" label is present. |

Why
This PR stack adds support for local composite functions in workflows. After structural expansion, inner steps still could not see
${{ inputs.* }}, and callerenvdid not yet reach them at runtime.This PR adds the data layer: input declaration, validation and interpolation, and runtime resolution of the caller context (env, templated working directories).
How
CompositeFunctionExpanderbuilds declared composite function inputs as realBuildStepInputinstancesBuildStepCompositeFunctionScopeexposes${{ inputs.* }}as lazy gettersenvand working directory templating resolve through the composite function scopeTest Plan
Added inputs and scoping unit tests.