Skip to content

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

Open
Berlioz wants to merge 1 commit into
mainfrom
vsfan_ext_secrets_set
Open

Prompt for which functions to update during functions:secrets:set#10843
Berlioz wants to merge 1 commit into
mainfrom
vsfan_ext_secrets_set

Conversation

@Berlioz

@Berlioz Berlioz commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@Berlioz
Berlioz requested a review from inlined July 24, 2026 00:16

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request updates the functions:secrets:set command to filter endpoints using isFirebaseManaged and introduces an interactive checkbox prompt allowing users to select which functions to re-deploy when multiple endpoints are affected. Feedback on these changes highlights two issues: first, using e.id as the checkbox value can cause conflicts since function IDs are not unique across regions (suggesting using the array index instead); second, passing an empty array [] as a fallback for e.labels causes a TypeScript type mismatch, which should be resolved by using an empty object {}.

Comment on lines +125 to +141
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));
}

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));
}

const endpointsToUpdate = backend
let endpointsToUpdate = backend
.allEndpoints(haveBackend)
.filter((e) => isFirebaseManaged(e.labels ?? []))

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants