-
Notifications
You must be signed in to change notification settings - Fork 559
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
Implement versions deploy
command
#5115
Conversation
🦋 Changeset detectedLatest commit: bab3451 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 |
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/8256774955/npm-package-wrangler-5115 You can reference the automatically updated head of this PR with: npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5115/npm-package-wrangler-5115 Or you can use npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8256774955/npm-package-wrangler-5115 dev path/to/script.js Additional artifacts:npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8256774955/npm-package-create-cloudflare-5115 --no-auto-update npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8256774955/npm-package-cloudflare-kv-asset-handler-5115 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8256774955/npm-package-miniflare-5115 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8256774955/npm-package-cloudflare-pages-shared-5115 npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8256774955/npm-package-cloudflare-vitest-pool-workers-5115 Note that these links will no longer work once the GitHub Actions artifact expires.
Please ensure constraints are pinned, and |
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.
There's a lot of new code in here with no tests...
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.
Halfway through my review, but left comments so far.
2362963
to
e28abf3
Compare
0df6b1f
to
2736606
Compare
df12e02
to
2722fb7
Compare
If I create a new Worker, publish a new version, and then run this, in interactive mode I'm shown:
...and then no matter which version I select, I get the following error:
(If I add specific version IDs as positional args then things work as expected, and this is looking really cool!) cc @tanushreesharma-cf |
@irvinebroque that's odd – have you had similar problems with C3's interactive prompts? I don't have any issues inside VSCode's terminal or in iTerm2: Screen.Recording.2024-03-08.at.10.32.11.mov |
28c06f0
to
724d2d4
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5115 +/- ##
==========================================
+ Coverage 70.47% 70.98% +0.50%
==========================================
Files 298 301 +3
Lines 15539 15828 +289
Branches 3999 4053 +54
==========================================
+ Hits 10951 11235 +284
- Misses 4588 4593 +5
|
@@ -86,8 +92,29 @@ type RenderProps = | |||
| Omit<ConfirmPrompt, "prompt"> | |||
| Omit<SelectRefreshablePrompt, "prompt">; | |||
|
|||
export const inputPrompt = async <T = string>(promptConfig: PromptConfig) => { | |||
const renderers = getRenderers(promptConfig); | |||
function acceptDefault<T>( |
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.
Should T extends Arg
to avoid as Arg
casting below?
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.
Doesn't quite work for reasons I can't remember now but I think the better solution is to dissect inputPrompt by type instead of passing in type as an option as if the other options are the same across all types
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.
I think the reason was not all possible values of T are Arg...
const lines = renderers.submit({ value: initialValue as Arg }); | ||
logRaw(lines.join("\n")); | ||
|
||
return initialValue as T; |
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.
initialValue
is already typed T
so I assume this case isn't necessary?
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.
Yep can fix. Copied it straight outta C3
return initialValue as T; | ||
} | ||
|
||
export const inputPrompt = async <T = string>( |
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.
Should this T extends string
too? It looks like later you cast a string
to T
.
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.
Unfortunately T can be anything – for example, select prompts can be anything and will be whatever the type of "value" is (it's an underlying key so doesn't need to be stringifiable) and multiselect will be an array of anything
@@ -167,3 +168,7 @@ function removeStandardPricingWarning(stdout: string): string { | |||
"" | |||
); | |||
} | |||
|
|||
function removeZeroWidthSpaces(stdout: string) { | |||
return stdout.replaceAll(/\u200a|\u200b/g, " "); |
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.
Would it be worth replacing these with a more recognisable sequence to distinguish these in tests? Maybe <ZWSP>
?
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.
It feels like an implementation detail. The zero-width space is for clack to not remove the indentation spacing. Doesn't need to be in the snapshots – since the absence of the zwsp will mean no indentation in the snapshot which will be caught and fail the test
); | ||
|
||
await expect(result).rejects.toMatchInlineSnapshot( | ||
`[Error: You must select at least 1 version to deploy.]` |
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.
Presumably this is only failing because the --yes
flag is set? Without that, you'd get an interactive prompt?
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.
Yes and if you didn't select anything from the interactive prompt – same flow.
│ | ||
├ What percentage of traffic should Worker Version 1 receive? | ||
├ 100% of traffic | ||
├ |
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.
Should this be a │
not a ├
?
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.
I didn't want to mess too much with the CLI package because of existing usage – I'm not doing anything special with it here so it can and should be fixed there but can be done as a follow-up when thought through properly.
For now, this is a minor design glitch I think we're fine with going ahead with
19dbe5a
to
96b38f7
Compare
Had to rebase because of conflict in main |
45473f1
to
32b6d43
Compare
not supported by api right now, can revisit later. excluded right now to avoid scope creep
for versions deploy command
to ensure spinner is cleaned up upon errors
+ implement inputPrompt acceptDefault option
will redo in a different PR to avoid the hude snapshot diff
32b6d43
to
b681d4e
Compare
@@ -261,15 +313,16 @@ const getSelectRenderers = ( | |||
const active = i === cursor; | |||
const isInListOfValues = | |||
Array.isArray(value) && value.includes(optionValue); | |||
const color = isInListOfValues || active ? blue : dim; |
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.
@RamIdeas was this change intentional? This is causing select values to render differently than they have in previous versions:
Can we change this back or make it overridable somehow?
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.
Ah yeah my bad I made it match the design doc but I think this is where single select vs multi select may differ. We should double check with Meaghan. If we want different styles for single vs multi select we can make it overridable in a base implementation and have higher level getSelectRenderers and getMultiSelectRenderers
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.
I also wanted to cc you on the acceptDefault option I added to inputPrompt
What this PR solves / how to test:
This PR implements the
versions deploy
command as a sibling command to the, already merged,versions upload
command. Both are still behind the--experimental-gradual-rollouts
flag.Note: in the commands below, it is assumed you have followed the install instructions of the prerelease build (from the bot comment in this PR). You can, instead, replace "wrangler" with the prerelease URL from the bot comment.
The most common usage will be without args where the prompts will walk you through selecting Versions and percentages for each and adding a deployment message. A sane default is provided for each percentage, evenly distributing the remaining percentage across the remaining versions.
You can also specify positional args in the form of
<version-id>
or<version-id>@<percentage>
which will set the default selected Versions and the default associated percentage for each. The prompts are still presented but with prefilled defaults that the user can press ENTER to confirm.You can also specify named args
--version-id
and--percentage
repeatedly. All--version-id
args are read in order into an array. All--percentage
args are read in order into an array. The indexes of each array are then used to correlate. By specifying less--percentage
args than--version-id
args, the percentages for the excess versions are deemed unspecified, like with positional args without@<percentage>
, and will have the remaining percentage distributed evenly as with positional/prompted percentages.$ npx wrangler versions deploy --experimental-gradual-rollouts \ --version-id 095f00a7-23a7-43b7-a227-e4c97cab5f22 \ --percentage 10% \ --version-id 1a88955c-2fbd-4a72-9d9b-3ba1e59842f2 \ --percentage 90% # is equivalent to $ npx wrangler versions deploy --experimental-gradual-rollouts \ --version-id 095f00a7-23a7-43b7-a227-e4c97cab5f22 \ --version-id 1a88955c-2fbd-4a72-9d9b-3ba1e59842f2 \ --percentage 10% \ --percentage 90%
A mixture of positional and named args will work and should be relatively predictable, with positional args taking precedence.
By using the
--yes
flag, the prompt defaults will be automatically accepted. This will allow the following:Author has addressed the following:
Note for PR author:
We want to celebrate and highlight awesome PR review! If you think this PR received a particularly high-caliber review, please assign it the label
highlight pr review
so future reviewers can take inspiration and learn from it.