fix: improve error message when deploying to a gcp project where fire… - #10820
fix: improve error message when deploying to a gcp project where fire…#10820IzaakGough wants to merge 14 commits into
Conversation
![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 improves the user experience by catching 404 errors when retrieving the Firebase Admin SDK configuration and throwing a friendly error message if Firebase is not enabled on the Google Cloud project. It also adds corresponding unit tests in functionsConfig.spec.ts. The review feedback correctly points out a style guide violation where any is used for the caught error in src/functionsConfig.ts, suggesting unknown instead.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
getFirebaseConfig also backs functions:delete, functions:export and the ext:* commands, so wording the error as a deploy failure was wrong for most callers. The endpoint also 404s for missing, deleted and inaccessible projects, so state the project was not found and point at projects:addfirebase rather than asserting Firebase is not enabled and linking a console page that may not resolve.
…10379 # Conflicts: # CHANGELOG.md
cabljac
left a comment
cabljac
left a comment
There was a problem hiding this comment.
@IzaakGough looks good. I checked the detection since the change hangs off it: responseToError sets status from the HTTP code (src/responseToError.ts:56-62) and FirebaseError defaults status to 500 (src/error.ts:13), so nothing else can fall into the 404 branch by accident. Reverted the guard and the 404 test goes red, broadened it and the 500 passthrough goes red, so the tests are doing real work. Two nits inline, neither blocking.
Worth a line in the description: #10379 says "403" in the title and body, but the log the reporter pasted is a 404, which is what you matched. Otherwise people will think you handled the wrong status.
Keeps the multi-paragraph message as a template literal alongside the other messages in the file so its shape is visible, and rewords the changelog entry from the user's point of view.
…10379 # Conflicts: # CHANGELOG.md
ajperel
left a comment
ajperel
left a comment
There was a problem hiding this comment.
This looks good to me but it might be worth waiting until next week to get review from @joehan since this will have impacts outside of functions.
I like this since it catches everything across commands nicely and improves the error message. But it does happen a bit into the process after checking a bunch of other stuff. One could imagine we should ensure Firebase is enabled upfront for all (almost all?) commands potentially but that would be a much larger change I expect.
| }); | ||
|
|
||
| afterEach(() => { | ||
| expect(nock.isDone()).to.be.true; |
There was a problem hiding this comment.
Gemini suggests you also add nock.cleanAll(); to ensure that any unused interceptors are cleared between tests, especially if a test fails before reaching assertions.
4 participants
Fixes #10379
This PR improves the error message produced when a user attempts to deploy to a gcp project which doesn't have firebase added/enabled.
Note on the status code: the issue title and body say 403, but the log the reporter pasted is a 404 from
adminSdkConfig, so that is the status matched here.getFirebaseConfigalso backsfunctions:delete,functions:exportand theext:*commands, so the message is command-neutral rather than phrased as a deploy failure. It also doesn't assert a cause, since the same endpoint 404s for a typo'd, deleted, or inaccessible project as well as for a Cloud project without Firebase.