-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix issue with init hosting prompts #9404
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
Summary of ChangesHello @aalej, 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 bug in the 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 effectively resolves a logic issue in the firebase init hosting command flow. The primary bug, where prompts for a traditional static hosting setup were incorrectly skipped when the webframeworks experiment was enabled but not chosen by the user, is fixed by restructuring the conditional logic in askQuestions. This ensures the correct prompts are always displayed based on user choice. Furthermore, moving the GitHub Actions initialization prompt to the actuate function is a crucial correction, as it guarantees that the necessary hosting configuration is available, preventing potential errors. The changes are clear, well-justified, and improve the robustness of the initialization process. The code quality is good, and the fix is solid.
As a side note, while reviewing the related initGitHub function, I noticed it seems to access setup.hosting (e.g., setup.hosting.useWebFrameworks), but the hosting setup information appears to be stored in setup.featureInfo.hosting. This is outside the scope of the current changes but might be a pre-existing issue worth investigating in a follow-up.
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.
Thank you for fixing this! The testing LGTM, but lets move initGithub out of actuate
| "Configure as a single-page app (rewrite all urls to /index.html)?", | ||
| ); | ||
| } | ||
| // GitHub Action set up is still structured as doSetup |
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 think we still need to keep this in here instead of allowing it into actuate - initGithub includes interactive prompts, and we need actuate to be safe to run noninteractively
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'm not sure if can move initGitHub into askQuestions. initGitHub uses setup.config.hosting a couple of times, which is only set during actuate.
firebase-tools/src/init/features/hosting/github.ts
Lines 60 to 64 in be433bc
| if (!setup.config.hosting) { | |
| return reject( | |
| `Didn't find a Hosting config in firebase.json. Run ${bold("firebase init hosting")} instead.`, | |
| ); | |
| } |
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.
Updated, moved initGitHub into askQuestions.
It looks like the only scenario Didn't find a Hosting config in firebase.json will error out is during the setup of a fresh project, otherwise it will correctly pick up the existing config.
During the initial set up with a blank project, I think we can instead use setup.featureInfo?.hosting if setup.config.hosting does not have values
Description
Fixes #9403
Scenarios Tested
Set up on 14.21.0 works, so I'm comparing with that one
Plain - blank
Plain - existing
WebFramework - blank
WebFramework - existing
Sample Commands
firebase init hostingNotes
The arrangements of the prompts are now a little different
Moved he
initGitHubcode toactuatesince initGitHub uses
setup.config.hostingfirebase-tools/src/init/features/hosting/github.ts
Lines 60 to 64 in be433bc
which is only set in the
actuatemethodfirebase-tools/src/init/features/hosting/index.ts
Lines 191 to 203 in be433bc
We could also opt for doing
so we could do