Prompt correctly for secrets defined in codebases with namespaced prefixes - #11058
Prompt correctly for secrets defined in codebases with namespaced prefixes#11058Berlioz wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
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}`); |
There was a problem hiding this comment.
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.
| param.resourceId = toUpperSnakeCase(`${prefix}-${param.resourceId || param.name}`); | |
| param.resourceId = `${prefix}-${param.resourceId || param.name}`; |
There was a problem hiding this comment.
we should actually do it in both places, because functions:secrets:set rejects any secret names that aren't in upper snake case
There was a problem hiding this comment.
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
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.