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.

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

Improve FAH create & delete #6726

Merged
merged 7 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 24 additions & 7 deletions src/gcp/apphosting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
| "error"
| "image"
| "sourceRef"
| "buildLogUri"
| "buildLogsUri"
| "reconciling"
| "uuid"
| "etag"
Expand Down Expand Up @@ -197,10 +197,20 @@
// end oneof trigger
stages?: RolloutStage[];
disabled?: boolean;

// TODO: This will be undefined if disabled is not true, right?
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it should be.

disabledTime: string;
}

export type RolloutPolicyOutputOnlyFields = "disabledtime";
export type RolloutPolicyOutputOnlyFields = "disabledTime";

export type RolloutPolicyInput = Omit<RolloutPolicy, RolloutPolicyOutputOnlyFields | "stages"> & {
stages: Omit<RolloutStage, "startTime" | "endTime">[];
};

export type TrafficInput = Omit<Traffic, TrafficOutputOnlyFields | "rolloutPolicy"> & {
rolloutPolicy: RolloutPolicyInput;
};

export type RolloutProgression =
| "PROGRESSION_UNSPECIFIED"
Expand Down Expand Up @@ -238,7 +248,7 @@
done: boolean;
// oneof result
error?: Status;
response?: any;

Check warning on line 251 in src/gcp/apphosting.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
// end oneof result
}

