BUG: experimental secret support for non-standard resource IDs was not propagating to deploy prepare#10839
BUG: experimental secret support for non-standard resource IDs was not propagating to deploy prepare#10839Berlioz wants to merge 3 commits into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new function matchSecretEnvVars to synchronize SecretEnvVars with their corresponding secret parameters in a build, addressing cases where new secret resources are created during parameter resolution. Feedback highlights a correctness bug regarding case-sensitive comparison of environment variable keys and secret parameter names, and suggests refactoring the function to reduce nesting in accordance with the repository's style guide.
| export function matchSecretEnvVars(build: Build): void { | ||
| for (const param of build.params) { | ||
| if (param.type !== "secret") { | ||
| continue; | ||
| } | ||
| const secretParam = param; | ||
|
|
||
| for (const endpointName of Object.keys(build.endpoints)) { | ||
| const endpoint = build.endpoints[endpointName]; | ||
| for (const envVar of endpoint.secretEnvironmentVariables ?? []) { | ||
| if (envVar.key === secretParam.name) { | ||
| if (secretParam.resourceId && envVar.secret !== secretParam.resourceId) { | ||
| logger.debug( | ||
| `Inserted newly created secret into build.SecretEnvVars: ${envVar.key}=${secretParam.resourceId}`, | ||
| ); | ||
| envVar.secret = secretParam.resourceId; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
but it came through applyEnvSecretBindings, so we know--whatever, fine
|
Since matchSecretEnvVars is newly exported, we might want to have some unit tests in place. Specifically to test that envVar.secret updates when secretParam.resourceId changes and that case-insensitive matching works properly. |
| export function matchSecretEnvVars(build: Build): void { | ||
| for (const param of build.params) { | ||
| if (param.type !== "secret") { | ||
| continue; | ||
| } | ||
| const secretParam = param; | ||
| if (!secretParam.resourceId) { | ||
| continue; | ||
| } | ||
|
|
||
| for (const endpoint of Object.values(build.endpoints)) { | ||
| for (const envVar of endpoint.secretEnvironmentVariables ?? []) { | ||
| if (envVar.key.toUpperCase() !== secretParam.name.toUpperCase()) { | ||
| continue; | ||
| } | ||
| if (envVar.secret !== secretParam.resourceId) { | ||
| logger.debug( | ||
| `Inserted newly created secret into build.SecretEnvVars: ${envVar.key}=${secretParam.resourceId}`, | ||
| ); | ||
| envVar.secret = secretParam.resourceId; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Jetski thinks we can flatten this more:
export function matchSecretEnvVars(build: Build): void {
const secretParamsByKey = new Map<string, SecretParam>();
for (const param of build.params) {
if (param.type === "secret" && param.resourceId) {
secretParamsByKey.set(param.name.toUpperCase(), param);
}
}
if (secretParamsByKey.size === 0) return;
for (const endpoint of Object.values(build.endpoints)) {
for (const envVar of endpoint.secretEnvironmentVariables ?? []) {
const secretParam = secretParamsByKey.get(envVar.key.toUpperCase());
if (secretParam && envVar.secret !== secretParam.resourceId) {
logger.debug(
`Inserted newly created secret into build.SecretEnvVars: ${envVar.key}=${secretParam.resourceId}`,
);
envVar.secret = secretParam.resourceId;
}
}
}
}There was a problem hiding this comment.
I'm pretty sure Record is preferred to Map in this repo?
More seriously, I don't agree with Jetski here and I don't even think this suggestion would compile without some fairly ugly surgery: in the context of this file, SecretParam is build.SecretParam which contains just the wire-level representation of a secret that we get from the SDK. For the types to line up here the Map would have to be declared with the params.SecretParam type, which isn't currently exported.
There was a problem hiding this comment.
Fair point on both counts! I didn't catch that SecretParam in build.ts was a local type collision with params.ts. Since you have already pushed the test cases, going ahead with the approval
3 participants
resolveParams() can now change the resourceID field of a secret param, which means that changes to them must be propagated to the Build's secretEnvVars before Backend compilation to be picked up by Functions/Run.