feat(cloudflare): support prebuilt images in Container resource #1194
Conversation
adunne09
force-pushed
the
alex/support-prebuilt-cloudflare-images
branch
from
fc3a0d1 to
473c416
Compare
|
@john-royal can you take a look at this? |
john-royal
left a comment
john-royal
left a comment
There was a problem hiding this comment.
Left some comments - happy to merge as-is but want to be sure the approach is correct.
| /** | ||
| * Use a prebuilt image instead of building one. | ||
| * Can be a string (image reference), RemoteImage resource, or Image resource. | ||
| * Mutually exclusive with `build` property. |
There was a problem hiding this comment.
If this is mutually exclusive with build, should we use a union type so you can't accidentally provide both?
There was a problem hiding this comment.
I opted with the runtime error at L316
| tag, | ||
| build: props.build, | ||
| }); | ||
| // In local dev mode, always build if build config is provided |
There was a problem hiding this comment.
What motivated this when build and image are meant to be mutually exclusive? Does it have to do with the build platform potentially being different?
| if (isCloudflareRegistryLink(imageRef)) { | ||
| // Already in CF registry, create a lightweight Image reference | ||
| const imageName = extractNameFromImageRef(imageRef); | ||
| const imageTag = imageRef.includes(":") | ||
| ? imageRef.split(":").pop()!.split("@")[0] | ||
| : "latest"; | ||
|
|
||
| image = { | ||
| name: imageName, | ||
| tag: imageTag, | ||
| imageRef: imageRef, | ||
| repoDigest: imageRef.includes("@") ? imageRef : undefined, | ||
| builtAt: Date.now(), | ||
| }; | ||
| } else { | ||
| // External image - automatically push to CF registry | ||
| // Cloudflare Containers currently only support images from registry.cloudflare.com | ||
| image = await retagAndPushToCloudflare(imageRef, name, tag, api); | ||
| } | ||
| } else { | ||
| // It's an Image or RemoteImage resource | ||
| const sourceImage = props.image as Image | RemoteImage; | ||
| const sourceImageRef = sourceImage.imageRef; | ||
|
|
||
| // Check if it's already in CF registry | ||
| if (isCloudflareRegistryLink(sourceImageRef)) { | ||
| // Already in CF registry, use as-is | ||
| image = sourceImage as Image; | ||
| } else { | ||
| // Not in CF registry - automatically push to CF registry | ||
| // Cloudflare Containers currently only support images from registry.cloudflare.com | ||
| image = await retagAndPushToCloudflare(sourceImageRef, name, tag, api); | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we use the RemoteImage resource here?
There was a problem hiding this comment.
my intent was to maintain the previous Image declaration from L235-
const image = await Image(id, {
name: `${api.accountId}/${name}`,
tag,
build: {
platform: props.build?.platform ?? "linux/amd64",
...props.build,
},
registry: {
server: "registry.cloudflare.com",
username: credentials.username || credentials.user!,
password: secret(credentials.password),
},
});
what would be the advantage of changing to RemoteImage ? I'm not sure what you mean
3 participants
Summary
Changes
imageproperty toContainerProps(mutually exclusive withbuild)RemoteImageresources, andImageresourcesregistry.cloudflare.comimageorbuildis specified (but not both)Testing
Added 5 new tests covering:
Closes #1109