Add build and push subcommands to cloudchamber #7378
Conversation
🦋 Changeset detectedLatest commit: ebb28fd 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
cloudchamber-build
branch
3 times, most recently
from
5e7b0b0 to
138e8b2
Compare
|
A wrangler prerelease is available for testing. You can install this latest build in your project with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-wrangler-7378You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/7378/npm-package-wrangler-7378Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-wrangler-7378 dev path/to/script.jsAdditional artifacts:cloudflare-workers-bindings-extension: wget https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-cloudflare-workers-bindings-extension-7378 -O ./cloudflare-workers-bindings-extension.0.0.0-ve27b4602b.vsix && code --install-extension ./cloudflare-workers-bindings-extension.0.0.0-ve27b4602b.vsixcreate-cloudflare: npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-create-cloudflare-7378 --no-auto-update@cloudflare/kv-asset-handler: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-cloudflare-kv-asset-handler-7378miniflare: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-miniflare-7378@cloudflare/pages-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-cloudflare-pages-shared-7378@cloudflare/unenv-preset: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-cloudflare-unenv-preset-7378@cloudflare/vite-plugin: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-cloudflare-vite-plugin-7378@cloudflare/vitest-pool-workers: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-cloudflare-vitest-pool-workers-7378@cloudflare/workers-editor-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-cloudflare-workers-editor-shared-7378@cloudflare/workers-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-cloudflare-workers-shared-7378@cloudflare/workflows-shared: npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/12898243735/npm-package-cloudflare-workflows-shared-7378Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
IRCody
force-pushed
the
cloudchamber-build
branch
from
138e8b2 to
16f1c9f
Compare
IRCody
force-pushed
the
cloudchamber-build
branch
from
16f1c9f to
8e75c5e
Compare
Added additional tests Removed login to managed registry in favor of credential helper Fixed lots of input validation/corner cases Piped dockers output directly to our stdio instead of trying to intercept.
| return yargs | ||
| .positional("PATH", { | ||
| type: "string", | ||
| describe: "path for the directory containing the dockerfile", |
There was a problem hiding this comment.
| describe: "path for the directory containing the dockerfile", | |
| describe: "path to the directory containing the Dockerfile to build", |
| alias: "t", | ||
| type: "string", | ||
| demandOption: true, | ||
| describe: "Tag to use for the built image", |
There was a problem hiding this comment.
We should mention that this should also include the name of the image, not just the tag (i.e. it should be -t foo-bar:1.0, not just -t 1.0).
docker build describes this as "Name and optionally a tag (format: "name:tag")". I think we should just use the exact same phrasing since it just gets passed to docker build -t anyway.
| describe: "Tag to use for the built image", | |
| describe: "Name and optionally a tag (format: \"name:tag\")", |
| .option("path-to-docker", { | ||
| type: "string", | ||
| default: "docker", | ||
| describe: "path to docker binary", |
There was a problem hiding this comment.
The option is called "path-to-docker" but accepts a command (i.e. "docker") so maybe docker-cmd is a better name? Path implies a full path.
There was a problem hiding this comment.
This is used to find the binary for docker. So if it's on your $PATH you can use 'docker' but if not you can pass it here.
Maybe the description could be changed to something like:
Add the path to your docker binary if it's not on your $PATH?
| }) | ||
| .option("build-command", { | ||
| type: "string", | ||
| describe: "custom build command to use", |
There was a problem hiding this comment.
I'm not really sure what this option does from the description or why/how it's different from the path-to-docker option. Do we need both? Can we eliminate one or the other?
I'm also curious to read more about the use case for this option. It strikes me as a potential footgun since specifying a custom build command seems to ignore all of the other options provided to this command, which seems like it could be confusing.
There was a problem hiding this comment.
The use case is you want to do something more complicated than we want to support so you can use this to hook in.
I could go either way on including it. Thinking about it again it seems pretty rare that someone will both need this level of control and not be comfortable using docker directly.
@mikenomitch: wdyt?
Agree the description needs more info for context if the option is going to stay.. Not sure a great way to phrase it but here is an attempt:
Build command to use instead of our default one. Note: Will override all of our defaults.
| }) | ||
| .option("platform", { | ||
| type: "string", | ||
| default: "linux/amd64", |
There was a problem hiding this comment.
Can we make the default whatever the user is using (e.g. linux/arm64 on an M1 Mac and linux/amd64 on x86)?
There was a problem hiding this comment.
The reasoning for defaulting to linux/amd64 is that is the only supported platform in cc today.
| .option("push", { | ||
| alias: "p", | ||
| type: "boolean", | ||
| describe: "push the built image to a registry", |
There was a problem hiding this comment.
Nit: since this only supports pushing to one registry, we should make that clear
| describe: "push the built image to a registry", | |
| describe: "push the built image to Cloudflare's managed registry", |
I see there's a TODO comment to potentially change this though. If that's the case then this suggestion doesn't apply.
Describe your change...
Add build and push subcommands to cloudchamber that build a docker image and push to the cloudchamber managed registry using a local docker daemon.
I wanted to get feedback on behavior/approach now but making a draft because still some todos: