feat(isolate_proto): add explicit container config#1065
Conversation
efiop
force-pushed
the
ruslan/container-config-proto
branch
from
dfb03a8 to
b0cd936
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b0cd93669f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Explicit container image configuration. When set, this replaces the legacy | ||
| // image payload previously embedded in `environments.configuration`. | ||
| optional ContainerConfig container_config = 16; |
There was a problem hiding this comment.
When clients adopt this explicit field and stop duplicating the image under environments.configuration, the explicit build path has no way to send the same container image: BuildEnvironmentRequest still only accepts environments/files/secrets even though the SDK documents it as the pre-build counterpart for Run/RegisterApplication (projects/fal/src/fal/sdk.py:1180-1184) and the CLI calls it before fetch_metadata(... build_environment=False) / run(... build_environment=False) (projects/fal/src/fal/cli/run.py:90-115, projects/fal/src/fal/api/deploy.py:256-277). In that context container apps would build/warm a different environment than the subsequent run/deploy, so add the same ContainerConfig field to BuildEnvironmentRequest before treating it as a replacement for the legacy payload.
Useful? React with 👍 / 👎.
| message ContainerConfig { | ||
| // Dockerfile contents used to build the image. | ||
| string dockerfile_str = 1; |
There was a problem hiding this comment.
HostedMap is the other RPC payload that carries repeated EnvironmentDefinition environments (line 69) and is documented as running the given function in parallel, but this new replacement for the legacy container image is only wired into HostedRun and RegisterApplicationRequest. If a Map caller migrates the image out of environments.configuration, there is no field to send it on this request, so mapped runs cannot use the explicit container config while single runs can.
Useful? React with 👍 / 👎.
1 participant
We currently pass all of this in environment as a raw dict, and this PR adds an explicit way to pass this through.