Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/apphosting/backend.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
} from "./backend";
import * as deploymentTool from "../deploymentTool";
import { FirebaseError } from "../error";
import * as experiments from "../experiments";

describe("apphosting setup functions", () => {
const projectId = "projectId";
Expand Down Expand Up @@ -67,7 +66,6 @@ describe("apphosting setup functions", () => {
testResourceIamPermissionsStub = sinon
.stub(iam, "testResourceIamPermissions")
.throws("Unexpected testResourceIamPermissions call");
sinon.stub(experiments, "isEnabled").returns(false).withArgs("abiu").returns(true);
});

afterEach(() => {
Expand Down
7 changes: 2 additions & 5 deletions src/apphosting/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@
import fetch from "node-fetch";
import { orchestrateRollout } from "./rollout";
import * as fuzzy from "fuzzy";
import { isEnabled } from "../experiments";

import { resolveRuntime } from "./prompts";

const DEFAULT_COMPUTE_SERVICE_ACCOUNT_NAME = "firebase-app-hosting-compute";
Expand All @@ -51,7 +51,7 @@
// SSL.
const maybeNodeError = err as { cause: { code: string }; code: string };
if (
/HANDSHAKE_FAILURE/.test(maybeNodeError?.cause?.code) ||

Check warning on line 54 in src/apphosting/backend.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Use `String#includes()` method with a string instead
"EPROTO" === maybeNodeError?.code
) {
return false;
Expand Down Expand Up @@ -214,7 +214,7 @@
projectId: string,
backendId: string,
nonInteractive: boolean,
rootDir: string = "/",

Check warning on line 217 in src/apphosting/backend.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Type string trivially inferred from a string literal, remove type annotation
): Promise<{ backend: Backend; location: string }> {
const location = await promptLocation(
projectId,
Expand Down Expand Up @@ -334,7 +334,7 @@
* Prompts the user for a backend id and verifies that it doesn't match a pre-existing backend.
*/
export async function promptNewBackendId(projectId: string, location: string): Promise<string> {
while (true) {

Check warning on line 337 in src/apphosting/backend.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Unexpected constant condition
const backendId = await input({
default: "my-web-app",
message: "Provide a name for your backend [1-30 characters]",
Expand Down Expand Up @@ -387,10 +387,7 @@
appId: webAppId,
};

// this is to be extra careful that we do not set the ABIU fields if the experiment is disabled
if (isEnabled("abiu")) {
backendReqBody.runtime = { value: runtime ?? "" };
}
backendReqBody.runtime = { value: runtime ?? "" };

async function createBackendAndPoll(): Promise<apphosting.Backend> {
const op = await apphosting.createBackend(projectId, location, backendReqBody, backendId);
Expand Down Expand Up @@ -649,7 +646,7 @@
message: locationDisambugationPrompt,
choices: [...backendsByLocation.keys()],
});
return backendsByLocation.get(location)!;

Check warning on line 649 in src/apphosting/backend.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Forbidden non-null assertion
}

/**
Expand Down
11 changes: 0 additions & 11 deletions src/apphosting/prompts.spec.ts
Original file line number Diff line number Diff line change
@@ -1,20 +1,17 @@
import * as sinon from "sinon";
import { expect } from "chai";
import * as prompts from "./prompts";
import * as experiments from "../experiments";
import * as apphosting from "../gcp/apphosting";
import * as promptImport from "../prompt";

describe("prompts", () => {
const projectId = "projectId";
const location = "us-central1";

let isEnabledStub: sinon.SinonStub;
let listSupportedRuntimesStub: sinon.SinonStub;
let selectStub: sinon.SinonStub;

beforeEach(() => {
isEnabledStub = sinon.stub(experiments, "isEnabled");
listSupportedRuntimesStub = sinon.stub(apphosting, "listSupportedRuntimes");
selectStub = sinon.stub(promptImport, "select");
});
Expand All @@ -29,20 +26,12 @@ describe("prompts", () => {
expect(result).to.equal("nodejs22");
});

it("should return undefined if abiu experiment is disabled", async () => {
isEnabledStub.withArgs("abiu").returns(false);
const result = await prompts.resolveRuntime(projectId, location, false);
expect(result).to.be.undefined;
});

it("should return DEFAULT_RUNTIME in non-interactive mode", async () => {
isEnabledStub.withArgs("abiu").returns(true);
const result = await prompts.resolveRuntime(projectId, location, true);
expect(result).to.equal(prompts.DEFAULT_RUNTIME);
});

it("should call promptRuntime and return selected runtime in interactive mode", async () => {
isEnabledStub.withArgs("abiu").returns(true);
listSupportedRuntimesStub.resolves([
{ runtimeId: "nodejs22", automaticBaseImageUpdatesSupported: true },
]);
Expand Down
4 changes: 0 additions & 4 deletions src/apphosting/prompts.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { select, Choice } from "../prompt";
import { logWarning, logBullet } from "../utils";
import * as apphosting from "../gcp/apphosting";
import { isEnabled } from "../experiments";

export const DEFAULT_RUNTIME = "nodejs";

Expand Down Expand Up @@ -49,7 +48,7 @@
/**
* Resolves the runtime for the backend.
* Checks if the 'abiu' experiment is enabled and prompts the user if interactive.
* @param runtimeOption The runtime specified in command arguments (--runtime) if any

Check warning on line 51 in src/apphosting/prompts.ts

View workflow job for this annotation

GitHub Actions / lint (24)

Expected @param names to be "projectId, location, nonInteractive, runtimeOption". Got "runtimeOption"
*/
export async function resolveRuntime(
projectId: string,
Expand All @@ -60,9 +59,6 @@
if (runtimeOption !== undefined) {
return runtimeOption;
}
if (!isEnabled("abiu")) {
return undefined;
}
if (nonInteractive) {
return DEFAULT_RUNTIME;
}
Expand Down
25 changes: 1 addition & 24 deletions src/commands/apphosting-backends-create.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,43 +3,23 @@ import { expect } from "chai";
import { Command } from "../command";
import { command as apphostingBackendsCreate } from "./apphosting-backends-create";
import * as backend from "../apphosting/backend";
import * as experiments from "../experiments";
import { FirebaseError } from "../error";

describe("apphosting:backends:create", () => {
const PROJECT_ID = "test-project";
let command: Command;
let isEnabledStub: sinon.SinonStub;
let doSetupStub: sinon.SinonStub;

beforeEach(() => {
command = apphostingBackendsCreate;
(command as unknown as { befores: unknown[] }).befores = []; // Bypass pre-action hooks for unit testing action
isEnabledStub = sinon.stub(experiments, "isEnabled");
doSetupStub = sinon.stub(backend, "doSetup").resolves();
});

afterEach(() => {
sinon.restore();
});

it("should fail if runtime flag is used without experiment enabled", async () => {
isEnabledStub.returns(false);
const options = {
project: PROJECT_ID,
runtime: "nodejs22",
backend: "test-backend",
primaryRegion: "us-central1",
};

await expect(command.runner()(options)).to.be.rejectedWith(
FirebaseError,
/The --runtime flag is only available when the 'abiu' experiment is enabled/,
);
});

it("should default runtime to undefined when experiment is on and no flag provided", async () => {
isEnabledStub.returns(true);
it("should default runtime to undefined when no flag provided", async () => {
const options = {
project: PROJECT_ID,
nonInteractive: true,
Expand All @@ -63,7 +43,6 @@ describe("apphosting:backends:create", () => {
});

it("should pass explicit empty runtime", async () => {
isEnabledStub.returns(true);
const options = {
project: PROJECT_ID,
nonInteractive: true,
Expand All @@ -88,7 +67,6 @@ describe("apphosting:backends:create", () => {
});

it("should pass explicit runtime", async () => {
isEnabledStub.returns(true);
const options = {
project: PROJECT_ID,
nonInteractive: true,
Expand All @@ -113,7 +91,6 @@ describe("apphosting:backends:create", () => {
});

it("should default runtime if flag is present without value (boolean true)", async () => {
isEnabledStub.returns(true);
const options = {
project: PROJECT_ID,
nonInteractive: true,
Expand Down
24 changes: 7 additions & 17 deletions src/commands/apphosting-backends-create.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { needProjectId } from "../projectUtils";
import { requireAuth } from "../requireAuth";
import { doSetup } from "../apphosting/backend";
import { ensureApiEnabled } from "../gcp/apphosting";
import * as experiments from "../experiments";

import { APPHOSTING_TOS_ID } from "../gcp/firedata";
import { requireTosAcceptance } from "../requireTosAcceptance";

Expand All @@ -29,13 +29,10 @@ export const command = new Command("apphosting:backends:create")
"specify the primary region for the backend. Required with --non-interactive.",
)
.option("--root-dir <rootDir>", "specify the root directory for the backend.");
const abiuEnabled = experiments.isEnabled("abiu");
if (abiuEnabled) {
command.option(
"--runtime [runtime]",
"specify the runtime for the backend (e.g., nodejs, nodejs22)",
);
}
command.option(
"--runtime [runtime]",
"specify the runtime for the backend (e.g., nodejs, nodejs22)",
);

command
.before(requireAuth)
Expand All @@ -47,16 +44,9 @@ command
throw new FirebaseError(`--non-interactive option requires --backend and --primary-region`);
}

const abiuAllowed = experiments.isEnabled("abiu");
if (!abiuAllowed && options.runtime) {
throw new FirebaseError(
"The --runtime flag is only available when the 'abiu' experiment is enabled. To enable it, run 'firebase experiments:enable abiu'.",
);
}
// When ABIU is allowed but the user doesn't provide a runtime string, we let doSetup handle it.
// When the user doesn't provide a runtime string, we let doSetup handle it.
// We strictly check for string type to avoid boolean true (flag present without value) causing issues.
const runtime =
abiuAllowed && typeof options.runtime === "string" ? options.runtime : undefined;
const runtime = typeof options.runtime === "string" ? options.runtime : undefined;

return doSetup(
projectId,
Expand Down
7 changes: 0 additions & 7 deletions src/commands/apphosting-backends-list.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,20 @@ import * as sinon from "sinon";
import { expect } from "chai";
import { logger } from "../logger";
import { printBackendsTable } from "./apphosting-backends-list";
import * as experiments from "../experiments";
import * as apphosting from "../gcp/apphosting";

describe("apphosting:backends:list printBackendsTable", () => {
let loggerStub: sinon.SinonStub;
let isEnabledStub: sinon.SinonStub;

beforeEach(() => {
loggerStub = sinon.stub(logger, "info");
isEnabledStub = sinon.stub(experiments, "isEnabled");
});

afterEach(() => {
sinon.restore();
});

it("should display Disabled if automaticBaseImageUpdatesDisabled is true", () => {
isEnabledStub.withArgs("abiu").returns(true);
const mockBackend: Partial<apphosting.Backend> = {
name: "projects/test-project/locations/us-central1/backends/test-backend",
uri: "https://test-backend.app",
Expand All @@ -35,7 +31,6 @@ describe("apphosting:backends:list printBackendsTable", () => {
});

it("should display Enabled if automaticBaseImageUpdatesDisabled is false", () => {
isEnabledStub.withArgs("abiu").returns(true);
const mockBackend: Partial<apphosting.Backend> = {
name: "projects/test-project/locations/us-central1/backends/test-backend",
uri: "https://test-backend.app",
Expand All @@ -51,7 +46,6 @@ describe("apphosting:backends:list printBackendsTable", () => {
});

it("should fallback to Disabled if automaticBaseImageUpdatesDisabled is missing and runtime is legacy", () => {
isEnabledStub.withArgs("abiu").returns(true);
const mockBackend: Partial<apphosting.Backend> = {
name: "projects/test-project/locations/us-central1/backends/test-backend",
uri: "https://test-backend.app",
Expand All @@ -66,7 +60,6 @@ describe("apphosting:backends:list printBackendsTable", () => {
});

it("should fallback to Enabled if automaticBaseImageUpdatesDisabled is missing and runtime is new", () => {
isEnabledStub.withArgs("abiu").returns(true);
const mockBackend: Partial<apphosting.Backend> = {
name: "projects/test-project/locations/us-central1/backends/test-backend",
uri: "https://test-backend.app",
Expand Down
29 changes: 11 additions & 18 deletions src/commands/apphosting-backends-list.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { needProjectId } from "../projectUtils";
import { requireAuth } from "../requireAuth";
import { Options } from "../options";
import * as apphosting from "../gcp/apphosting";
import { isEnabled } from "../experiments";

import * as Table from "cli-table3";

export const command = new Command("apphosting:backends:list")
Expand Down Expand Up @@ -35,12 +35,7 @@ export const command = new Command("apphosting:backends:list")
* Prints a table given a list of backends
*/
export function printBackendsTable(backends: apphosting.Backend[]): void {
const abiuEnabled = isEnabled("abiu");
const head = ["Backend", "Repository", "URL", "Primary Region"];
if (abiuEnabled) {
head.push("ABIU");
head.push("Runtime");
}
const head = ["Backend", "Repository", "URL", "Primary Region", "ABIU", "Runtime"];
head.push("Updated Date");

const table = new Table({
Expand All @@ -57,18 +52,16 @@ export function printBackendsTable(backends: apphosting.Backend[]): void {
backend.uri.startsWith("https:") ? backend.uri : "https://" + backend.uri,
location,
];
if (abiuEnabled) {
let abiuStatus = "N/A";
const runtimeValue = backend.runtime?.value ?? "";
// We know these runtimes do not support ABIU
if (runtimeValue === "" || runtimeValue === "nodejs") {
abiuStatus = "Disabled";
} else {
abiuStatus = backend.automaticBaseImageUpdatesDisabled ? "Disabled" : "Enabled";
}
row.push(abiuStatus);
row.push(backend.runtime?.value ?? "N/A");
let abiuStatus = "N/A";
const runtimeValue = backend.runtime?.value ?? "";
// We know these runtimes do not support ABIU
if (runtimeValue === "" || runtimeValue === "nodejs") {
abiuStatus = "Disabled";
} else {
abiuStatus = backend.automaticBaseImageUpdatesDisabled ? "Disabled" : "Enabled";
}
row.push(abiuStatus);
row.push(backend.runtime?.value ?? "N/A");

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using the nullish coalescing operator (??) here means that if backend.runtime?.value is an empty string (""), it will be pushed as "" instead of falling back to "N/A". Since an empty string is falsy, using the logical OR operator (||) will correctly fall back to "N/A" when the runtime value is empty, improving the table's readability.

Suggested change
row.push(backend.runtime?.value ?? "N/A");
row.push(backend.runtime?.value || "N/A");

row.push(datetimeString(new Date(backend.updateTime)));
table.push(row);
}
Expand Down
7 changes: 0 additions & 7 deletions src/experiments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -149,13 +149,6 @@ export const ALL_EXPERIMENTS = experiments({
public: false,
},

abiu: {
shortDescription:
"Enable Automatic Base Image Updates (ABIU) and runtime selection for App Hosting",
default: true,
public: true,
},

// TODO(joehanley): Delete this once weve scrubbed all references to experiment from docs.
dataconnect: {
shortDescription: "Deprecated. Previosuly, enabled SQL Connect related features.",
Expand Down
Loading