Expand Down Expand Up @@ -304,8 +314,8 @@
location: string,
backendId: string,
): Promise<Operation> {
const name = `projects/${projectId}/locations/${location}/backends/${backendId}?force=true`;
const res = await client.delete<Operation>(name);
const name = `projects/${projectId}/locations/${location}/backends/${backendId}`;
const res = await client.delete<Operation>(name, { queryParams: { force: "true" } });

return res.body;
}
Expand Down Expand Up @@ -393,14 +403,21 @@
projectId: string,
location: string,
backendId: string,
traffic: Omit<Traffic, TrafficOutputOnlyFields | "name">,
traffic: Omit<TrafficInput, "name">,
): Promise<Operation> {
const fieldMasks = proto.fieldMasks(traffic);
// BUG(b/322891558): setting deep fields on rolloutPolicy doesn't work for some
// reason. Create a copy without deep fields to force the updateMask to be
// correct.
const trafficCopy = { ...traffic };
if ("rolloutPolicy" in traffic) {
trafficCopy.rolloutPolicy = {} as any;

Check warning on line 413 in src/gcp/apphosting.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe assignment of an `any` value

Check warning on line 413 in src/gcp/apphosting.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
}
const fieldMasks = proto.fieldMasks(trafficCopy);
const queryParams = {
updateMask: fieldMasks.join(","),
};
const name = `projects/${projectId}/locations/${location}/backends/${backendId}/traffic`;
const res = await client.patch<Omit<Traffic, TrafficOutputOnlyFields>, Operation>(
const res = await client.patch<TrafficInput, Operation>(
name,
{ ...traffic, name },
{
Expand Down Expand Up @@ -439,7 +456,7 @@
/**
* Ensure that Frameworks API is enabled on the project.
*/
export async function ensureApiEnabled(options: any): Promise<void> {

Check warning on line 459 in src/gcp/apphosting.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
const projectId = needProjectId(options);

Check warning on line 460 in src/gcp/apphosting.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe argument of type `any` assigned to a parameter of type `{ projectId?: string | undefined; project?: string | undefined; rc?: RC | undefined; }`
return await ensure(projectId, API_HOST, "frameworks", true);
}
43 changes: 35 additions & 8 deletions src/init/features/apphosting/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import { promptOnce } from "../../../prompt";
import { DEFAULT_REGION } from "./constants";
import { ensure } from "../../../ensureApiEnabled";
import * as deploymentTool from "../../../deploymentTool";

const apphostingPollerOptions: Omit<poller.OperationPollerOptions, "operationResourceName"> = {
apiOrigin: apphostingOrigin,
Expand All @@ -28,7 +29,7 @@
/**
* Set up a new App Hosting backend.
*/
export async function doSetup(setup: any, projectId: string): Promise<void> {

Check warning on line 32 in src/init/features/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unexpected any. Specify a different type
await Promise.all([
ensure(projectId, "cloudbuild.googleapis.com", "apphosting", true),
ensure(projectId, "secretmanager.googleapis.com", "apphosting", true),
Expand All @@ -38,10 +39,10 @@

const allowedLocations = (await apphosting.listLocations(projectId)).map((loc) => loc.locationId);

if (setup.location) {

Check warning on line 42 in src/init/features/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .location on an `any` value
if (!allowedLocations.includes(setup.location)) {

Check warning on line 43 in src/init/features/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe argument of type `any` assigned to a parameter of type `string`

Check warning on line 43 in src/init/features/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Unsafe member access .location on an `any` value
throw new FirebaseError(
`Invalid location ${setup.location}. Valid choices are ${allowedLocations.join(", ")}`,

Check warning on line 45 in src/init/features/apphosting/index.ts

View workflow job for this annotation

GitHub Actions / lint (20)

Invalid type "any" of template literal expression
);
}
}
Expand Down Expand Up @@ -76,6 +77,33 @@

const backend = await onboardBackend(projectId, location, backendId);

// TODO: Once tag patterns are implemented, prompt which method the user
// prefers. We could reduce the nubmer of questions asked by letting people
// enter tag:<pattern>?
const branch = await promptOnce({
name: "branch",
type: "input",
default: "main",
message: "Pick a branch for continuous deployment",
});
const traffic: Omit<apphosting.TrafficInput, "name"> = {
rolloutPolicy: {
codebaseBranch: branch,
stages: [
{
progression: "IMMEDIATE",
targetPercent: 100,
},
],
},
};
const op = await apphosting.updateTraffic(projectId, location, backendId, traffic);
await poller.pollOperation<apphosting.Traffic>({
...apphostingPollerOptions,
pollerName: `updateTraffic-${projectId}-${location}-${backendId}`,
operationResourceName: op.name,
});

const confirmRollout = await promptOnce({
type: "confirm",
name: "rollout",
Expand All @@ -89,13 +117,6 @@
return;
}

const branch = await promptOnce({
name: "branch",
type: "input",
default: "main",
message: "Which branch do you want to deploy?",
});

const { build } = await onboardRollout(projectId, location, backendId, {
source: {
codebase: {
Expand All @@ -105,6 +126,12 @@
});

if (build.state !== "READY") {
if (!build.buildLogsUri) {
throw new FirebaseError(
"Failed to build your app, but failed to get build logs as well. " +
"This is an internal error and should be reported",
);
}
throw new FirebaseError(
`Failed to build your app. Please inspect the build logs at ${build.buildLogsUri}.`,
{ children: [build.error] },
Expand Down Expand Up @@ -137,7 +164,7 @@
repository: `${cloudBuildConnRepo.name}`,
rootDirectory: "/",
},
labels: {},
labels: deploymentTool.labels(),
};
}

Expand Down
3 changes: 2 additions & 1 deletion src/test/init/apphosting/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import * as poller from "../../../operation-poller";
import * as prompt from "../../../prompt";
import { createBackend, onboardBackend } from "../../../init/features/apphosting/index";
import { FirebaseError } from "../../../error";
import * as deploymentTool from "../../../deploymentTool";

describe("operationsConverter", () => {
const sandbox: sinon.SinonSandbox = sinon.createSandbox();
Expand Down Expand Up @@ -66,7 +67,7 @@ describe("operationsConverter", () => {
repository: cloudBuildConnRepo.name,
rootDirectory: "/",
},
labels: {},
labels: deploymentTool.labels(),
};

it("should createBackend", async () => {
Expand Down