Add support for deploying local source to App Hosting #8516

blidd-google · 2025-05-07T20:23:14Z

firebase deploy now deploys source code on the user's local machine to the corresponding backend on App Hosting. Guides the user through backend creation if the backend does not exist yet.

Note that firebase deploy relies on a new apphosting section in the firebase.json file, which can be initialized using the enhanced firebase init apphosting command (implemented here #8479).

annajowang · 2025-05-08T01:13:17Z

some changes from https://github.com/firebase/firebase-tools/pull/8479/files are showing up in this PR
could you separate these?

I'm happy to do a fast review of https://github.com/firebase/firebase-tools/pull/8479/files after comments have been resolved so you can merge and get those changes rebased into this PR

meanwhile will try an initial review using https://github.com/firebase/firebase-tools/compare/b5697e6209372606c2692bafe253c0fc9f9280dd..fdc6255ec928bac4a3871572793d856703674a06?diff=split&w

annajowang

initial review of just the first few files in https://github.com/firebase/firebase-tools/compare/b5697e6209372606c2692bafe253c0fc9f9280dd..fdc6255ec928bac4a3871572793d856703674a06?diff=split&w

I skimmed the others. Could you just pull out src/gcp/apphosting.ts, src/gcp/devConnect.ts, and src/gcp/storage.ts changes into other PRs, and I can take a quick look and then stamp, before continuing with this PR

annajowang · 2025-05-08T03:06:17Z

src/apphosting/backend.ts

@@ -250,12 +251,31 @@ export async function ensureAppHostingComputeServiceAccount(
      );
    }
  }
+  if (deployFromSource) {


Can you add a comment in the code explaining why this new roles/storage.objectViewer stuff is necessary

^ for better code comprehension e.g. letting reader know that deploy from source has the source code bundled in storage buckets so we need access to that bucket.

but also, now I see below that roles/storage.objectViewer is also added in provisionDefaultComputeServiceAccount() below, which is called in this ensureAppHostingComputeServiceAccount()

and I'm not sure why?

either there's a better way to do it to make things more clear natively or let's document what we're doing and explain any weird/confusing stuff.

annajowang · 2025-05-08T15:46:39Z

src/commands/deploy.ts

@@ -23,6 +23,7 @@ export const VALID_DEPLOY_TARGETS = [
  "remoteconfig",
  "extensions",
  "dataconnect",
+  "apphosting",
 ];
 export const TARGET_PERMISSIONS: Record<(typeof VALID_DEPLOY_TARGETS)[number], string[]> = {


does apphosting require any permissions we need to check for?

added calls with ensureApiEnabled to check if App Hosting API is enabled. Double checking with team which specific IAM permissions are required

annajowang

reviewed up to src/deploy/apphosting/deploy.ts

src/deploy/apphosting/index.ts

src/deploy/apphosting/deploy.ts

annajowang · 2025-05-09T00:55:19Z

src/deploy/apphosting/deploy.ts

+  }
+
+  // Ensure that a bucket exists in each region that a backend is or will be deployed to
+  const backendLocations = context.backendLocations.values() || [];


if context.backendLocations is empty, i would expect .values() to just return [], so we don't need the || [] ?

also nit: can we inline context.backendLocations.values() into the for loop as well?

annajowang · 2025-05-09T01:06:01Z

src/deploy/apphosting/deploy.ts

+        try {
+          await gcs.createBucket(projectId, {
+            name: bucketName,
+            location: "US-CENTRAL1",


does the bucket location also need to match the apphosting location? or are we just creating a bucket for each apphosting location?

We're creating a bucket on demand for every location that has an deploy-from-source backend in it.

For example: the user is trying to deploy from source backend A in us-central1, but there's no bucket yet in us-central1 so we create it. There is also a backend B in asia-east1, but the user hasn't tried to deploy B from source yet, so we don't create that bucket.

So should this line be:
location: loc
not:
location: "US-CENTRAL1"

src/deploy/apphosting/deploy.ts

annajowang · 2025-05-10T00:02:26Z

src/deploy/apphosting/prepare.ts

+      if (options.force) {
+        throw new FirebaseError(
+          `Failed to deploy in non-interactive mode: backend ${cfg.backendId} does not exist yet, ` +
+            "and we cannot create one for you because you must choose a primary region for the backend. " +
+            "Please run 'firebase apphosting:backends:create' to create the backend, then retry deployment.",
+        );
+      }
+      logBullet(`No backend '${cfg.backendId}' found. Creating a new backend...`);
+      const location = await promptLocation(
+        projectId,
+        "Select a primary region to host your backend:\n",
+      );
+      const webApp = await webApps.getOrCreateWebApp(projectId, null, cfg.backendId);
+      if (!webApp) {
+        logWarning(`Firebase web app not set`);
+      }
+      const createBackendSpinner = ora("Creating your new backend...").start();
+      const backend = await createBackend(
+        projectId,
+        location,
+        cfg.backendId,
+        null,
+        undefined,
+        webApp?.id,
+      );
+      createBackendSpinner.succeed(`Successfully created backend!\n\t${backend.name}\n`);
+      context.backendConfigs.set(cfg.backendId, cfg);
+      context.backendLocations.set(cfg.backendId, location);


Can you pull this into some separate doSetupSourceDeploy() function that lives in https://github.com/firebase/firebase-tools/blob/master/src/apphosting/backend.ts

This has a too much repeat code from deSetup(): https://github.com/firebase/firebase-tools/blob/master/src/apphosting/backend.ts#L74

if we don't have time to refactor and reduce the duplicate code here, these should at least live near each other, so we're more likely to dedupe in the future and we're more likely to change things in both places and keep the UX behaviors consistent.

Also it seems like the big block of code above

await Promise.all([ ensure(projectId, developerConnectOrigin(), "apphosting", true), ensure(projectId, cloudbuildOrigin(), "apphosting", true), ensure(projectId, secretManagerOrigin(), "apphosting", true), ensure(projectId, cloudRunApiOrigin(), "apphosting", true), ensure(projectId, artifactRegistryDomain(), "apphosting", true), ensure(projectId, iamOrigin(), "apphosting", true), ]); await ensureAppHostingComputeServiceAccount( projectId, /* serviceAccount= */ null, /* deployFromSource= */ true, );

should go into this new doSetupSourceDeploy(), since it's also in doSetup() and all these don't need to be made when the backend already exists?

This is repeated below:

context.backendConfigs.set(cfg.backendId, cfg); context.backendLocations.set(cfg.backendId, location);

can you move this to the bottom of the for loop

I see your point about having the create logic in the same module, I've refactored it into a doSetupSourceDeploy() function in https://github.com/firebase/firebase-tools/blob/master/src/apphosting/backend.ts. However, I think de-duping might be a bit premature; refactoring the duplicate logic into a helper function would require introducing new arguments to that helper (i.e. we would need an optional backendId arg since source deploys won't need to prompt for a backend ID, and same thing for webAppName), just to avoid repeating three function calls. So as of now, from a readability perspective I'm leaning on the side of keeping separate doSetup and doSetupSourceDeploy. But as the logic expands in the future, I'm open to revisiting the refactor option. WDYT?

yeah keeping similar code close together, so anyone making changes in one is more likely to not miss making the same changes in the other is my main concern. agree deduping would be a big task right now that can be left for later given timeline.

annajowang · 2025-05-10T01:01:33Z

src/deploy/apphosting/prepare.ts

+    }
+    if (selector.startsWith("apphosting:")) {
+      const backendId = selector.replace("apphosting:", "");
+      if (selector.length > 0) {


Should this be if (backendId.length > 0) ?

annajowang · 2025-05-10T01:11:03Z

src/deploy/apphosting/prepare.ts

+  for (const id of backendIds) {
+    const cfg = backendConfigs.find((cfg) => cfg.backendId === id);
+    if (cfg) {
+      filtered.push(cfg);
+    }
+  }


feel like this could all be replaced by

filtered = backendConfigs.filter(cfg => backendIds.include(cfg.BackendId));

annajowang · 2025-05-10T01:14:23Z

src/deploy/apphosting/prepare.ts

+      if (cfg.alwaysDeployFromSource === false) {
+        continue;
+      }


nit: move this above defining the two constants, since you don't need them.
for readability, delay creating new variables until right before they're first needed.

annajowang · 2025-05-10T01:23:49Z

src/deploy/apphosting/prepare.ts

+        if (!confirmDeploy) {
+          logWarning(`Skipping deployment of backend ${cfg.backendId}`);
+          continue;
+        }


I don't think confirmDeploy could ever be falsey except within the if (!options.force) { if statement. So i think we can keep confirmDeploy and the logwarning all inside the if statement and get rid of this extra else

annajowang · 2025-05-10T01:33:31Z

src/deploy/apphosting/release.ts

+  const multipleRolloutsMessage = `Starting new rollouts for backends ${Array.from(context.backendConfigs.keys()).join(", ")}`;
+  const singleRolloutMessage = `Starting a new rollout for backend ${Array.from(context.backendConfigs.keys()).join(", ")}`;
+  logBullet(
+    `${rollouts.length > 1 ? multipleRolloutsMessage : singleRolloutMessage}; this may take a few minutes. It's safe to exit now.`,
+  );


i think just saying "Starting rollout(s) for backend(s) ...." is fine and does not degrade user experience.

annajowang · 2025-05-12T16:29:54Z

src/deploy/apphosting/util.ts

+  ignore.push(...gitIgnorePatterns);
+
+  if (!projectRoot) {
+    projectRoot = process.cwd();


do we want to let the user known what directory is being used as their projectRoot?
I think it would be a big deal if the user accidentally deploys the wrong set of files, but I don't know if we're preventing / reducing chances of that happening at some point prior to this.

Added some log lines. Users can cancel the upload file stream if they realize they uploaded the wrong files.

annajowang · 2025-05-12T16:38:47Z

src/deploy/apphosting/util.ts

+      crlfDelay: Infinity,
+    });
+    rl.on("line", (line: string) => {
+      if (line.startsWith("#") || line.trim() === "") {


nit: would feel better if line was trimmed before checking if it starts with "#" as well.

annajowang · 2025-05-12T16:40:40Z

src/deploy/apphosting/util.ts

+
+async function parseGitIgnorePatterns(filePath = ".gitignore"): Promise<string[]> {
+  const absoluteFilePath: string = path.resolve(filePath);
+  const lines: string[] = [];


nit: move lines to inside promise?

…e feature (#8545) * add gcp api calls to support deploy from source

Gemini is currently not able to handle arbitrary key/value dictionaries, so I had to change some of the schemas around.

annajowang · 2025-05-12T19:03:59Z

src/deploy/apphosting/util.ts

+  return { projectSourcePath: projectRoot, zippedSourcePath: tmpFile };
+}
+
+async function parseGitIgnorePatterns(filePath = ".gitignore"): Promise<string[]> {


I'd really like us to not write code where we don't have to. but since we're on a time crunch:
could you leave a TODO to say that this could potentially be replaced by
https://www.npmjs.com/package/parse-gitignore

so whoever touches this next can consider doing it.

actually, as bryan said somewhere else, this package essentially does
"fs.ReadFileSync("path").filter(startsWith("#"))"

I'm realizing it's not clear to me why we need 30+ lines of code instead of just

return fs.ReadFileSync(".gitinore").filter( (line) => line.trim().startsWith("#") || line.trim() === ""))

Fair enough. The event listening approach is kind of an old school way to do it. Made the changes

src/deploy/apphosting/util.ts

src/apphosting/backend.ts

src/deploy/apphosting/util.ts

github-project-automation bot added this to [Cloud] Extensions + Functions May 7, 2025

blidd-google requested a review from annajowang May 7, 2025 20:26

annajowang reviewed May 8, 2025

View reviewed changes

annajowang reviewed May 9, 2025

View reviewed changes

annajowang reviewed May 10, 2025

View reviewed changes

blidd-google added 3 commits May 12, 2025 00:39

deploy from local source

944ab1f

minor fixes & use fuzzy search for backends

a0f00ff

add storage.objectViewer role on compute SA

1588252

blidd-google force-pushed the bl-fah-deploy-from-source branch from ce05795 to 1588252 Compare May 12, 2025 04:41

blidd-google added 2 commits May 12, 2025 00:54

respond to first round of feedback

f7be953

second round of feedback

b5519ac

blidd-google mentioned this pull request May 12, 2025

Add GCP API client functions to support App Hosting deploy from source feature #8545

Merged

annajowang reviewed May 12, 2025

View reviewed changes

joehan and others added 6 commits May 12, 2025 14:05

Fix VSCode import error (#8546)

e344fae

Add GCP API client functions to support App Hosting deploy from sourc…

68caf08

…e feature (#8545) * add gcp api calls to support deploy from source

fix(mcp): Make all input schemas valid for Gemini. (#8537)

588b11f

Gemini is currently not able to handle arbitrary key/value dictionaries, so I had to change some of the schemas around.

Merge branch 'master' into bl-fah-deploy-from-source

8cc675f

log source code upload path

219b901

Merge branch 'master' into bl-fah-deploy-from-source

cbe7ffe

annajowang reviewed May 12, 2025

View reviewed changes

blidd-google and others added 6 commits May 12, 2025 16:25

simplify gitignore parsing

908eb34

fix bug not deploying with no --only flag

d771ce8

support negation rules in .gitignore

affa033

clean up readdirrecursive logic

bc0cfbf

Merge branch 'master' into bl-fah-deploy-from-source

41ca746

polish & cleanup

f0eefcc

annajowang reviewed May 13, 2025

View reviewed changes

src/deploy/apphosting/util.ts Outdated Show resolved Hide resolved

blidd-google added 2 commits May 14, 2025 14:43

refactor to use 'ignore' pkg to apply .gitignore rules

f84ff77

fix gitignore unit test

9fe4133

blidd-google mentioned this pull request May 14, 2025

Provision App Hosting compute service account during init flow #8580

Merged

blidd-google and others added 4 commits May 14, 2025 21:40

rework prepare flow and polish copy

d019b57

Merge branch 'master' into bl-fah-deploy-from-source

93db1a6

fix merge issues

e72954e

more polish & cleanup

57b9d4c

Yuangwang reviewed May 15, 2025

View reviewed changes

src/apphosting/backend.ts Show resolved Hide resolved

src/deploy/apphosting/util.ts Show resolved Hide resolved

src/deploy/apphosting/util.ts Outdated Show resolved Hide resolved

Yuangwang and others added 4 commits May 15, 2025 11:00

Merge branch 'master' into bl-fah-deploy-from-source

23c3e2f

skip not found backends with force

d6e37bc

return empty array if git ignore doesn't exist

611dc97

return empty array if no gitignore found

1526d3d

Yuangwang approved these changes May 15, 2025

View reviewed changes

github-project-automation bot moved this to Approved [PR] in [Cloud] Extensions + Functions May 15, 2025

blidd-google added 3 commits May 15, 2025 13:40

Merge branch 'master' into bl-fah-deploy-from-source

a999b7a

Merge branch 'master' into bl-fah-deploy-from-source

09bcac9

Merge branch 'master' into bl-fah-deploy-from-source

df7042d

blidd-google enabled auto-merge (squash) May 15, 2025 19:11

Merge branch 'master' into bl-fah-deploy-from-source

f8cce50

blidd-google merged commit 0486bd9 into master May 15, 2025
50 checks passed

github-project-automation bot moved this from Approved [PR] to Done in [Cloud] Extensions + Functions May 15, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for deploying local source to App Hosting #8516

Add support for deploying local source to App Hosting #8516

blidd-google commented May 7, 2025 •

edited

Loading

annajowang commented May 8, 2025 •

edited

Loading

annajowang left a comment

annajowang May 8, 2025

annajowang May 8, 2025

annajowang May 8, 2025

blidd-google May 12, 2025

annajowang left a comment

annajowang May 9, 2025

annajowang May 9, 2025

blidd-google May 9, 2025

annajowang May 9, 2025 •

edited

Loading

annajowang May 10, 2025

annajowang May 10, 2025

annajowang May 10, 2025

blidd-google May 12, 2025

annajowang May 12, 2025

annajowang May 10, 2025

annajowang May 10, 2025

annajowang May 10, 2025

annajowang May 10, 2025

annajowang May 10, 2025

annajowang May 12, 2025

blidd-google May 12, 2025

annajowang May 12, 2025

annajowang May 12, 2025

annajowang May 12, 2025

annajowang May 12, 2025 •

edited

Loading

blidd-google May 12, 2025

Add support for deploying local source to App Hosting #8516

Add support for deploying local source to App Hosting #8516

Conversation

blidd-google commented May 7, 2025 • edited Loading

annajowang commented May 8, 2025 • edited Loading

annajowang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annajowang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annajowang May 9, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annajowang May 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blidd-google commented May 7, 2025 •

edited

Loading

annajowang commented May 8, 2025 •

edited

Loading

annajowang May 9, 2025 •

edited

Loading

annajowang May 12, 2025 •

edited

Loading