Update buildAndMaybePush to use more robust method to find local digests #9641
Conversation
🦋 Changeset detectedLatest commit: 0bf5624 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
IRCody
force-pushed
the
cdr/docker-inspect
branch
from
88c950b to
981ac74
Compare
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: |
IRCody
force-pushed
the
cdr/docker-inspect
branch
8 times, most recently
from
b550b1c to
22ed63b
Compare
Older versions of docker sometimes will report the digest as "<none>" which would break with the previous implementation. This implementation should correctly grab the values.
IRCody
force-pushed
the
cdr/docker-inspect
branch
from
22ed63b to
40c3372
Compare
|
left a few nit comments, but LGTM otherwise. |
Fixes an issue where the user could get into a state where the image has been pushed but the container fails to update it's configuration. On the next push container.configuration.image will not be set and container.image is deleted leading to failures when trying to update the container app.
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
There was a problem hiding this comment.
I'm not too familiar with this part of the codebase, but the code looks good to me 🙂
(I am also trusting Carmen's comment #9641 (comment))
| @@ -18,14 +18,17 @@ export async function maybeBuildContainer( | |||
| imageTag: string, | |||
| dryRun: boolean, | |||
| pathToDocker: string | |||
| ) { | |||
| ): Promise<{ image: string; pushed: boolean }> { | |||
| try { | |||
| if ( | |||
| !isDockerfile( | |||
| containerConfig.image ?? containerConfig.configuration?.image | |||
There was a problem hiding this comment.
containerConfig.image has type string so the ?? isn't necessary
| containerConfig.image ?? containerConfig.configuration?.image | |
| containerConfig.image |
There was a problem hiding this comment.
For this should it be:
`containerconfig.image === "" ? containerConfig.configuration?.image : containerConfig.image;
There was a problem hiding this comment.
looks like containerConfig.image === "" throws a validation error, so that shouldn't be possible by the time we get here.
also yeah the nullish coalescing thingy should be unnecessary anymore, that was me earlier today 😅
we should have a diff type for wrangler config and the api (since one expects containers.image and one containers.configuration.image 🫠 ) to make this sort of thing clearer, but we'll follow up on that later.
Older versions of docker sometimes will report the digest as "" which would break with the previous implementation. This implementation should correctly grab the values.
Fixes #[insert GH or internal issue link(s)].
Describe your change...