Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for optional secret parameters (OptionalSecretParam and defineOptionalSecret), allowing users to declare secrets that can be left unbound during deployment. While the implementation is mostly complete, several issues were identified in the review: OptionalSecretParam.runtimeValue() incorrectly falls back to an empty string instead of returning undefined when unbound; the SupportedSecretParam union type needs to be updated to include OptionalSecretParam to prevent TypeScript compilation errors when configuring function secrets; and a test assertion in params.spec.ts incorrectly references spec instead of jsonSpec.
| runtimeValue(): string | unknown { | ||
| const val = process.env[this.name]; | ||
| if (val === undefined) { | ||
| logger.info( | ||
| `No value found for optional secret parameter "${this.name}". Either it was left intentionally unbound, or you must add the secret to the function's dependency array.` | ||
| ); | ||
| } | ||
| return val || ""; | ||
| } | ||
|
|
||
| /** @internal */ | ||
| toSpec(): ParamSpec<string> { | ||
| return { | ||
| type: "secret", | ||
| optional: true, | ||
| name: this.name, | ||
| ...this.options, | ||
| }; | ||
| } | ||
|
|
||
| /** Returns the secret's value at runtime. Throws an error if accessed during deployment. */ | ||
| value(): string | unknown { | ||
| if (process.env.FUNCTIONS_CONTROL_API === "true") { | ||
| throw new Error( | ||
| `Cannot access the value of secret "${this.name}" during function deployment. Secret values are only available at runtime.` | ||
| ); | ||
| } | ||
| return this.runtimeValue(); | ||
| } |
There was a problem hiding this comment.
The implementation of runtimeValue() returns val || "" which evaluates to "" (an empty string) when the environment variable is undefined. This contradicts the documentation and type signature which state that .value() should return undefined (or unknown) when unbound/declined. Returning "" makes it impossible for users to distinguish between an explicitly set empty secret and an unbound/declined optional secret.\n\nWe should return val directly (which is string | undefined) and update the return type of both runtimeValue() and value() to string | undefined.
runtimeValue(): string | undefined {\n const val = process.env[this.name];\n if (val === undefined) {\n logger.info(\n `No value found for optional secret parameter "${this.name}". Either it was left intentionally unbound, or you must add the secret to the function's dependency array.`\n );\n }\n return val;\n }\n\n /** @internal */\n toSpec(): ParamSpec<string> {\n return {\n type: "secret",\n optional: true,\n name: this.name,\n ...this.options,\n };\n }\n\n /** Returns the secret's value at runtime. Throws an error if accessed during deployment. */\n value(): string | undefined {\n if (process.env.FUNCTIONS_CONTROL_API === "true") {\n throw new Error(\n `Cannot access the value of secret "${this.name}" during function deployment. Secret values are only available at runtime.`\n );\n }\n return this.runtimeValue();\n }
another take on #1966