Cloud Run Functions data model #8767
Conversation
|
/gemini review |
![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 extends the data model to support Cloud Run Functions, introducing a new 'run' platform type. The changes include refactoring shared utilities into a new k8s.ts file and adding a comprehensive runv2.ts module with tests for interacting with the Cloud Run v2 API.
My review focuses on ensuring consistency and correctness in the new code. I've identified areas for improvement, including an inconsistency in the AllFunctionsPlatforms array, a potential bug in how CPU values are handled, and critical bugs related to function ID conversion and billing calculations.
| // N.B. If GCF starts allowing v1's allowable characters in IDs they're | ||
| // going to need to have a transform to create a service ID (which has a | ||
| // more restrictive character set). We'll need to reimplement that here. | ||
| // BUG BUG BUG. This has happened and we need to fix it. |
There was a problem hiding this comment.
This comment indicates a known issue where the function ID to service ID conversion is not happening. It's marked as a BUG BUG BUG, suggesting it's critical and needs to be addressed.
| // BUG BUG BUG. This has happened and we need to fix it. | |
| // TODO: Implement function ID to service ID conversion |
There was a problem hiding this comment.
IMO the severity of this comment is accurate. This is a WIP and a BUG should be resolved before releasing to the public whereas a TODO can be ignored for years.
| ], | ||
| resources: { | ||
| limits: { | ||
| cpu: String((endpoint.cpu as Number) || 1), |
There was a problem hiding this comment.
Using the logical OR || for default values can lead to unexpected behavior if 0 is a valid value for endpoint.cpu. The nullish coalescing operator ?? is safer as it only provides the default for null or undefined values.
| cpu: String((endpoint.cpu as Number) || 1), | |
| cpu: String((endpoint.cpu as number) ?? 1), |
There was a problem hiding this comment.
0 is not a valid value AFAIK, so this shouldn't make a difference.
| // This may be tricky because the image has been deleted. How does this work | ||
| // with GCF? | ||
| throw new FirebaseError("Updating Cloud Run functions is not yet implemented."); |
There was a problem hiding this comment.
This error message is not very informative. Consider adding more context to the error message to help users understand why updating Cloud Run functions is not yet implemented.
| // This may be tricky because the image has been deleted. How does this work | |
| // with GCF? | |
| throw new FirebaseError("Updating Cloud Run functions is not yet implemented."); | |
| throw new FirebaseError("Updating secrets for Cloud Run functions is not yet implemented. Please use GCFv2."); |
Address robo-review Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
| export const RUNTIME_LABEL = "goog-cloudfunctions-runtime"; | ||
| export const CLIENT_NAME_LABEL = "goog-managed-by"; | ||
| export const CLIENT_NAME_ANNOTATION = "run.googleapis.com/client-name"; | ||
| export const CPU_BOOST_ANNOTATION = "run.googleapis.com/startup-cpu-boost"; | ||
| export const TRIGGER_TYPE_ANNOTATION = "cloudfunctions.googleapis.com/trigger-type"; | ||
| export const FUNCTION_TARGET_ANNOTATION = "run.googleapis.com/build-function-target"; // e.g. '{"worker":"triggerTest"}' | ||
| export const FUNCTION_ID_ANNOTATION = "cloudfunctions.googleapis.com/function-id"; // e.g. "triggerTest" | ||
| export const BASE_IMAGE_ANNOTATION = "run.googleapis.com/base-images"; // : '{"worker":"us-central1-docker.pkg.dev/serverless-runtimes/google-22-full/runtimes/nodejs22"}' | ||
| export const MAX_INSTANCES_ANNOTATION = "autoscaling.knative.dev/maxScale"; | ||
| export const MIN_INSTANCES_ANNOTATION = "autoscaling.knative.dev/minScale"; | ||
| export const DEFAULT_FUNCTION_CONTAINER_NAME = "worker"; |
There was a problem hiding this comment.
In https://cloud.google.com/run/docs/reference/rest/v2/projects.locations.services#Service
Cloud Run API v2 does not support annotations with run.googleapis.com, cloud.googleapis.com, serving.knative.dev, or autoscaling.knative.dev namespaces, and they will be rejected in new resources. All system annotations in v1 now have a corresponding field in v2 Service.
Suspect we'll have to refactor many of these?
Thanks for your patience. I've got part one of 2-3 on getting some PoCs ready for CRF. This just extends the data model to support the RunV2 API (needed for the submit build stuff), the service<->endpoint stuff, and the changes that popped up with assertExhaustive once a new type has been added.
This PR exposed two bugs (not fixed):