-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add disallowLegacyRuntimeConfig property to skip Runtime Config API during function deploys. #9354
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
When using --only functions:SELECTOR, resolve ambiguous selectors by checking if SELECTOR matches a known codebase name: - If yes: target only that codebase - If no: treat as function ID in default codebase This prevents unnecessary codebase scanning and runtime config fetches for non-targeted codebases. Example: With codebases 'default' and 'other': firebase deploy --only functions:other Before: scans both 'other' codebase AND 'default' for 'other-*' functions After: scans only 'other' codebase
Summary of ChangesHello @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 enhances the Firebase Functions deployment process by introducing a new "disallowLegacyRuntimeConfig" property in firebase.json. This property empowers developers to explicitly prevent the fetching and use of the deprecated functions.config() API during deployment for specific function codebases. This change facilitates a smoother transition away from legacy runtime configuration, especially for new projects where this property will be enabled by default. It also streamlines the internal logic by replacing a global experiment flag with a more granular, codebase-specific configuration option. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
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 the disallowLegacyRuntimeConfig
property to give users control over using legacy Runtime Config on a per-codebase basis. The implementation is solid, with the old experiment flag being correctly removed and replaced by the new configuration. The changes are consistent across the codebase, and I appreciate the addition of comprehensive unit tests for the new logic.
My review includes a few suggestions to improve code maintainability and performance, mainly by refactoring repetitive test code and optimizing a loop. Overall, this is a great change that improves the configuration flexibility for Firebase Functions.
* feat: Add isAvailable function for dynamic tool availability This change introduces a new system for determining tool availability in the MCP server. Each tool now has an `isAvailable` function that returns a boolean, and this function is invoked whenever the available tools are checked. - Added an optional `isAvailable` async function to the `ServerTool` interface and the `tool` factory function. - Refactored `availableTools` to be asynchronous, filtering tools based on the result of their `isAvailable` function. - Updated `FirebaseMcpServer` and all relevant call sites to handle asynchronous tool loading. - Implemented the `isAvailable` function for all tools. - Most tools use the `checkFeatureActive` utility to check for both `firebase.json` configuration and API enablement. - Core tools, like `list_projects` and `get_project`, always return `true`. This addresses the feedback from previous code reviews and ensures a consistent and complete implementation of the dynamic tool availability feature. * Create default availability functions and caching on api availability * Respond to code review comments --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: Cleo Schneider <[email protected]> Co-authored-by: Visu <[email protected]>
…uring function deploys. Adds per-codebase firebase.json property 'disallowLegacyRuntimeConfig' to prevent fetching and using legacy Runtime Config (functions.config()) during deployment. Defaults to false for backward compatibility. Example firebase.json: { "functions": { "source": "functions", "codebase": "default", "disallowLegacyRuntimeConfig": true } }
7c57c5f
to
224673e
Compare
/gemini review |
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 a disallowLegacyRuntimeConfig
property to firebase.json
, allowing developers to opt-out of fetching legacy functions.config()
values. This is a solid step towards deprecating the old runtime config API. The changes are well-implemented across the configuration schema, deployment logic, and initialization templates, and are supported by a good set of tests. I have one suggestion to improve the type definitions for a cleaner configuration schema. Overall, this is a great enhancement.
); | ||
const discoveredBuild = await runtimeDelegate.discoverBuild(runtimeConfig, { | ||
|
||
const codebaseRuntimeConfig = shouldUseRuntimeConfig(codebaseConfig) |
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.
worth logging here? maybe if true
, log a warning that runtime config is going away? tempting to log if false
, but that would probably get too noisy and might confuse.
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.
Will functions.config() just silently fail if someone on v6 has runtime config turned off in their firebase.json but tries to use it in their functions code? the env var the firebase-functions SDK reads for functions.config will just be undefined, right?
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.
yes it would be equivalent to functions.config() returning an empty object
…irebase/firebase-tools into disallow-legacy-runtime-config
Adds per-codebase firebase.json property
disallowLegacyRuntimeConfig
to prevent fetching and using legacy Runtime Config (functions.config()) during deployment. Defaults to false for backward compatibility.Example firebase.json:
New functions codebase initialized using
firebase init function
will have this property set to true. This shouldn't be disruptive since template code uses the v2 API which doesn't allow use offunctions.config()
API anyway.