Resolve dockerfile to absolute path in redirected config in vite plugin #10038
Conversation
🦋 Changeset detectedLatest commit: 0f5b3f1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
| // however, wrangler deploy will re-resolve the config, so we should | ||
| // deduplicate the container.configuration.image field in favour of | ||
| // the non-deprecated one when we write out the deploy config | ||
| delete container.configuration?.image; |
There was a problem hiding this comment.
Could we set this to undefined in the output variable above rather than deleting here?
There was a problem hiding this comment.
What are your concerns with deleting it? I'd rather not have two different values for image kicking around here, the ContainerApp type and validation in wrangler is quite messed up. I have a refactor PR that should clean this up soon, but in the meantime its really hard in wrangler to know whether you should use container.configuration.image or container.image, since the types are just wrong 😭
There was a problem hiding this comment.
Your version is fine and it doesn't make any difference to the behaviour.
| // Wrangler's config validation resolves to container.configuration.image, even though it is deprecated | ||
| const image = | ||
| container.configuration?.image ?? container.image; | ||
| if (isDockerfile(image, workerConfig.configPath)) { |
There was a problem hiding this comment.
Does it matter that workerConfig.configPath is possible undefined here? Not sure if it ever can actually be undefined or if that's just a types thing.
There was a problem hiding this comment.
Ah - if its possibly undefined, then we fall back on the cwd. is there a better fallback i should be using here?
There was a problem hiding this comment.
I think that should be OK for now. We may be able to assert the configPath if it's always defined but would need to investigate more.
There was a problem hiding this comment.
I noticed that in wrangler we are also not resolving image_build_context to an absolute path, so i fixed that in 1776a99, even though it is not strictly related to this.
| // Wrangler's config validation resolves to container.configuration.image, even though it is deprecated | ||
| const image = | ||
| container.configuration?.image ?? container.image; | ||
| if (isDockerfile(image, workerConfig.configPath)) { |
There was a problem hiding this comment.
I think that should be OK for now. We may be able to assert the configPath if it's always defined but would need to investigate more.
To the same purpose as #10005, but in a different place :)