fix: #8168 - enforce webframeworks only when needed #8169

fivecar · 2025-02-05T01:47:59Z

Description

In deployments where --only hosting:boo is used, enforce webframeworks enablement only when the target actually uses webframeworks. This PR:

Refactors prepareFrameworksIfNeeded to make testing easier
Introduces 5 unit tests
Parses --only and matches webframeworks requirement only for webframeworks sites

Fixes #8168

Scenarios Tested

Created a firebase.json with one webframeworks site and one normal site.

Disabled webframeworks and tried deploying --only hosting:frameworkysite, and verified error message
Enabled webframeworks and tried the same deployment, verifying successful deploy
Disabled webframeworks and deployed --only hosting:nonframeworksite and verified success
Enabled webframeworks and deployed the same site, verifying success

And ran all unit tests clean.

In deployments where `--only hosting:boo` is used, enforce webframeworks enablement only when the target actually uses webframeworks.

google-cla · 2025-02-05T01:48:03Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

fivecar · 2025-02-05T01:49:05Z

src/deploy/index.ts

+              (!options.only.includes("hosting:") ||
+                new RegExp(`\\bhosting:${it.target}\\b`).exec(options.only)),


I understand this is lame, and reflects my lack of understanding of all the config flags/etc and how best to parse this. But the point is to make this logic, "Enforce webframeworks if either all sites need to be deployed, or if the site being deployed itself has source").

@fivecar I just pushed this commit to improve readability and to handle the case when using firebase deploy which was erroring out.
All the functionality should remain the same.

Awesome - it looks so much better, @chalosalvador . Thanks for your help and support!

fivecar · 2025-02-06T21:21:35Z

@aalej : This is the PR I submitted for #8168, which you reproduced. Would you mind directing this to the right engineer to approve? Thank you!

aalej · 2025-02-07T11:39:37Z

@fivecar, already shared this PR with our engineering team when I raised the issue. Thanks again for opening up this PR!

…-fix

chalosalvador

Thanks for working on this @fivecar 🚀

…-fix

codecov-commenter · 2025-02-20T10:44:57Z

Codecov Report

Attention: Patch coverage is 56.25000% with 7 lines in your changes missing coverage. Please review.

Project coverage is 51.03%. Comparing base (c0226eb) to head (62c060e).
Report is 70 commits behind head on master.

Files with missing lines	Patch %	Lines
src/deploy/index.ts	56.25%	5 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8169      +/-   ##
==========================================
- Coverage   51.12%   51.03%   -0.10%     
==========================================
  Files         423      423              
  Lines       29722    29930     +208     
  Branches     6083     6134      +51     
==========================================
+ Hits        15196    15275      +79     
- Misses      13160    13277     +117     
- Partials     1366     1378      +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

This reverts commit 4621328.

…8295) * Revert "fix: #8168 - enforce webframeworks only when needed (#8169)" This reverts commit 4621328. * prepare webframeworks only if some is being deployed fixes #8274 * handle only properly * restore types improvements * more tests * cleanup * changelog * enforce return type * remove unnecessary type cast

* fix: #8168 - enforce webframeworks only when needed In deployments where `--only hosting:boo` is used, enforce webframeworks enablement only when the target actually uses webframeworks. * remove console * add changelog, add tests * Add matchesHostingTarget to improve readability --------- Co-authored-by: Chalo Salvador <[email protected]>

