-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Upgrades functions::list command to utilize Cloud Run API #9425
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
base: master
Are you sure you want to change the base?
Changes from 10 commits
12b9257
0a30722
df189ee
66cf473
3af536c
f6ef8c0
d9fd843
84725cf
6a45457
9cedf12
481d02f
0362303
d4ed0f9
2de426f
7cb75d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| - Upgraded functions::list command to use cloud run api (#9425) | ||
| - Adds 2nd gen Firebase Data Connect triggers to firebase deploy (#9394). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,26 +6,57 @@ | |
| import * as backend from "../deploy/functions/backend"; | ||
| import { logger } from "../logger"; | ||
| import * as Table from "cli-table3"; | ||
| import { listServices, endpointFromService } from "../gcp/runv2"; | ||
|
|
||
| export const command = new Command("functions:list") | ||
| .description("list all deployed functions in your Firebase project") | ||
| .before(requirePermissions, ["cloudfunctions.functions.list"]) | ||
| .before(requirePermissions, ["cloudfunctions.functions.list", "run.services.list"]) | ||
| .action(async (options: Options) => { | ||
| const projectId = needProjectId(options); | ||
| const context = { | ||
| projectId: needProjectId(options), | ||
| projectId, | ||
| } as args.Context; | ||
| const existing = await backend.existingBackend(context); | ||
| const endpointsList = backend.allEndpoints(existing).sort(backend.compareFunctions); | ||
|
|
||
| let v1Endpoints: backend.Endpoint[] = []; | ||
| try { | ||
| const existing = await backend.existingBackend(context); | ||
| v1Endpoints = backend.allEndpoints(existing); | ||
| } catch (err: any) { | ||
| logger.debug(`Failed to list v1 functions:`, err); | ||
| logger.warn( | ||
| `Failed to list v1 functions. Ensure you have the Cloud Functions API enabled and the necessary permissions.`, | ||
| ); | ||
| } | ||
|
|
||
| let v2Endpoints: backend.Endpoint[] = []; | ||
| try { | ||
| const services = await listServices(projectId); | ||
| v2Endpoints = services.map((service) => endpointFromService(service)); | ||
| } catch (err: any) { | ||
| logger.debug(`Failed to list v2 functions:`, err); | ||
| logger.warn( | ||
| `Failed to list v2 functions. Ensure you have the Cloud Run Admin API enabled and the necessary permissions.`, | ||
|
Comment on lines
+31
to
+38
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this entire logic should live inside existingBackend is the abstraction we have that calls GCF v1/v2 API and then transform it into a common representation. We should add support for Run functions there directly (which would benefit this command along with other places where we need to list functions (e.g. during deploy) |
||
| ); | ||
| } | ||
|
|
||
| const endpointsList = [...v1Endpoints, ...v2Endpoints].sort(backend.compareFunctions); | ||
brittanycho marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if (endpointsList.length === 0) { | ||
| logger.info(`No functions found in project ${projectId}.`); | ||
| return []; | ||
| } | ||
|
|
||
| const table = new Table({ | ||
| head: ["Function", "Version", "Trigger", "Location", "Memory", "Runtime"], | ||
| style: { head: ["yellow"] }, | ||
| }); | ||
|
|
||
| for (const endpoint of endpointsList) { | ||
| const trigger = backend.endpointTriggerType(endpoint); | ||
| const availableMemoryMb = endpoint.availableMemoryMb || "---"; | ||
| const entry = [ | ||
| endpoint.id, | ||
| endpoint.platform === "gcfv2" ? "v2" : "v1", | ||
| endpoint.platform === "gcfv2" ? "v2" : endpoint.platform === "run" ? "run" : "v1", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmm maybe we can use a small map, like type PLATFORM_DISPLAY_NAME = "v1" | "v2" | "run"; // Returns 'v2', 'run', or falls back to 'v1' |
||
| trigger, | ||
| endpoint.region, | ||
| availableMemoryMb, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,12 @@ | ||
| import { expect } from "chai"; | ||
| import * as sinon from "sinon"; | ||
|
|
||
| import * as runv2 from "./runv2"; | ||
| import * as backend from "../deploy/functions/backend"; | ||
| import { latest } from "../deploy/functions/runtimes/supported"; | ||
| import { CODEBASE_LABEL } from "../functions/constants"; | ||
| import { Client } from "../apiv2"; | ||
| import { FirebaseError } from "../error"; | ||
|
|
||
| describe("runv2", () => { | ||
| const PROJECT_ID = "project-id"; | ||
|
|
@@ -95,13 +98,13 @@ | |
| httpsTrigger: {}, | ||
| environmentVariables: { FOO: "bar" }, | ||
| }; | ||
| const expectedServiceInput = JSON.parse( | ||
| JSON.stringify({ | ||
| ...BASE_RUN_SERVICE, | ||
| name: `projects/${PROJECT_ID}/locations/${LOCATION}/services/${FUNCTION_ID.toLowerCase()}`, | ||
| }), | ||
| ); | ||
| expectedServiceInput.template.containers[0].env.unshift({ name: "FOO", value: "bar" }); | ||
|
Check warning on line 107 in src/gcp/runv2.spec.ts
|
||
|
|
||
| expect(runv2.serviceFromEndpoint(endpoint, IMAGE_URI)).to.deep.equal(expectedServiceInput); | ||
| }); | ||
|
|
@@ -114,13 +117,13 @@ | |
| { key: "MY_SECRET", secret: "secret-name", projectId: PROJECT_ID, version: "1" }, | ||
| ], | ||
| }; | ||
| const expectedServiceInput = JSON.parse( | ||
| JSON.stringify({ | ||
| ...BASE_RUN_SERVICE, | ||
| name: `projects/${PROJECT_ID}/locations/${LOCATION}/services/${FUNCTION_ID.toLowerCase()}`, | ||
| }), | ||
| ); | ||
| expectedServiceInput.template.containers[0].env.unshift({ | ||
|
Check warning on line 126 in src/gcp/runv2.spec.ts
|
||
| name: "MY_SECRET", | ||
| valueSource: { secretKeyRef: { secret: "secret-name", version: "1" } }, | ||
| }); | ||
|
|
@@ -134,13 +137,13 @@ | |
| minInstances: 1, | ||
| maxInstances: 10, | ||
| }; | ||
| const expectedServiceInput = JSON.parse( | ||
| JSON.stringify({ | ||
| ...BASE_RUN_SERVICE, | ||
| name: `projects/${PROJECT_ID}/locations/${LOCATION}/services/${FUNCTION_ID.toLowerCase()}`, | ||
| }), | ||
| ); | ||
| expectedServiceInput.scaling = { | ||
| minInstanceCount: 1, | ||
| maxInstanceCount: 10, | ||
| }; | ||
|
|
@@ -201,13 +204,11 @@ | |
| const service: Omit<runv2.Service, runv2.ServiceOutputFields> = { | ||
| ...BASE_RUN_SERVICE, | ||
| name: `projects/${PROJECT_ID}/locations/${LOCATION}/services/${SERVICE_ID}`, | ||
| labels: { | ||
| [runv2.RUNTIME_LABEL]: latest("nodejs"), | ||
| }, | ||
| annotations: { | ||
| ...BASE_RUN_SERVICE.annotations, | ||
| [runv2.FUNCTION_ID_ANNOTATION]: FUNCTION_ID, // Using FUNCTION_ID_ANNOTATION as primary source for id | ||
| [runv2.FUNCTION_TARGET_ANNOTATION]: "customEntryPoint", | ||
| [runv2.TRIGGER_TYPE_ANNOTATION]: "HTTP_TRIGGER", | ||
| }, | ||
| template: { | ||
| containers: [ | ||
|
|
@@ -239,6 +240,7 @@ | |
| httpsTrigger: {}, | ||
| labels: { | ||
| [runv2.RUNTIME_LABEL]: latest("nodejs"), | ||
| [runv2.CLIENT_NAME_LABEL]: "firebase-functions", | ||
| }, | ||
| environmentVariables: {}, | ||
| secretEnvironmentVariables: [], | ||
|
|
@@ -259,6 +261,7 @@ | |
| ...BASE_RUN_SERVICE.annotations, | ||
| [runv2.FUNCTION_ID_ANNOTATION]: FUNCTION_ID, // Using FUNCTION_ID_ANNOTATION as primary source for id | ||
| [runv2.FUNCTION_TARGET_ANNOTATION]: "customEntryPoint", | ||
| [runv2.TRIGGER_TYPE_ANNOTATION]: "HTTP_TRIGGER", | ||
| }, | ||
| template: { | ||
| containers: [ | ||
|
|
@@ -442,7 +445,7 @@ | |
| entryPoint: SERVICE_ID, // No FUNCTION_TARGET_ANNOTATION | ||
| availableMemoryMb: 128, | ||
| cpu: 0.5, | ||
| httpsTrigger: {}, | ||
| eventTrigger: { eventType: "unknown", retry: false }, | ||
| labels: {}, | ||
| environmentVariables: {}, | ||
| secretEnvironmentVariables: [], | ||
|
|
@@ -452,4 +455,65 @@ | |
| expect(runv2.endpointFromService(service)).to.deep.equal(expectedEndpoint); | ||
| }); | ||
| }); | ||
|
|
||
| describe("listServices", () => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like its missing test on whether filtering by label works correctly |
||
| let sandbox: sinon.SinonSandbox; | ||
| let getStub: sinon.SinonStub; | ||
|
|
||
| beforeEach(() => { | ||
| sandbox = sinon.createSandbox(); | ||
| getStub = sandbox.stub(Client.prototype, "get"); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| sandbox.restore(); | ||
| }); | ||
|
|
||
| it("should return a list of services", async () => { | ||
| const mockServices = [{ name: "service1" }, { name: "service2" }]; | ||
| getStub.resolves({ status: 200, body: { services: mockServices } }); | ||
|
|
||
| const services = await runv2.listServices(PROJECT_ID); | ||
|
|
||
| expect(services).to.deep.equal(mockServices); | ||
| expect(getStub).to.have.been.calledOnceWithExactly( | ||
| `/projects/${PROJECT_ID}/locations/-/services`, | ||
| { queryParams: {} }, | ||
| ); | ||
| }); | ||
|
|
||
| it("should handle pagination", async () => { | ||
| const mockServices1 = [{ name: "service1" }]; | ||
| const mockServices2 = [{ name: "service2" }]; | ||
| getStub | ||
| .onFirstCall() | ||
| .resolves({ status: 200, body: { services: mockServices1, nextPageToken: "nextPage" } }); | ||
| getStub.onSecondCall().resolves({ status: 200, body: { services: mockServices2 } }); | ||
|
|
||
| const services = await runv2.listServices(PROJECT_ID); | ||
|
|
||
| expect(services).to.deep.equal([...mockServices1, ...mockServices2]); | ||
| expect(getStub).to.have.been.calledTwice; | ||
| expect(getStub.firstCall).to.have.been.calledWithExactly( | ||
| `/projects/${PROJECT_ID}/locations/-/services`, | ||
| { queryParams: {} }, | ||
| ); | ||
| expect(getStub.secondCall).to.have.been.calledWithExactly( | ||
| `/projects/${PROJECT_ID}/locations/-/services`, | ||
| { queryParams: { pageToken: "nextPage" } }, | ||
| ); | ||
| }); | ||
|
|
||
| it("should throw an error if the API call fails", async () => { | ||
| getStub.resolves({ status: 500, body: "Internal Server Error" }); | ||
|
|
||
| try { | ||
| await runv2.listServices(PROJECT_ID); | ||
| expect.fail("Should have thrown an error"); | ||
| } catch (err: any) { | ||
| expect(err).to.be.instanceOf(FirebaseError); | ||
| expect(err.message).to.contain('Failed to list services: 500 "Internal Server Error"'); | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like this would list all Run services, which is not what we want.