[eas-build-job][build-tools][steps] Accept legacy command/path custom function shape in function.yml; rename references in the consumers - #4095
Conversation
e893fae to
3b018d6
Compare
3b018d6 to
891ef23
Compare
891ef23 to
27f53d7
Compare
27f53d7 to
e8d69a0
Compare
e8d69a0 to
702bea2
Compare
961f32c to
9d5f159
Compare
9d5f159 to
36a1945
Compare
🤖 AI code reviewDecision: Ready for human review (with comments) Overall PR risk: Medium. The change modifies an existing shared path — the composite/local function loading and expansion used by EAS workflows and build hooks — rather than being purely additive. Existing composite-function workflows keep working because 🟡 Warning (1)
This review is advisory — it never blocks a merge and never auto-approves. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4095 +/- ##
==========================================
+ Coverage 63.28% 63.36% +0.08%
==========================================
Files 1024 1027 +3
Lines 46699 46833 +134
Branches 9848 9872 +24
==========================================
+ Hits 29547 29669 +122
- Misses 15698 15709 +11
- Partials 1454 1455 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ef13e90 to
ca56231
Compare
975902e to
6faae3b
Compare
6faae3b to
709f6de
Compare
709f6de to
fb38166
Compare
abf928e to
c4b9c7b
Compare
hSATAC
left a comment
There was a problem hiding this comment.
I took a pass through this and left two inline questions. I don't have much context on legacy custom build functions, so I'm mostly calling out a couple of compatibility differences I noticed rather than making a call on the intended behavior.
Neither is meant to block from my side, and I'll leave the final approval to Stanley since he has more context on the intended compatibility scope.
|
|
||
| const LegacyFunctionPlatformZ = z.enum(['darwin', 'linux']); | ||
|
|
||
| const LegacyFunctionBaseZ = z.object({ |
There was a problem hiding this comment.
Non-blocking, and this may be intentional: should the legacy schema also accept the camelCase spellings?
From what I can tell, those are the native keys in the old Joi schema and snake_case is added as a rename alias, so supportedRuntimePlatforms, defaultValue, allowedValues, and allowedValueType work in custom builds today. Here they fail with the generic union error:
command: echo hi
supportedRuntimePlatforms: [darwin]Maybe only grandfathering the documented snake_case shape is what we want. If so, could we add a rejection test to make that boundary explicit? Otherwise I think these branches should accept both.
There was a problem hiding this comment.
👍 Good point, we should just keep full compatibility
| return config.runs === undefined; | ||
| } | ||
|
|
||
| export type LocalFunctionCatalog = Record<string, LocalFunctionConfig>; |
There was a problem hiding this comment.
Minor and non-blocking: do we want to keep a deprecated alias for CompositeFunctionCatalog? It has been exported from the published package since 21.3.0, so removing it would be a source break for existing importers.
/** @deprecated Use `LocalFunctionCatalog`. */
export type CompositeFunctionCatalog = LocalFunctionCatalog;The widened return types of buildCompositeFunctionCatalogFromStepsAsync and buildLocalCompositeFunctionCatalogAsync look like another possible source break: the loader still only returns CompositeFunctionConfig, but downstream code accessing catalog[path].runs.steps may stop compiling. Would it make sense to keep those return types composite-only until the loader starts returning legacy functions?
If we'd rather make both changes now, should they go under breaking changes in the changelog so the version bump comes out right?
There was a problem hiding this comment.
Restored the old names via aliases to avoid breaking changes. My thinking was that we're fine to change this, thes packages are for internal stuff only, but right, this is public and can be used by some outside consumer.
c4b9c7b to
c72448e
Compare
c47e9c1 to
e704845
Compare
|
⏩ The changelog entry check has been skipped since the "no changelog" label is present. |
Why
We want to grandfather the legacy reusable functions from custom builds (https://docs.expo.dev/custom-builds/functions/) into the new reusable functions that can be used in workflows. This is to make the migration from custom builds to workflows easier.
Currently, we only support composite functions. The first step towards extending the support is modifying the
eas-build-jobZod schema.How
legacyFunctionmodule with Zod schemas to match the legacy custom builds functions.LegacyCommandFunctionConfigZ(functions withcommand:present) andLegacyPathFunctionConfigZ(functions withpath:present).compositeFunctionand created a newlocalFunctionmodule with schema that is a union of composite function schema and legacy function schemas.stepsandbuild-toolsthat should reference "local functions".Test Plan
Added unit tests.