Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for optional secret parameters by adding an optional property to secret parameter specifications and options. This allows users to bypass creating a backing Cloud Secret Manager resource, resulting in an undefined runtime value without triggering warnings. While the changes successfully update SecretParam and the associated test suites, the feedback highlights that JsonSecretParam also inherits these options but does not yet support optional values in its runtimeValue() implementation, which currently throws an error. It is recommended to update JsonSecretParam to support optional values and add corresponding test coverage for it.
| /** If true, allows the user to decline to create a backing Cloud Secret Manager resource, resulting in undefined runtime value. */ | ||
| optional?: boolean; |
There was a problem hiding this comment.
Since optional is now added to SecretParamOptions, it is also accepted by JsonSecretParam (which uses SecretParamOptions in its constructor). However, JsonSecretParam.runtimeValue() currently always throws an error if the environment variable is undefined, regardless of whether optional is set to true.
To support optional JSON secrets, JsonSecretParam should be updated to return undefined when the secret is missing and optional is true.
For example, in JsonSecretParam (around line 604):
export class JsonSecretParam<T = any> {
// ...
runtimeValue(): T | undefined {
const val = process.env[this.name];
if (val === undefined) {
if (this.options.optional) {
return undefined;
}
throw new Error(
`No value found for secret parameter "${this.name}". A function can only access a secret if you include the secret in the function's dependency array.`
);
}
try {
return JSON.parse(val) as T;
} catch (error) {
throw new Error(
`"${this.name}" could not be parsed as JSON. Please verify its value in Secret Manager. Details: ${error}`
);
}
}
value(): T | undefined {
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.
Okay, what I did instead is clunky, but also there appear to be literally zero users of non-required JSON secrets in actual 1p or 3p extensions right now so...eh....
ajperel
left a comment
There was a problem hiding this comment.
I think this looks good but will wait on API review
| export type ParamOptions<T extends string | number | boolean | string[]> = Omit< | ||
| ParamSpec<T>, | ||
| "name" | "type" | ||
| "name" | "type" | "optional" |
There was a problem hiding this comment.
For my own learning what is this doing?
There was a problem hiding this comment.
ParamOptions is the type being used for a Param<string>, Param<number> etc, so this is expressing that optional is not a valid option for those.
| const val = process.env[this.name]; | ||
| if (val === undefined) { | ||
| if (this.options.optional) { | ||
| return {} as T; |
There was a problem hiding this comment.
Should we have a test for this return value?
relnote: none