Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Sep 26, 2025
Merged

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

merged 5 commits into from
Sep 26, 2025

Conversation

IRCody
Copy link
Contributor

@IRCody IRCody commented Sep 11, 2025 •
edited
Loading

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
Copy link

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

🦋 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
Copy link
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
Copy link

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

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
Copy link
Contributor

@emily-shen emily-shen left a comment

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
@emily-shen emily-shen added the skip-v3-pr Skip validation of presence of a v3 backport PR label Sep 11, 2025
@IRCody
Copy link
Contributor Author

IRCody commented Sep 11, 2025

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
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);
Copy link
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?
Copy link
Contributor Author

@IRCody IRCody Sep 11, 2025 •
edited
Loading

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?
Copy link
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
@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

skip-v3-pr Skip validation of presence of a v3 backport PR

7 participants

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