Skip to content
Open
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
30 changes: 28 additions & 2 deletions src/deploy/functions/backend.ts
Original file line number Diff line number Diff line change
Expand Up @@ -170,8 +170,24 @@ export const AllIngressSettings: IngressSettings[] = [
"ALLOW_INTERNAL_ONLY",
"ALLOW_INTERNAL_AND_GCLB",
];
export type MemoryOptions = 128 | 256 | 512 | 1024 | 2048 | 4096 | 8192 | 16384 | 32768;
const allMemoryOptions: MemoryOptions[] = [128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768];
export type MemoryOptions =
| 128
| 256
| 512
| 1024
| 1536
| 2048
| 3072
| 4096
| 8192
| 16384
| 32768;
const allMemoryOptions: MemoryOptions[] = [
128, 256, 512, 1024, 1536, 2048, 3072, 4096, 8192, 16384, 32768,
];
export const GCFV1_MEMORY_OPTIONS: MemoryOptions[] = [
128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768,
];

/**
* Is a given number a valid MemoryOption?
Expand All @@ -180,6 +196,10 @@ export function isValidMemoryOption(mem: unknown): mem is MemoryOptions {
return allMemoryOptions.includes(mem as MemoryOptions);
}

export function isValidGcfv1MemoryOption(mem: unknown): mem is MemoryOptions {
return GCFV1_MEMORY_OPTIONS.includes(mem as MemoryOptions);
}

export function isValidEgressSetting(egress: unknown): egress is VpcEgressSettings {
return egress === "PRIVATE_RANGES_ONLY" || egress === "ALL_TRAFFIC";
}
Expand All @@ -201,7 +221,9 @@ export function memoryOptionDisplayName(option: MemoryOptions): string {
256: "256MB",
512: "512MB",
1024: "1GB",
1536: "1.5GB",
2048: "2GB",
3072: "3GB",
4096: "4GB",
8192: "8GB",
16384: "16GB",
Expand All @@ -222,7 +244,9 @@ export function memoryToGen1Cpu(memory: MemoryOptions): number {
256: 0.1666, // ~1/6
512: 0.3333, // ~1/3
1024: 0.5833, // ~5/7
1536: 0.8,
2048: 1,
3072: 1.5,
4096: 2,
8192: 2,
16384: 4,
Expand All @@ -241,7 +265,9 @@ export function memoryToGen2Cpu(memory: MemoryOptions): number {
256: 1,
512: 1,
1024: 1,
1536: 1,
2048: 1,
3072: 2,
4096: 2,
8192: 2,
16384: 4,
Expand Down
17 changes: 15 additions & 2 deletions src/deploy/functions/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -205,8 +205,21 @@ export interface SecretEnvVar {
projectId: string; // The project containing the Secret
}

export type MemoryOption = 128 | 256 | 512 | 1024 | 2048 | 4096 | 8192 | 16384 | 32768;
const allMemoryOptions: MemoryOption[] = [128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768];
export type MemoryOption =
| 128
| 256
| 512
| 1024
| 1536
| 2048
| 3072
| 4096
| 8192
| 16384
| 32768;
const allMemoryOptions: MemoryOption[] = [
128, 256, 512, 1024, 1536, 2048, 3072, 4096, 8192, 16384, 32768,
];
Comment on lines +208 to +222
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

There's a duplication of memory option definitions here and in src/deploy/functions/backend.ts. To improve maintainability and avoid having to update memory options in multiple places, we should define these constants in a single location.

I suggest we make src/deploy/functions/backend.ts the single source of truth for memory options. You can remove MemoryOption and allMemoryOptions from this file. Additionally, the type is named MemoryOption here, but MemoryOptions in backend.ts. Consolidating them would also resolve this inconsistency.

To fix the error message in toBackend (line 516), you'll need to:

  1. Export allMemoryOptions from src/deploy/functions/backend.ts.
  2. Import allMemoryOptions in this file (src/deploy/functions/build.ts).

This will centralize the memory option definitions and make future updates easier.


// Run is an automatic migration from gcfv2 and is not used on the wire.
export type FunctionsPlatform = Exclude<backend.FunctionsPlatform, "run">;
Expand Down
12 changes: 11 additions & 1 deletion src/deploy/functions/validate.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,16 @@ describe("validate", () => {
expect(() => validate.endpointsAreValid(backend.of(ep))).to.throw(/GCF gen 1/);
});

it("disallows unsupported memory for GCF gen 1", () => {
const ep: backend.Endpoint = {
...ENDPOINT_BASE,
platform: "gcfv1",
availableMemoryMb: 1536,
};

expect(() => validate.endpointsAreValid(backend.of(ep))).to.throw(/only supports/);
});

it("Disallows concurrency for low-CPU gen 2", () => {
const ep: backend.Endpoint = {
...ENDPOINT_BASE,
Expand Down Expand Up @@ -289,7 +299,7 @@ describe("validate", () => {
});
}

for (const mem of [128, 256, 512, 1024, 2048, 4096, 8192, 16384, 32768] as const) {
for (const mem of [128, 256, 512, 1024, 1536, 2048, 3072, 4096, 8192, 16384, 32768] as const) {
it(`allows gcfv2 endpoints with mem ${mem} and no cpu`, () => {
const ep: backend.Endpoint = {
...ENDPOINT_BASE,
Expand Down
15 changes: 15 additions & 0 deletions src/deploy/functions/validate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,21 @@ export function endpointsAreValid(wantBackend: backend.Backend): void {
}

// Our SDK doesn't let people articulate this, but it's theoretically possible in the manifest syntax.
const gcfV1WithUnsupportedMemory = matchingIds(
endpoints,
(endpoint) =>
endpoint.platform === "gcfv1" &&
endpoint.availableMemoryMb !== undefined &&
endpoint.availableMemoryMb !== null &&
!backend.isValidGcfv1MemoryOption(endpoint.availableMemoryMb),
);
if (gcfV1WithUnsupportedMemory.length) {
const msg = `Cannot set availableMemoryMb on the functions ${gcfV1WithUnsupportedMemory} because GCF gen 1 only supports ${backend.GCFV1_MEMORY_OPTIONS.join(
", ",
)} MB`;
throw new FirebaseError(msg);
}

const gcfV1WithConcurrency = matchingIds(
endpoints,
(endpoint) => (endpoint.concurrency || 1) !== 1 && endpoint.platform === "gcfv1",
Expand Down