Skip to content

Prompt correctly for secrets defined in codebases with namespaced prefixes - #11058

Open
Berlioz wants to merge 2 commits into
mainfrom
vsfan_param_args
Open

Prompt correctly for secrets defined in codebases with namespaced prefixes#11058
Berlioz wants to merge 2 commits into
mainfrom
vsfan_param_args

Conversation

@Berlioz

@Berlioz Berlioz commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Taking a shot at fixing #11054.

The prefix feature alters a Build's SecretEnvVars to change the resource ID backing a Secret binding, but did not alter the Build's SecretParams, which have a resourceId field that controls where secret prompting expects to find/create the backing Secret resource.

Note that this only applies to greenfield/.env-less deploys--for deploys with an .env where secret bindings are already defined, applyEnvSecretBindings() runs after this code and blows away changes made during build parsing.

Also, changed resolveParams to take an Opts struct because the number of arguments it was developing was getting out of control and resulting in bugs. resolveBackend, which basically wraps resolveParams, was already doing something similar.

@Berlioz
Berlioz requested a review from ajperel September 8, 2026 23:33

@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 refactors resolveParams to accept an options object instead of positional arguments, updating all corresponding call sites and tests. It also introduces prefixing for secret parameter resource IDs in applyPrefix. The review feedback correctly identifies a critical issue where normalizing the resource ID with toUpperSnakeCase creates a mismatch with the unnormalized secret names in secretEnvironmentVariables, which would cause runtime failures. The reviewer suggests removing this normalization, updating the associated test assertions, and removing the now-unused import of toUpperSnakeCase.

if (param.type !== "secret") {
continue;
}
param.resourceId = toUpperSnakeCase(`${prefix}-${param.resourceId || param.name}`);

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.

critical

Using toUpperSnakeCase here causes a discrepancy between the parameter's resourceId (which becomes uppercase with underscores, e.g., STAGING_API_KEY) and the endpoint's secretEnvironmentVariables (which are prefixed directly without case normalization, e.g., staging-API_KEY or staging-api-secret). This mismatch will cause the deployed Cloud Function to look for a different secret name at runtime than the one prompted/created by the CLI, leading to immediate runtime failures on greenfield deploys. We should use the same prefixing logic without toUpperSnakeCase to keep them consistent.

Suggested change
param.resourceId = toUpperSnakeCase(`${prefix}-${param.resourceId || param.name}`);
param.resourceId = `${prefix}-${param.resourceId || param.name}`;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

we should actually do it in both places, because functions:secrets:set rejects any secret names that aren't in upper snake case

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fwiw lowercase and hyphens are allowed in gcp secret names so we could also just change functions:secrets:set if people think that would be neater

Comment thread src/deploy/functions/build.spec.ts
Comment thread src/deploy/functions/build.ts
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.

2 participants