refactor(functions): Create kits install API and include env seeding support - #10967
Conversation
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try the Wiz Code extension for VS Code, JetBrains, or Visual Studio. |
There was a problem hiding this comment.
Code Review
This pull request refactors the function kits installation logic by extracting utility functions and scaffolding helpers from src/commands/functions-kits-install.ts into a new dedicated module under src/functions/kits/, accompanied by comprehensive unit tests in install.spec.ts and env.spec.ts. The review feedback highlights several key improvement opportunities: ensuring robust configuration updates in addInstanceToKitConfig by searching the configuration array instead of relying on object reference identity, utilizing a discriminated union for KitInstanceEnvSeed to enforce compile-time safety when environment variables are provided, and replacing string-based .replace() calls with global regular expressions to prevent partial template replacement bugs.
a5d6c74 to
77ae4fd
Compare
ajperel
left a comment
There was a problem hiding this comment.
I think this likely needs more thought to be the right abstraction as to what we have easily available for ext:migrate to use, but I'm approving it as is since it might be easier to coordinate with Thomas on the next iteration if it's merged and I don't think this is bad so much as not quite right.
| continue; | ||
| } | ||
| if (Array.isArray(value)) { | ||
| normalizedEnvs[key] = value.join(","); |
There was a problem hiding this comment.
This feels like another place where we're exacerbating the problem with how migrate index.js is doing a probably invalid JSON.parse. Is now the time to fix that?
There was a problem hiding this comment.
This handles arrays, but this is a good reminder that I'm not handling map values here. I'm going to serialize map/object values into a JSON string, as that is what the template expects.
| ); | ||
| } | ||
|
|
||
| export const command = new Command("functions:kits:install") |
There was a problem hiding this comment.
I think the biggest thing is we need to be clear on what needs to be in this command file vs. a library more. Honestly my first take would have been that functions-kits-install is only this new command and everything else lives in functions/kits/install.ts for maximum flexibility.
And even there... what does ext:migrate want to have in overlapping behavior. We still in that flow want to prompt about 3rd party, shrink wrap, etc. I think even the majority of this command should probably move into the library and ext migrate can do something like:
installKit({
package: <identified npm package>,
template: <migration template>,
env: <dict>
})```
And we have a discussion of if that should also accept kit id or kit instance id into the install command. And then basically `ext:migrate` once it has identified what kit needs to be installed can delegate all details of it to `installKit` and get back the info it needs (kit name, instance id, etc.) Or provide it upfront.
We could land this and iterate in a future PR, or see if you can get a few minutes with Thomas and iterate on it in this PR. I'm not sure what's better.
There was a problem hiding this comment.
Thanks for calling this out. I moved the remaining logic to the library and created an InstallKitOrInstance (just to be a bit more self-documenting about how it can add either), and the function should be able to take kit or instance id to override defaults. This function also returns an "action" field to provide more information on what action was performed. I think this warrants another review, PTAL.
77ae4fd to
ed5d6dd
Compare
ajperel
left a comment
There was a problem hiding this comment.
Nice. I think this makes things maximally re-useable for ext:migrate and very composable/easy to change from here if needed.
Description
This PR refactors the Function Kits installation workflow from
src/commands/functions-kits-install.tsinto a reusable, programmatic library insrc/functions/kits/, enabling shared usage by bothfirebase functions:kits:installandfirebase ext:migrate:src/functions/kits/install.ts):installKitOrInstance(options: InstallKitOrInstanceOptions): Promise<InstallKitOrInstanceResult>, which coordinates the full installation flow (package validation, security check/prompt, ID prompting/resolution, scaffolding, npm install/build, config update, env seeding, and deploy reporting).installedKit), adding instances to an existing kit (addedInstance), and environment guidance for existing instances (configuredEnv).kitIdandinstanceIdoverrides inInstallKitOrInstanceOptionsto allow programmatic callers (likeext:migrate) to specify IDs directly and bypass interactive prompts.promptKitId,promptKitInstanceId,promptSecurityConfirmation,promptExistingInstanceForProject,addKitInstanceOrConfigureProject) and deploy reporting (printKitFirstDeployReport) into the library.src/functions/kits/env.ts):seedKitInstanceEnvto write/seed instance environment variables into.env.<projectId>, supporting primitives, arrays (comma-separated), and nested map/JSON objects (e.g. for migrating extension parameters).src/commands/functions-kits-install.ts):kitsexperiment, validate input presence, and delegate directly toinstallKitOrInstance.src/functions/kits/*.spec.ts,src/commands/functions-kits-install.spec.ts):Scenarios Tested
npm-shrinkwrap.jsonsecurity confirmation flows.installationandmigrationtemplates..env.<projectId>with strings, numbers, booleans, arrays, and JSON objects.firebase.json.functions:kits:install.Sample Commands
# Existing CLI usage remains unchanged: firebase functions:kits:install --package @firebase-functions-kits/firestore-bigquery-export