Skip to content

containers: update push and build to resolve the correct uri - #10623

Merged
petebacondarwin merged 5 commits into
mainfrom
cdr/containers-push-resolve
Sep 26, 2025
Merged

containers: update push and build to resolve the correct uri#10623
petebacondarwin merged 5 commits into
mainfrom
cdr/containers-push-resolve

Conversation

@IRCody

@IRCody IRCody commented Sep 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Before it was possible to get incorrect/confusing uris for images pushed with containers push and build.

Example:

containers push registry.cloudflare.com/image:tag would become registry.cloudflare.com//registry.cloudflare.com/image:tag.

This change modifies resolveImageName to behave like:

  • image:tag -> prepend registry.cloudflare.com/accountid/
  • registry.cloudflare.com/image:tag -> registry.cloudlfare.com/accountid/image:tag
  • registry.cloudflare.com/accountid/image:tag -> no change
  • anyother-registry.com/anything -> no change

Fixes #CC-6029

  • Tests
    • Tests included
    • Tests not necessary because:
  • Public documentation
    • Cloudflare docs PR(s):
    • Documentation not necessary because: Fixing behavior to be more correct not changing how the user interacts.
  • Wrangler V3 Backport
    • Wrangler PR:
    • Not necessary because: This feature is only in v4.
@IRCody
IRCody requested review from a team as code owners September 11, 2025 17:13
@changeset-bot

changeset-bot Bot commented Sep 11, 2025 •
edited
Loading

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 19e753d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@cloudflare/containers-shared Minor
wrangler Patch
@cloudflare/vite-plugin Patch
@cloudflare/vitest-pool-workers Patch

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
@github-project-automation github-project-automation Bot moved this to Untriaged in workers-sdk Sep 11, 2025
@github-actions

github-actions Bot commented Sep 11, 2025

Copy link
Copy Markdown
Contributor

Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the v3-maintenance branch with your changes. Thank you for helping us keep Wrangler v3 supported!

Depending on your changes, running git rebase --onto v3-maintenance main cdr/containers-push-resolve might be a good starting point.

Notes:

  • your PR branch should be named v3-backport-10623
  • add the skip-v3-pr label to the current PR to stop this workflow from failing
@pkg-pr-new

pkg-pr-new Bot commented Sep 11, 2025 •
edited
Loading

Copy link
Copy Markdown
create-cloudflare

npm i https://pkg.pr.new/create-cloudflare@10623

@cloudflare/kv-asset-handler

npm i https://pkg.pr.new/@cloudflare/kv-asset-handler@10623

miniflare

npm i https://pkg.pr.new/miniflare@10623

@cloudflare/pages-shared

npm i https://pkg.pr.new/@cloudflare/pages-shared@10623

@cloudflare/unenv-preset

npm i https://pkg.pr.new/@cloudflare/unenv-preset@10623

@cloudflare/vite-plugin

npm i https://pkg.pr.new/@cloudflare/vite-plugin@10623

@cloudflare/vitest-pool-workers

npm i https://pkg.pr.new/@cloudflare/vitest-pool-workers@10623

@cloudflare/workers-editor-shared

npm i https://pkg.pr.new/@cloudflare/workers-editor-shared@10623

wrangler

npm i https://pkg.pr.new/wrangler@10623

commit: 19e753d
emily-shen
emily-shen approved these changes Sep 11, 2025
View reviewed changes

@emily-shen emily-shen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is much nicer, thanks :)

should we do anything to wrangler containers build -p for consistency?
@github-project-automation github-project-automation Bot moved this from Untriaged to Approved in workers-sdk Sep 11, 2025
@IRCody

IRCody commented Sep 11, 2025

Copy link
Copy Markdown
Contributor Author

this is much nicer, thanks :)

should we do anything to wrangler containers build -p for consistency?

Good call. I added that in the latest commit.
@IRCody
IRCody force-pushed the cdr/containers-push-resolve branch from dc8a847 to f125331 Compare September 11, 2025 20:15
@IRCody IRCody changed the title containers: update push to resolve the correct uri Sep 11, 2025
@IRCody
IRCody requested a review from emily-shen September 11, 2025 20:17
gpanders
gpanders reviewed Sep 11, 2025
View reviewed changes
Comment thread packages/containers-shared/src/images.ts
} catch {
return image;
// not a valid url so assume it is in the format image:tag and pre-pend the registry
return getCloudflareRegistryWithAccountNamespace(accountId, image);

@gpanders gpanders Sep 11, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if image is a file path, like ./Dockerfile?

@IRCody IRCody Sep 11, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really should never be since this is only called before pushing an image.

I'm not sure if the function should be changed to deal with that or just change the name to be more reflective of what it is used for. WDYT?

@gpanders gpanders Sep 12, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine as is then, thanks for clarifying.
emily-shen
emily-shen reviewed Sep 12, 2025
View reviewed changes
Comment thread packages/wrangler/src/__tests__/cloudchamber/build.test.ts Outdated
@IRCody
IRCody requested a review from emily-shen September 12, 2025 16:48
@IRCody
IRCody force-pushed the cdr/containers-push-resolve branch 2 times, most recently from 2756e4a to 4767a79 Compare September 17, 2025 22:21
@IRCody
IRCody force-pushed the cdr/containers-push-resolve branch from 4767a79 to 9555cd6 Compare September 23, 2025 19:56
@IRCody @petebacondarwin
containers: update push to resolve the correct uri
5fd6701 
Before it was possible to get incorrect/confusing uris for images pushed
with containers push.

Example:

containers push registry.cloudflare.com/image:tag would become
registry.cloudflare.com/<accountid>/registry.cloudflare.com/image:tag.

This change modifies resolveImageName to behave like:

image:tag -> prepend registry.cloudflare.com/accountid/
registry.cloudflare.com/image:tag -> registry.cloudlfare.com/accountid/image:tag
registry.cloudflare.com/accountid/image:tag -> no change
anyother-registry.com/anything -> no change
@IRCody @petebacondarwin
containers: update build to resolve the correct uri
ec0e7d5 
Before it was possible to get incorrect/confusing uris for images built
with containers build -p.

Example:

containers build -p -t registry.cloudflare.com/image:tag . would become
registry.cloudflare.com/<accountid>/registry.cloudflare.com/image:tag.

This change modifies build to use resolveImageName which handles these
cases.
@IRCody @petebacondarwin
Containers: modify build to use the users provided tag
2035805 
Except when we are pushing to our registry.
@IRCody @petebacondarwin
check + test updates
809c5b5
@IRCody @petebacondarwin
Deploy test updates
19e753d
@petebacondarwin
petebacondarwin force-pushed the cdr/containers-push-resolve branch from 9555cd6 to 19e753d Compare September 26, 2025 12:51
@petebacondarwin
petebacondarwin merged commit 7a6381c into main Sep 26, 2025
34 of 36 checks passed
@petebacondarwin
petebacondarwin deleted the cdr/containers-push-resolve branch September 26, 2025 15:54
@github-project-automation github-project-automation Bot moved this from Approved to Done in workers-sdk Sep 26, 2025
@workers-devprod workers-devprod mentioned this pull request Sep 26, 2025
Merged
@lrapoport-cf lrapoport-cf mentioned this pull request Sep 30, 2025
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

8 participants

@IRCody @skepticfx @gpanders @nikitassharma @emily-shen @flakey5 @petebacondarwin @workers-devprod