[build-tools][steps] Load composite function catalogs for hook steps - #4063
[build-tools][steps] Load composite function catalogs for hook steps#4063sswrk wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4063 +/- ##
==========================================
+ Coverage 62.64% 62.66% +0.03%
==========================================
Files 999 999
Lines 45175 45207 +32
Branches 9483 9488 +5
==========================================
+ Hits 28294 28326 +32
Misses 15433 15433
Partials 1448 1448 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b9e011c to
eb60cb4
Compare
eb60cb4 to
e2453da
Compare
e2453da to
be59908
Compare
be59908 to
e65600c
Compare
31f42c2 to
b5884c9
Compare
b5884c9 to
e810dd3
Compare
e810dd3 to
2f52da6
Compare
2f52da6 to
55401ab
Compare
55401ab to
078f9c2
Compare
ff09ed2 to
c302845
Compare
c302845 to
ae26d93
Compare
ae26d93 to
400067c
Compare
There was a problem hiding this comment.
Overall this looks good to me, and the native/steps split makes sense. I left a couple of non-blocking inline comments on things I wasn’t sure were intentional.
One separate follow-up: eas workflow:validate only checks composites in job.steps, so references from hooks or defaults.hooks could pass validation and then fail on the worker. Not blocking this PR, but worth tracking.
400067c to
f940337
Compare
Thanks for thinking about everything. This is coming in the next PR, I'm also looking into reducing the duplication between build-tools and eas-cli. |
8fa56f7 to
d156a2e
Compare
There was a problem hiding this comment.
The lazy loader addresses what I raised last time, and the rejectingLoader() test covers it. Removing hooks from buildCompositeFunctionCatalogAsync also resolves my other comment.
I left one blocking comment in jobHooks.ts about the hook error messages regressing from main. The other three are non-blocking.
| const catalogRootSteps = wrappedAnchors.flatMap(anchor => | ||
| (['before', 'after'] as const).flatMap(side => { | ||
| const steps = hooks[`${side}_${anchor}`]; | ||
| return Array.isArray(steps) ? steps : []; | ||
| }) | ||
| ); | ||
| let compositeFunctionCatalog: CompositeFunctionCatalog; | ||
| try { | ||
| compositeFunctionCatalog = await buildCompositeFunctionCatalogAsync( | ||
| ctx.getReactNativeProjectDirectory(), | ||
| { steps: catalogRootSteps, logger: ctx.logger } | ||
| ); | ||
| } catch (err) { | ||
| throw new UserError( | ||
| ErrorCode.HOOKS_ERROR, | ||
| `Failed to load a local composite function referenced from the job's hooks: ${ | ||
| err instanceof Error ? err.message : String(err) | ||
| }`, | ||
| { cause: err } | ||
| ); | ||
| } |
There was a problem hiding this comment.
This changes hook error reporting on the native path, and I think it’s a regression from main.
On main, a bad composite under a wrapped key is handled by constructHookEntriesAsync, so the error includes the hook key:
Failed to parse hooks.before_install_node_modules: Local composite function "./.eas/functions/missing" does not exist. Expected a "function.yml" (or "function.yaml") file at ...
Here, the catalog build gets there first after catalogRootSteps has flattened the wrapped anchors, so that context is lost:
Failed to load a local composite function referenced from the job's hooks: Local composite function "./.eas/functions/missing" was referenced by a step but no such composite function exists. ...
working_directory makes this more noticeable because it isn’t a load failure at all. collectLocalCompositeFunctionPathsFromSteps throws while collecting paths, but the error still gets the Failed to load prefix:
main:Failed to parse hooks.before_install_node_modules: "working_directory" is not supported ...- here:
Failed to load a local composite function referenced from the job's hooks: "working_directory" is not supported ...
The steps path already avoids this by rethrowing BuildConfigError unchanged. Would it be better to build the catalog per key inside the for (const anchor of wrappedAnchors) loop below? That would keep the key in scope and let BuildConfigError fall through to the existing Failed to parse hooks.${key} wrapper.
The new test only checks the inner no such composite function exists, so it passes with either behavior.
There was a problem hiding this comment.
Fair point about the regression. Applied a suggestion and added a test to assert the expected fomrat of the error message: dbb877b
| } | ||
| } | ||
|
|
||
| for (const hookKey of Object.keys(validatedHooks)) { |
There was a problem hiding this comment.
Non-blocking: this warning can’t be reached for one of the cases it covers.
validateAllStepFunctionsExist runs over every registered hook key, even when its anchor isn’t present. Local composites are skipped and handled by the lazy loader, but a plain uses: still throws before reaching this warning.
For example:
defaults:
hooks:
before_submit:
- uses: eas/some_newer_function
jobs:
mobile_build:
type: build
lint:
type: custom
steps:
- run: yarn lintlint has no submit anchor, so I’d expect only the warning. Instead it fails with Calling non-existent functions: "eas/some_newer_function".
jobHooks.ts deliberately doesn’t validate function existence for unwrapped keys, so native and steps jobs behave differently here. Would it make sense to move this check into constructHookSideEntriesAsync, where it would only run for anchors that actually appear? constructHookEntriesAsync already validates per key that way.
|
|
||
| for (const hookKey of Object.keys(validatedHooks)) { | ||
| const parsed = parseHookKey(hookKey); | ||
| if (parsed !== null && !seenAnchorIds.has(parsed.anchorId)) { |
There was a problem hiding this comment.
Minor and non-blocking: parsed can’t be null here. validateHooks() only keeps keys for which parseHookKey() returned non-null, so this is narrowing rather than a real check.
Would it be better for validateHooks() to return the anchor IDs it already parsed, or type its result as Partial<Record<HookKey, Step[]>>? Either would avoid parsing the keys again here.
| const parsed = parseHookKey(hookKey); | ||
| if (parsed !== null && !seenAnchorIds.has(parsed.anchorId)) { | ||
| this.ctx.baseLogger.warn( | ||
| `Ignoring "hooks.${hookKey}": this build does not run the "${parsed.anchorId}" step.` |
There was a problem hiding this comment.
Minor and non-blocking: jobHooks.ts warns for both an unknown hook key and a registered key whose anchor isn’t wrapped. This adds the second warning with matching wording.
Should validateHooks() also warn about the unknown keys it skips? parseHookKey(hookKey) === null currently just continues, so a typo like before_install_node_module is silently dropped for a steps job but reported for a native one.
🤖 AI code reviewDecision: Approve with comments One warning remains in packages/steps/src/StepsConfigParser.ts. The code logs an untrusted hook key without removing terminal control characters. 🟡 Warning (1)
This review is advisory — it never blocks a merge and never auto-approves. |
|
✅ Thank you for adding the changelog entry! |
|
Re. AI review:
Fair, goes hand in hand with #4063 (comment). Addressed: 060c136
Leaving this out for now, I think it's not really specific to this PR |
Why
Local composite functions can be referenced from hook steps (
uses: ./...), but build-tools only builds the composite catalog from jobsteps.How
steps, pass a lazyloadCompositeFunctionloader so hook composites load only when their anchor runsTest Plan
Added unit tests.