[wrangler] Add wrangler flagship commands#14423
Conversation
🦋 Changeset detectedLatest commit: a556313 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
985b941 to
e0b3fb6
Compare
@cloudflare/autoconfig
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
2c729ff to
e0b6e06
Compare
Add `wrangler flagship` commands for managing Flagship apps and feature flags. The new `wrangler flagship apps` and `wrangler flagship flags` command groups let you create, list, get, inspect, update, set, split, rollout, enable, disable, evaluate, and delete Flagship apps and flags from the CLI, including targeting rules, variations, percentage rollouts, evaluation context, and flag changelogs.
e0b6e06 to
311aa58
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers |
| export async function listApps(config: Config): Promise<App[]> { | ||
| const accountId = await requireAuth(config); | ||
| return await fetchResult(config, `/accounts/${accountId}/flagship/apps`, { | ||
| method: "GET", | ||
| }); | ||
| } |
There was a problem hiding this comment.
🚩 Apps listing does not support pagination unlike flags listing
The listApps function at packages/wrangler/src/flagship/client.ts:151-156 uses a simple fetchResult call that returns all apps in a single response. In contrast, listFlags (client.ts:192-205) uses fetchPage with cursor-based pagination, and the CLI exposes --limit, --cursor, and --all options. If the number of apps grows large enough to be paginated by the API, listApps would only return the first page. This may be intentional if the API doesn't paginate apps, but it's an asymmetry worth noting.
Was this helpful? React with 👍 or 👎 to provide feedback.
petebacondarwin
left a comment
There was a problem hiding this comment.
Left a small number of nits but looks good.
Can you get a member of your product team to either actually approve the PR or comment with their approval that this all does what it is expected of the product?
| } | ||
| const byPriority = new Map(rules.map((rule) => [rule.priority, rule])); | ||
| const existing = sortedRules(rules).map((rule) => rule.priority); | ||
| if (order.length !== rules.length || new Set(order).size !== order.length) { |
There was a problem hiding this comment.
NIT: you could probably just do:
| if (order.length !== rules.length || new Set(order).size !== order.length) { | |
| if (new Set(order).size !== rules.length) { |
| target: { | ||
| type: "string", | ||
| array: true, | ||
| demandOption: true, | ||
| description: "The app ID followed by one or more flag keys to delete", | ||
| }, |
There was a problem hiding this comment.
Why does this command not take the app-id as its own positional parameter?
| target: { | ||
| type: "string", | ||
| array: true, | ||
| demandOption: true, | ||
| description: `The app ID followed by one or more flag keys to ${verb.toLowerCase()}`, | ||
| }, |
There was a problem hiding this comment.
Similarly, why does target conflate app-id and keys?
| import type { FlagType, Rule } from "../client"; | ||
|
|
||
| export const flagshipFlagsUpdateCommand = createCommand({ | ||
| metadata: { |
There was a problem hiding this comment.
This feels like a command that could really do with some examples.
| import { UserError } from "@cloudflare/workers-utils"; | ||
| import { logger } from "../logger"; | ||
|
|
||
| export function splitAppIdAndKeys(targets: string[]): { |
There was a problem hiding this comment.
why export this one?
| if (typeof value === "string") { | ||
| return `"${value}"`; | ||
| } | ||
| return JSON.stringify(value) ?? String(value); |
There was a problem hiding this comment.
stringify() will always return a string or undefined so I assume what you want here is that if value is undefined we get the string "undefined"?
| if (typeof value === "string") { | ||
| return `"${value}"`; | ||
| } |
There was a problem hiding this comment.
This is not necessary since if value is a string then JSON.stringify(value) will return exactly what is being generated here.
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
Adds
wrangler flagshipfor managing Cloudflare Flagship apps and feature flags.This includes
appscommands for create/list/get/update/delete, andflagscommands for create/list/get/update/set/split/rollout/enable/disable/evaluate/delete/changelog, with aliases,--jsonoutput, config binding fallback for app IDs, human-readable rendering, and unit test coverage.Includes a
wranglerminor changeset.