Skip to content

Prompt for which functions to update during functions:secrets:set #10843

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.

Sign up for GitHub

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

Open
Berlioz wants to merge 1 commit into
base: main
Choose a base branch
from
+30 −2
Open

Prompt for which functions to update during functions:secrets:set #10843

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 30 additions & 2 deletions src/commands/functions-secrets-set.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,11 @@ import * as tty from "tty";
import { logger } from "../logger";
import { FirebaseError } from "../error";
import { ensureValidKey, ensureSecret, validateJsonSecret } from "../functions/secrets";
import { isFirebaseManaged } from "../deploymentTool";
import { Command } from "../command";
import { requirePermissions } from "../requirePermissions";
import { Options } from "../options";
import { confirm } from "../prompt";
import { confirm, checkbox, Choice } from "../prompt";
import { logBullet, logSuccess, logWarning, readSecretValue } from "../utils";
import { needProjectId, needProjectNumber } from "../projectUtils";
import {
Expand Down Expand Up @@ -92,8 +93,9 @@ export const command = new Command("functions:secrets:set <KEY>")
}

let haveBackend = await backend.existingBackend({ projectId } as args.Context);
const endpointsToUpdate = backend
let endpointsToUpdate = backend
.allEndpoints(haveBackend)
.filter((e) => isFirebaseManaged(e.labels ?? []))

@gemini-code-assist gemini-code-assist Bot Jul 24, 2026

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

Passing an empty array [] as a fallback for e.labels is a type mismatch because isFirebaseManaged expects an object of type { [key: string]: any }. Under strict TypeScript compilation, this will cause a type error. Use an empty object {} instead.

Suggested change
.filter((e) => isFirebaseManaged(e.labels ?? []))
.filter((e) => isFirebaseManaged(e.labels ?? {}))
References
  1. TypeScript: Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. Use strict null checks and handle undefined/null explicitly. (link)

@inlined inlined Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

True bug. Labels are not hashtags, they're key value pairs

.filter((e) => secrets.inUse({ projectId, projectNumber }, secret, e));

if (endpointsToUpdate.length === 0) {
Expand All @@ -120,6 +122,32 @@ export const command = new Command("functions:secrets:set <KEY>")
return;
}

if (endpointsToUpdate.length > 1) {

@inlined inlined Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This whole if + indentation is unnecessary since you early exited if length === 0 and the value cannot be negative.

const choices = endpointsToUpdate.map((e): Choice<string> => {
const currentVersion = secrets.getSecretVersions(e)[secret.name];
return {
name: `${e.id}(${e.region}) - current secret version: ${currentVersion ?? "unknown"}`,
value: e.id,
checked: true,
};
});
const selectedEndpointIds = await checkbox<string>({
message:
"Which functions do you want to re-deploy?" +
"Press Space to select functions, then Enter to confirm your choices.",
choices: choices,
});
endpointsToUpdate = endpointsToUpdate.filter((e) => selectedEndpointIds.includes(e.id));
}
Comment on lines +125 to +141

@gemini-code-assist gemini-code-assist Bot Jul 24, 2026

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.

high

Using e.id as the choice value in the checkbox prompt is problematic because function IDs are not guaranteed to be unique across different regions (e.g., a function named foo can be deployed in both us-central1 and us-east1). If a user deselects one of them, both will still be kept in endpointsToUpdate because selectedEndpointIds.includes(e.id) will match both. Additionally, there is a missing space in the prompt message concatenation. Using the array index as the choice value resolves this issue cleanly.

Suggested change
if (endpointsToUpdate.length > 1) {
const choices = endpointsToUpdate.map((e): Choice<string> => {
const currentVersion = secrets.getSecretVersions(e)[secret.name];
return {
name: `${e.id}(${e.region}) - current secret version: ${currentVersion ?? "unknown"}`,
value: e.id,
checked: true,
};
});
const selectedEndpointIds = await checkbox<string>({
message:
"Which functions do you want to re-deploy?" +
"Press Space to select functions, then Enter to confirm your choices.",
choices: choices,
});
endpointsToUpdate = endpointsToUpdate.filter((e) => selectedEndpointIds.includes(e.id));
}
if (endpointsToUpdate.length > 1) {
const choices = endpointsToUpdate.map((e, idx): Choice<number> => {
const currentVersion = secrets.getSecretVersions(e)[secret.name];
return {
name: e.id + "(" + e.region + ") - current secret version: " + (currentVersion ?? "unknown"),
value: idx,
checked: true,
};
});
const selectedIndices = await checkbox<number>({
message:
"Which functions do you want to re-deploy? " +
"Press Space to select functions, then Enter to confirm your choices.",
choices: choices,
});
endpointsToUpdate = endpointsToUpdate.filter((_, idx) => selectedIndices.includes(idx));
}

@inlined inlined Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a fair critique only because the UI allowed you to select a function per region. Given that all functions with the same ID share the same source code (unless they're in different codebases for some reason???) I'm OK with just making this selecting ID irrespective of region. Or you can do Gemini's suggestion much easier by just making it a Choice, setting value to e, and you no longer need to filter.

if (endpointsToUpdate.length === 0) {
logBullet(`No functions confirmed for automatic re-deployment.`);
logBullet(
"Please deploy your functions for the change to take effect by running:\n\t" +
clc.bold("firebase deploy --only functions"),
);
return;
}

const updateOps = endpointsToUpdate.map(async (e) => {
logBullet(`Updating function ${e.id}(${e.region})...`);
const updated = await secrets.updateEndpointSecret(
Expand Down
Loading