-
Notifications
You must be signed in to change notification settings - Fork 1k
try pulling an image even if docker login fails #11360
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.
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 258e5be The changes in this PR will be included in the next version bump. 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
@cloudflare/workers-utils
wrangler
commit: |
| try { | ||
| // this will fail in two cases: | ||
| // 1. this is being called from the vite plugin (doesn't have the appropriate auth context) | ||
| // 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
domain not configured I think?
And if so, are we okay with local dev working for something that won't work in prod?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whoops yes i forgot to finish that sentence. :') but yes exactly.
I think this is equivalent to being able to local dev with kv etc. but not have created the kv yet. So that wouldn't work at deploy, but with kv etc. we do have a nice flow that provisions KV for you at deploy time.
What we could do is check if you can get the creds at deploy time, and if you can't then run the user through the registries configure flow.
Alternatively, I could make the fall back only happen for vite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we could do is check if you can get the creds at deploy time, and if you can't then run the user through the registries configure flow.
I like this idea, feels like it would be useful for more than just local dev -> deploy workflow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay I was thinking about this more and I'm not sure this would work with public images. especially with docker, we have no way to differentiate public and private images from domain
Fixes CC-6472
Previously the only image registry allowed was the cloudflare managed one, and the only way to auth with that was to query the containers api for credentials. So we always did that before pulling an image. The vite plugin (for various reasons) couldn't call that api, so registry uris didn't work at all with the vite plugin.
Now we support some external registries, which users could authenticate with directly using docker login, so let's try pulling the image anyway even if calling our API fails. The API might return (short-lived) credentials for external registries if this has been previously configured via
wrangler contaienrs registries configure, so it is worth trying to call it still.The cloudflare-managed registry still wont work with the vite plugin as the only way to get credentials for that is via our API, so this PR also adds a more helpful error message in that case.
Also note that I'm not sure if it is worth adding CI tests for this, since it would involve having to set up a wrangler AWS or dockerhub account to run those tests, and storing secrets for that on this github repo, and introducing a likely spot for flakes.
However, i have tested this manually by: