feat: add new runtimeVersion to manifest #1367
Conversation
⏱ Benchmark resultsComparing with f9e621b largeDepsEsbuild: 2s⬇️ 4.56% decrease vs. f9e621b Legend
largeDepsNft: 7.6s⬇️ 7.89% decrease vs. f9e621b Legend
largeDepsZisi: 15s⬇️ 5.73% decrease vs. f9e621b Legend
|
danez
changed the title
src/manifest.ts
Outdated
| mainFile, | ||
| name, | ||
| path, | ||
| runtime, | ||
| runtimeID, |
There was a problem hiding this comment.
I find it a bit confusing to have runtime and runtimeID here. My understanding is that the former comes from the input and the latter is a coerced version of that? Do we need them both here? If so, can we do anything to make the distinction a bit clearer?
There was a problem hiding this comment.
Yes this was also annoying me.
runtime is js, rs, go
Where runtimeID now is the actual runtimenodejs18.x
Imho, runtime should probably called language, but I didn't want to change an existing field. Other solution would be instead of runtimeID add a field called nodeVersion where we simply add the node version that zisi received and use it in bitballoon/FO to decide on the runtime. Although that would not allow the deployment config API to set a runtime, but maybe if they set a node-version would be okay? What do you think?
There was a problem hiding this comment.
I am waiting on this for reviewing the rest of the PRs, as this field it's going to be change it makes the other PRs obsolete or in needs of a change. My two cents: I need to always check in code what these 2 are.
language and runtimeID seems good for me, as it kind of match the description for AWS lambda runtimes.
By this comment, looks like we are going to make a PR in every repository touching runtimes, so maybe the effort of renaming them is not that big?
There was a problem hiding this comment.
I went for using one field for either language or the runtime name. Zisi writes both (runtime and runtimeName) to the manifest, because I'm not sure where runtime might be used outside of buildbot/open-api.
open-api then reads both from the manifest and sends runtimeName || runtime to the API call.
danez
changed the title
No breaking changes anymore. the |
danez
changed the title
…1367) * feat: add new runtimeName to manifest * chore: increase test timeout * chore: fix docs * chore: rename field
Summary
The option
nodeVersionnow works differently and allows different styles of how the Node.js version is specified.If it cannot be coerced to a Node.js version, the
undefinedis added to the manifest, leaving the choice of runtime to origin. We could also change this to have zisi add the default to the manifest, but I didn't do it because I feel its nicer to have the default version only in one place.How it is supposed to work is that
@netlify/buildsetsnodeVersiontoAWS_LAMBDA_JS_VERSION || buildUserNodeVersionand based on this input we set a new fieldruntimeVersionin the manifest, or not if it is not a valid runtime version.open-api reads the manifest and sends
runtimeVersion || runtime. In the origin we then detect if it is a runtime shortcut 'js' or a full runtime image name.Ref: netlify/pod-compute#33
Ref: netlify/pod-compute#64
This would also allow the runtime being set by the deployment config API.