Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

wants to merge 5 commits into from

Conversation

fivecar
Copy link

@fivecar fivecar commented Feb 5, 2025 •
edited
Loading

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.

@fivecar
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.
@google-cla Google CLA
Copy link

google-cla bot commented Feb 5, 2025

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
fivecar commented Feb 5, 2025
View reviewed changes
src/deploy/index.ts Outdated
Comment on lines 77 to 78
(!options.only.includes("hosting:") ||
new RegExp(`\\bhosting:${it.target}\\b`).exec(options.only)),
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 fivecar mentioned this pull request Feb 6, 2025
Open
@fivecar
Copy link
Author

fivecar commented Feb 6, 2025

@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
Copy link
Contributor

aalej commented Feb 7, 2025

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

@aalej aalej requested a review from jamesdaniels February 7, 2025 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jamesdaniels jamesdaniels Awaiting requested review from jamesdaniels

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hosting deploy wrongly assumes all-or-none webframeworks
2 participants
@fivecar @aalej