-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow apphosting:backends:create to be called with --non-interactive #9025
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
/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 adds a non-interactive --force
option to the apphosting:backends:create
command. The changes look good and correctly implement the non-interactive flow by skipping prompts. I've found a couple of areas for improvement. There's an incorrect error message for the --force
flag that should be corrected. Also, I've suggested replacing a type assertion with a runtime check in backend.ts
to improve code safety and readability.
/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 adds a non-interactive mode to the apphosting:backends:create
command, allowing it to be used in CI/CD environments. The changes introduce new command-line options required for non-interactive execution and refactor the doSetup
function to handle both interactive and non-interactive flows.
My review focuses on improving correctness and user experience. I've identified a couple of potential bugs related to handling of undefined
values for rootDir
and type narrowing for location
and backendId
. I've also suggested an improvement to an error message for better clarity. Overall, the changes are well-structured and address the feature request effectively.
/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 adds a non-interactive mode to the apphosting:backends:create
command, allowing for automated backend creation by providing necessary parameters as flags. The changes are well-structured, with logic correctly split between interactive and non-interactive paths. I've found one critical issue related to a TypeScript type assertion that needs to be fixed to ensure the code compiles and runs correctly. Other than that, the implementation looks solid.
/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 non-interactive support for the apphosting:backends:create
command by adding new flags (--backend
, --primary-region
, --root-dir
). The changes are well-implemented, with clear separation of logic for interactive and non-interactive modes, and appropriate validation checks. The related update to the AI prompt in src/mcp/prompts/core/deploy.ts
to leverage this new non-interactive capability is also a great addition. I've found one minor typo in the prompt instructions that should be corrected.
src/apphosting/backend.ts
Outdated
let branch: string | undefined; | ||
if (nonInteractive) { | ||
if (backendId === undefined || primaryRegion === undefined) { | ||
throw new FirebaseError("non-interactive mode requires a backendId and primaryRegion"); |
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.
throw new FirebaseError("non-interactive mode requires a backendId and primaryRegion"); | |
throw new FirebaseError("non-interactive mode requires a --backend and --primary-region"); |
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.
I already have an error like this using --backend
and --primary-region
in the command file that would be triggered for this case.
At this level, anything could be calling this function, so I referenced the args of the function, not the CLI command options.
One thing I'm struggling with in the prompt is making sure the AI first detects that there is already an active Firebase project. It seems to always ask which project to use, even if there is an active project. I don't think this is a launch blocker, but curious if @mbleigh has ideas to fix this Edit: I think it's because I was removing firebase.json each time. Does removing firebase.json unset the active project in the CLI? |
Description
Allow for apphosting:backends:create to be called with --non-interactive. This adds two flags that are required in --non-interactive mode, but if provided even without --non-interactive, they will reduce the number of user interactions required.
Scenarios Tested
firebase apphosting:backends:create --primary-region=europe-west4 --backend=test-non-interactive
firebase apphosting:backends:create --primary-region=europe-west4 --backend=test-non-interactive --non-interactive
firebase apphosting:backends:create --primary-region=europe-west4 --non-interactive
firebase apphosting:backends:create
Sample Commands