* Helper functions and basic setup for dataconnect:sql:setup * Refactor setupIamUsers for better composability * FDC MVP brownfield and greenfield to brownfield schema setup * Add required logic inside schemaMigration for handling brownfield * Cleanup and fix bugs in brownfield setup * Use firebasesuperuser instead of cloudsqlsuper user for brownfield migration success * Add default permissions for brownfield * Fix lint/format * Refactor to allow setup reruns * Fix small things and address comments * Fix bug in role grants * Add logging that database setup completed * Make grant command not go through setup if roles can be granted in brownfield * bug fix from changing the getting schema owner command * Simplify getSchemaMetaData in permissions.ts * Fix log statement * Split permissions.ts into front facing permissions_setup.ts and keep backend permissions there * No need to ask user if they want to rerun greenfield setup * Make setupSQLPermissions return a setup status instead of a boolean * Change an if statment to switch statement * Keep upserting new user in grant command * Bump FDC local toolkit to v1.8.0. (#8210) * Bump FDC local toolkit to v1.8.0. * Update changelog. * First pass at auto generating sdk configs (#7833) * First pass at auto generating sdk configs * Fixed formatting issues * Removed extra command * Deleted unnecessary files * Fixed more linting' * Removed test assertion * Fixed formatting * Updated erros * Misc * Updated platforms list * Undid last changes * Addressed comments * Fixed client test * Driveby type fixing * missed a spot * Fixed test * Fix issue where if a user passes in an empty 'out' parameter, the CLI crashes * Added intelligent sensing where app should be * Fixed formatting * Fixed lint * Fixed app dir * Misc * Wrote tests * Reverted apps sdkconfig changes * Fixed formatting * Small changes * Revert shrinkwrap changes * Updated test:management script * Fixed apps-sdkconfig boolean check * Fixed more boolean * Fixed formatting * Added changelog * Added new options * Removed unused var * Added experimental flag * Moved apps:init behind a flag * Added apps:init command * Removed unnecessary experiments * Addressed comments --------- Co-authored-by: Joe Hanley <[email protected]> * 13.31.0 * [firebase-release] Removed change log and reset repo after 13.31.0 release * FDC Emulator Update v1.8.1(#8216) * 13.31.1 * [firebase-release] Removed change log and reset repo after 13.31.1 release * Update formatting of connector evolution and insecure operation issues. (#8204) * Format INTERACTIVE_ACK issues as table as well and add extra "type" column to table. * Update warning and prompt wording to reflect insecure operations as well as connector evolution issues. * Wording. * Sort issues in table by category and some formatting fixes. * Use correct import path for data connect emulator (#8220) * Use correct import path for data connect emulator * Actually fix it this time * fix the thing i broke and format * Update src/emulator/dataconnect/pgliteServer.ts Co-authored-by: Maneesh Tewani <[email protected]> --------- Co-authored-by: Maneesh Tewani <[email protected]> * Don't surface insecure operations errors in VSCode. (#8215) * Don't surface connector evolution or insecure operation issues in VSCode.' * Fix * Filter by "INSECURE" substring rather than warningLevel. * Add path information to formatted GraphqlError. (#8228) * App Hosting Emulator bug - apphosting emulator info is not complete when env vars for emulators are set (#8231) * fix bug where apphosting emulator info is not complete when env vars for other emulators are set * add proper fix and test * fix * remove async from non-async test func * address comments * Bump FDC local toolkit to v1.8.2. (#8232) * Bump FDC local toolkit to v1.8.2. * Update changelog. * 13.31.2 * [firebase-release] Removed change log and reset repo after 13.31.2 release * fix: #8168 - enforce webframeworks only when needed (#8169) * fix: #8168 - enforce webframeworks only when needed In deployments where `--only hosting:boo` is used, enforce webframeworks enablement only when the target actually uses webframeworks. * remove console * add changelog, add tests * Add matchesHostingTarget to improve readability --------- Co-authored-by: Chalo Salvador <[email protected]> * Added env var to magically import data connect service from console (#8237) * Added env var to magically import data connect service from console * actually check location too * formats * Formats * Add initial delay when loading python functions (#8239) * Improve robustness of function discovery for python Anecdotally, python function discovery is flakey. We propose 2 change in this PR: 1. For python discovery, add a small initial delay for python's admin server to boot. 2. Add a request timeout to retry call to retrieve trigger information. Previously, the default timeout would've been set to OS-level TCP timeout, which in my laptop was between 20~30s. * Add changelog. * Remove per-req timeout to accomodate loading large/slow main.py. * Update changelog. * Revert timeout bump. * Update vscode to 0.13.1 (#8236) * update vscode to 0.13.1 * remove changelog line * Propagate overrides (#8253) * Apply ajv and ajv-formats overrides to shrinkwrap * Apply whatwg-url override to shrinkwrap * npm i to stabilize shrinkwrap --------- Co-authored-by: Joe Hanley <[email protected]> * Print warning about --location removal from apphosting commands. (#8229) `--location` will be removed from apphosting commands in the next major CLI release. Before then, the CLI will print a warning about this removal whenever `--location` is used. * Fix issue where apps:init breaks on app creation (#8258) * Rename MetaData to Metadata * Change setup to set up in firebase error * Improve logger message * Fix bugs in brownfield setup status checks * fix lint issues --------- Co-authored-by: Rosalyn Tan <[email protected]> Co-authored-by: Maneesh Tewani <[email protected]> Co-authored-by: Joe Hanley <[email protected]> Co-authored-by: Google Open Source Bot <[email protected]> Co-authored-by: Mathusan Selvarajah <[email protected]> Co-authored-by: Philip Su <[email protected]> Co-authored-by: Chalo Salvador <[email protected]> Co-authored-by: Daniel Lee <[email protected]> Co-authored-by: Harold Shen <[email protected]> Co-authored-by: Sarah Clark <[email protected]> Co-authored-by: annajowang <[email protected]>

fix: firebase#8168 - enforce webframeworks only when needed

a9c586b

In deployments where `--only hosting:boo` is used, enforce webframeworks enablement only when the target actually uses webframeworks.

fivecar commented Feb 5, 2025

View reviewed changes

fivecar added 2 commits February 4, 2025 17:49

remove console

7b68a0e

add changelog, add tests

6e8b10b

fivecar force-pushed the 8168-fix branch from 9271475 to 6e8b10b Compare February 5, 2025 23:51

Merge branch 'master' into 8168-fix

f3d6a1f

fivecar mentioned this pull request Feb 6, 2025

Hosting deploy wrongly assumes all-or-none webframeworks #8168

Closed

Merge branch 'master' into 8168-fix

6507875

aalej requested a review from jamesdaniels February 7, 2025 14:53

chalosalvador added 2 commits February 14, 2025 12:47

Add matchesHostingTarget to improve readability

00cc1fc

Merge branch 'master' of github.com:firebase/firebase-tools into 8168…

538b024

…-fix

chalosalvador approved these changes Feb 14, 2025

View reviewed changes

Merge branch 'master' into 8168-fix

2fa91a6

chalosalvador enabled auto-merge (squash) February 17, 2025 08:20

chalosalvador disabled auto-merge February 17, 2025 15:37

chalosalvador added 2 commits February 19, 2025 14:36

Merge branch 'master' into 8168-fix

9e1faee

Merge branch 'master' of github.com:firebase/firebase-tools into 8168…

62c060e

…-fix

chalosalvador merged commit 4621328 into firebase:master Feb 20, 2025
48 of 53 checks passed

fivecar deleted the 8168-fix branch February 20, 2025 22:28

leoortizz added a commit that referenced this pull request Mar 6, 2025

Revert "fix: #8168 - enforce webframeworks only when needed (#8169)"

b75b5eb

This reverts commit 4621328.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: #8168 - enforce webframeworks only when needed #8169

fix: #8168 - enforce webframeworks only when needed #8169

Uh oh!

fivecar commented Feb 5, 2025 •

edited

Loading

Uh oh!

google-cla bot commented Feb 5, 2025

Uh oh!

fivecar Feb 5, 2025

Uh oh!

chalosalvador Feb 14, 2025

Uh oh!

fivecar Feb 15, 2025

Uh oh!

fivecar commented Feb 6, 2025

Uh oh!

aalej commented Feb 7, 2025

Uh oh!

chalosalvador left a comment

Uh oh!

codecov-commenter commented Feb 20, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

		(!options.only.includes("hosting:") \|\|
		new RegExp(`\\bhosting:${it.target}\\b`).exec(options.only)),

fix: #8168 - enforce webframeworks only when needed #8169

fix: #8168 - enforce webframeworks only when needed #8169

Uh oh!

Conversation

fivecar commented Feb 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Scenarios Tested

Uh oh!

google-cla bot commented Feb 5, 2025

Uh oh!

fivecar Feb 5, 2025

Choose a reason for hiding this comment

Uh oh!

chalosalvador Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

fivecar Feb 15, 2025

Choose a reason for hiding this comment

Uh oh!

fivecar commented Feb 6, 2025

Uh oh!

aalej commented Feb 7, 2025

Uh oh!

chalosalvador left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

fivecar commented Feb 5, 2025 •

edited

Loading

codecov-commenter commented Feb 20, 2025 •

edited

Loading