add npx wrangler containers registry commands - #10605
Conversation
🦋 Changeset detectedLatest commit: a52538e 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: |
31e3a27 to
66fc3ae
Compare
66fc3ae to
8a5ab65
Compare
8a5ab65 to
186ea83
Compare
186ea83 to
fa77070
Compare
2756e4a to
4767a79
Compare
fa77070 to
0a7dd25
Compare
4767a79 to
9555cd6
Compare
9555cd6 to
19e753d
Compare
84bb09e to
587323e
Compare
f6643b0 to
643f892
Compare
1f4e96e to
05c4367
Compare
05c4367 to
d1a85f8
Compare
d1a85f8 to
7e1f886
Compare
| }); | ||
|
|
||
| export const validateSecretName = (name: string) => { | ||
| const validName = /^[A-z0-9-_]+$/; |
There was a problem hiding this comment.
This looks like a valid concern, maybe switch to /^[\w-]+$/ (\w include _)
There was a problem hiding this comment.
this is the same regex that secret store applies api side
|
|
||
| /** | ||
| * Request body for creating a new image registry configuration | ||
| */ |
There was a problem hiding this comment.
Why is this comment removed?
Should the new fields be documented?
| /* istanbul ignore file */ | ||
| /* tslint:disable */ | ||
| /* eslint-disable */ | ||
|
|
There was a problem hiding this comment.
Could this be removed everywhere it is added (not applicable to this repo)?
There was a problem hiding this comment.
everything in containers-shared/src/client/ is autogenerated from an openapi spec in the cloudchamber repo, and this is unfortunately full of 'any' uses so we do actually need to disable eslint.
we could remove the others but again i'd like to avoid unnecessarily increasing the diff here.
| * The type of external registry that is being configured. | ||
| */ | ||
| export enum ExternalRegistryKind { | ||
| ECR = "ECR", |
There was a problem hiding this comment.
nit: any particular reason to give this an explicit name ( = "ECR")?
| /* eslint-disable */ | ||
|
|
||
| /** | ||
| * A JSON string that encodes the auth required to authenticate with an external image registry. The format of the JSON object is determined by the registry being configured. |
| options: Exclude<ContainerDevOptions, DockerfileConfig> | ||
| ): Promise<{ abort: () => void; ready: Promise<void> }> { | ||
| await dockerLoginManagedRegistry(dockerPath); | ||
| const domain = new URL(`http://${options.image_uri}`).hostname; |
There was a problem hiding this comment.
This could throw, should the error be handled?
There was a problem hiding this comment.
this will already be validated by this point so it shouldn't, adding a comment to explain
| let url: URL; | ||
| try { | ||
| url = new URL(`http://${domain}`); | ||
| } catch (e) { | ||
| if (e instanceof Error) { | ||
| throw new Error(`${domain} is invalid:\n${e.message}`); | ||
| } | ||
| throw e; | ||
| } |
There was a problem hiding this comment.
You could use URL.canParse here
There was a problem hiding this comment.
alas we still support node 20 where this doesn't exist
|
|
||
| // containers | ||
| wrangler.command("containers", false, (containersArgs) => { | ||
| wrangler.command("containers", "📦 Manage Containers ", (containersArgs) => { |
There was a problem hiding this comment.
| wrangler.command("containers", "📦 Manage Containers ", (containersArgs) => { | |
| wrangler.command("containers", "📦 Manage Containers", (containersArgs) => { |
Not sure if this will break all the snapshots!
| } | ||
|
|
||
| const acceptedRegistries: RegistryPattern[] = [ | ||
| { |
There was a problem hiding this comment.
Would it make sense to consolidate this object (for ext registries) and type in an ExternalRegistry.ts file that would also have the content from the current ExternalRegistryKind.ts
There was a problem hiding this comment.
Everything in containers-shared/src/client/ is autogenerated from an open api spec in the cloudchamber internal repo, and I would prefer to not mix in hand written code with the autogenerated code. also the autogenerated client will be removed soon anyway in favour of the cloudflare ts sdk :)
| expect(result[0].rollout_step_percentage).toBe(100); | ||
| }); | ||
|
|
||
| describe("image validation and resolution", async () => { |
There was a problem hiding this comment.
Is "image validation" accurate here?
It looks like this is validating registries instead?
There was a problem hiding this comment.
it is also validating how we resolve image names (e.g. adding account ids)
| ) | ||
| .command( | ||
| "registries", | ||
| // hide for now so it doesn't show up in help while we not publicly available |
There was a problem hiding this comment.
| // hide for now so it doesn't show up in help while we not publicly available | |
| // hide for now so it doesn't show up in help while not publicly available |
| description: | ||
| "The public part of the registry credentials, e.g. `AWS_ACCESS_KEY_ID` for ECR", | ||
| demandOption: true, | ||
| alias: ["aws-access-key-id"], |
There was a problem hiding this comment.
Isn't that going to be a problem when we add more registry types?
i.e. you'll be able to use aws-access-key-id to configure GCP containers
| log("\n"); | ||
| log("Setting up integration with Secrets Store...\n"); |
There was a problem hiding this comment.
| log("\n"); | |
| log("Setting up integration with Secrets Store...\n"); | |
| log("\nSetting up integration with Secrets Store...\n"); |
| const stores = await listStores(config, accountId); | ||
| if (stores.length === 0) { | ||
| const defaultStoreName = "default_secret_store"; | ||
| const check = await confirm( |
There was a problem hiding this comment.
nit: use yes/create instead? The former is consistent with other usage in this PR, create is also nice to describe what is being done/asked.
vicb
left a comment
There was a problem hiding this comment.
The regexp should be updated.
Also added other minor comments.
LGTM with the changes.
0f96364 to
a52538e
Compare
| /** | ||
| * get type of container registry, and validate | ||
| * currently we support cloudflare managed registries and AWS ECR | ||
| * when using cloudflare mananged registries we expect CLOUDFLARE_CONTAINER_REGISTRY to be set |
There was a problem hiding this comment.
| * when using cloudflare mananged registries we expect CLOUDFLARE_CONTAINER_REGISTRY to be set | |
| * when using cloudflare managed registries we expect CLOUDFLARE_CONTAINER_REGISTRY to be set |
| } | ||
|
|
||
| /** | ||
| * get type of container registry, and validate |
There was a problem hiding this comment.
| * get type of container registry, and validate | |
| * Get type of container registry, and validate |
|
merging this as-is because the remaining comments are nits, this is hidden and i am going to do a follow-up pr immediately after |
CC-5835
Add commands to support configuring credentials for external (non cloudflare) registries. Currently this should only be AWS ECR. We are still working on this incrementally so the commands are hidden.
This works by integrating with the secrets store. Credentials for registries usually consist of a public and private component - only the actually secret bit is stored in the secret store, which wrangler will create for you. The secret is either added interactively or piped in. Currently we don't allow you to provide an existing secret. The secret store id and secret name are then passed to the containers control plane.
The public part is passed in via
--public-credentialand stored in the containers control plane.You can provide a store id but currently secret store only allows for one store per account so that is a bit redundant.