fix(functions): Set serviceAccount for Cloud Functions when custom service account is specified#9598
fix(functions): Set serviceAccount for Cloud Functions when custom service account is specified#9598HassanBahati wants to merge 7 commits into
Conversation
Summary of ChangesHello @HassanBahati, 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 addresses a critical deployment failure for Cloud Functions v2 by ensuring that custom service accounts are properly applied to the function's build configuration. This change prevents issues arising from the absence of a default compute service account, making deployments more robust when specific service accounts are mandated. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly fixes an issue where a custom service account was not being applied to the build configuration for Cloud Functions v2 deployments. The changes are logical, and the accompanying tests are thorough, covering various scenarios including null, shorthand, and full service account specifications. I have one suggestion to improve code consistency, but overall, this is a solid contribution that addresses the reported bug effectively.
|
/gemini |
|
It looks like you tried to invoke a command, but you didn't specify which one. Please use
|
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where Cloud Functions v2 deployments with a custom service account would fail. The fix involves correctly setting the serviceAccount property in the buildConfig of the function. The changes look good and are well-tested.
I've added a couple of suggestions to improve code maintainability by reducing duplication in both the implementation and the tests. One suggestion is to refactor the service account handling to avoid repetitive logic, and the other is to remove a redundant test case.
Overall, this is a solid fix for an important bug.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request addresses a bug where the service account for the build process was not being set for Cloud Functions v2, leading to deployment failures when the default compute service account is unavailable. The changes correctly set both buildConfig.serviceAccount and serviceConfig.serviceAccountEmail when a custom service account is specified in the function options. The implementation properly handles setting, unsetting (with null), and expanding shorthand service account notations. The accompanying test updates are thorough and validate the new behavior. The changes are well-implemented and directly resolve the reported issue.
|
@cabljac Is there any progress on this PR? |
|
@andrewbrook @christhompsongoogle @joehan I apologize for drawing attention in this manner. I would like to know if this PR is allowed to be merged. |
I'll let Joe take a look - I won't merge it this week though since we're in a code chill prior to Google I/O next week |
I really appreciate the progress on this matter. I will temporarily fork the repository myself and adopt the changes from this PR. |
There was a problem hiding this comment.
Ran this through gemini and it also suggested some additional test cases:
- updateFunction Mask Verification: updateFunction unit tests in src/gcp/cloudfunctionsv2.spec.ts currently only verify default environment variable injection. A test case should be added to verify that updating a function with a custom or null buildConfig.serviceAccount correctly populates the updateMask query parameter.
- endpointFromFunction Reverse Mapping Tests: While the forward transformation (functionFromEndpoint) is thoroughly tested, the inverse transformation (endpointFromFunction) should include test cases ensuring that when serviceAccountEmail is null or omitted in a GCF API response, endpoint.serviceAccount is correctly mapped to null or undefined.
| : proto.formatServiceAccount(from, endpoint.project, true /* removeTypePrefix */), | ||
| ); | ||
|
|
||
| if (Object.prototype.hasOwnProperty.call(endpoint, "serviceAccount")) { |
There was a problem hiding this comment.
nit: Can you leave a comment here on why we're not using convertIfPresent like how the rest of the function is using the proto API? This could help with readability.
wandamora
left a comment
wandamora
left a comment
There was a problem hiding this comment.
New to this codebase and at a glance this looked good, but after running a sanity check through Gemini, i left some suggestions.
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
6 participants
Fixes 8841
Description
When deploying Cloud Functions v2 with a custom service account specified via setGlobalOptions({ serviceAccount: "..." }), the Firebase CLI fails if the default compute service account has been deleted.
Scenarios Tested
Sample Commands