-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat: Add experimental support to discover functions via dynamic generation of functions.yaml #8848
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Add detectFromStdio() function to parse base64-encoded manifest from stderr - Extract findFunctionsBinary() to avoid code duplication - Add execAdmin() method for stdio discovery execution - Modify discoverBuild() to use stdio when FIRESTORE_EMULATOR_HOST is set - Stdio discovery avoids HTTP server overhead and improves reliability on resource-constrained CI/CD machines - Maintains backward compatibility with existing HTTP discovery This implementation requires firebase-functions SDK v6.3.2+ with stdio discovery support.
…T_OUTPUT_PATH - Replace stdio XML parsing with file-based discovery mechanism - Functions SDK writes manifest to path specified by FUNCTIONS_MANIFEST_OUTPUT_PATH env var - CLI reads manifest from file path and cleans up after reading - Preserves stderr capture for error reporting - Updates discovery tests to use new env var instead of FUNCTIONS_DISCOVERY_MODE - Maintains backward compatibility - uses HTTP discovery when env var not set
- Update all fixtures to use local firebase-functions from /Users/daniel/dev/firebase-functions - Add runtime: nodejs22 to all firebase.json files - Add stress-test fixture with 20 functions to test scalability - Fix package.json syntax issues (trailing commas, empty engines)
…nto feat/stdio-discovery
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @taeold, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces an experimental, file-based function discovery mechanism for Firebase Functions. Instead of relying on an HTTP server to expose function metadata, this new approach allows the user's function code to dynamically generate a functions.yaml manifest file, which the Firebase CLI then reads to determine available functions. This change aims to provide a more robust and potentially faster discovery method, especially for complex projects or environments where HTTP-based discovery might be problematic.
Highlights
- New Function Discovery Mechanism: I've added experimental support for a new function discovery method that relies on user code dynamically generating a
functions.yamlmanifest file. This provides an alternative to the existing HTTP-based discovery. - Environment Variable Activation: This new file-based discovery is activated by setting the
FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATHenvironment variable, which specifies the output location for the generatedfunctions.yaml. - Refactored Discovery Logic: The core function discovery logic has been updated to prioritize static YAML discovery, then the new file-based discovery (if the environment variable is set), and finally fall back to the existing HTTP-based discovery.
- Enhanced Test Suite: I've added a new 'stress-test' fixture to validate the discovery of a large number of functions and updated existing test fixtures to use the
nodejs22runtime and localfirebase-functionsSDK paths. The test suite now includes specific tests for the new file-based discovery method.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces an experimental feature for discovering functions by dynamically generating a functions.yaml manifest file. The changes include the core logic for this new file-based discovery mechanism, updates to the Node.js runtime delegate to support it, and new tests. Please address the hardcoded paths, potential data loss, and code duplication.
scripts/functions-discover-tests/fixtures/codebases/v1/package.json
Outdated
Show resolved
Hide resolved
scripts/functions-discover-tests/fixtures/codebases/v2/package.json
Outdated
Show resolved
Hide resolved
scripts/functions-discover-tests/fixtures/esm/functions/package.json
Outdated
Show resolved
Hide resolved
scripts/functions-discover-tests/fixtures/pnpm/functions/package.json
Outdated
Show resolved
Hide resolved
Adds a new environment variable, FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH, to enable an experimental file-based function discovery mechanism. - If FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH=true, a temporary file path will be generated for the discovery manifest (functions.yaml). - If FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH is set to a path, that path will be used to store the discovery manifest. This change also includes a refactoring of the discovery codebase to use the modern fs/promises API instead of util.promisify.
Refactors the function discovery integration tests to use a shared helper function, reducing code duplication and improving maintainability.
Update all test fixtures to use the released firebase-functions v6.4.0 instead of local file references, now that the stdio discovery feature is available in the published package. Also fix Node engine requirement in yarn workspaces fixture to support Node 22.
- Use fs.mkdtemp instead of Date.now() to avoid race conditions in tests - Add proper TypeScript type for childProcess parameter (ChildProcess)
- Replace promisify(fs.mkdtemp) with fs.promises.mkdtemp - Replace promisify(fs.exists) with fs.promises.access - Change discovery manifest log messages from logLabeledBullet to logger.debug - Run npm format to fix code style issues
Remove unconditional file deletion logic that could delete user-specified files. Temporary files in os.tmpdir() will be cleaned up by the OS.
|
/gemini reciew |
|
/gemini review |
|
Thanks for triggering the review, @taeold! I'm now performing a comprehensive code review of your pull request. Please note that your previous command I'll post my findings as review comments shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces an experimental file-based discovery mechanism for Firebase Functions. I've identified a critical issue in the integration tests, a minor typing issue, and an opportunity to refactor duplicated code.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
…rebase-tools into feat/stdio-discovery
- Fix test to use correct env var (FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH) - Refactor detectFromOutputPath to poll for manifest file instead of waiting for process exit - Extract common spawn logic to eliminate code duplication between execAdmin/serveAdmin - Make execAdmin and serveAdmin function signatures consistent - Improve error handling and support partial file writes This makes file-based discovery more robust by allowing the SDK to write the manifest at any time during execution, similar to HTTP discovery.
joehan
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for adding integration test coverage
Reverted the polling implementation back to the simpler approach that waits for process exit before reading the manifest file. The added complexity of polling was not justified since: - The SDK likely writes the file just before exiting anyway - We still need the childProcess parameter - The simpler approach is more predictable and easier to maintain
- Replace || with ?? for nullish coalescing in discovery timeout checks
- Add comment explaining defensive config || {} check for external data
- Move stderr handling to spawnFunctionsProcess for consistency
- Refactor nested if-else to improve readability with early returns
- Add JSDoc comment explaining execAdmin's purpose
- Add TODO for future refactoring of nested promises in serveAdmin
All integration tests passing.
The getFunctionDiscoveryTimeout() function returns 0 when no timeout is set. Using ?? would pass through the 0 value causing immediate timeouts. Using || ensures we fallback to the default timeout for falsy values including 0. Fixes unit test failure: "Timeout after 0"
feat: Add experimental file-based discovery via env var
Adds a new environment variable, FIREBASE_FUNCTIONS_DISCOVERY_OUTPUT_PATH, to enable an experimental file-based function discovery mechanism.
This change also includes a refactoring of the discovery codebase to use the modern fs/promises API instead of util.promisify.