Skip to content

Add prompt to wrangler pages download config#5506

Merged
penalosa merged 4 commits intomainfrom
penalosa/add-pages-download-warning
Apr 4, 2024
Merged

Add prompt to wrangler pages download config#5506
penalosa merged 4 commits intomainfrom
penalosa/add-pages-download-warning

Conversation

@penalosa
Copy link
Copy Markdown
Contributor

@penalosa penalosa commented Apr 4, 2024

What this PR solves / how to test

Adds an interactive prompt to wrangler pages download config if downloading the config would overwrite an existing wrangler.toml file.

Fixes DEVX-1243

Author has addressed the following

@penalosa penalosa requested review from a team as code owners April 4, 2024 11:21
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented Apr 4, 2024

🦋 Changeset detected

Latest commit: 55f08a3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
wrangler Minor
@cloudflare/vitest-pool-workers Patch

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

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 4, 2024

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/8553967145/npm-package-wrangler-5506

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5506/npm-package-wrangler-5506

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8553967145/npm-package-wrangler-5506 dev path/to/script.js
Additional artifacts:
npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8553967145/npm-package-create-cloudflare-5506 --no-auto-update
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8553967145/npm-package-cloudflare-kv-asset-handler-5506
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8553967145/npm-package-miniflare-5506
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8553967145/npm-package-cloudflare-pages-shared-5506
npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/8553967145/npm-package-cloudflare-vitest-pool-workers-5506

Note that these links will no longer work once the GitHub Actions artifact expires.


wrangler@3.45.0 includes the following runtime dependencies:

Package Constraint Resolved
miniflare workspace:* 3.20240329.1
workerd 1.20240403.0 1.20240403.0
workerd --version 1.20240403.0 2024-04-03

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

Copy link
Copy Markdown
Contributor

@petebacondarwin petebacondarwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is false the correct default option here for non-interactive?
I feel like it should actually fail... because otherwise if someone is expecting their CI to download and keep the wrangler.toml updated before deploying, and haven't specified --force it will just deploy the old wrangler.toml instead.

Comment thread .changeset/wise-grapes-prove.md Outdated
Comment thread packages/wrangler/src/__tests__/pages/pages-download-config.test.ts
penalosa and others added 2 commits April 4, 2024 12:30
Co-authored-by: Pete Bacon Darwin <pete@bacondarwin.com>
@penalosa penalosa requested a review from petebacondarwin April 4, 2024 11:36
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.17%. Comparing base (6c3be5b) to head (55f08a3).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5506      +/-   ##
==========================================
+ Coverage   72.09%   72.17%   +0.08%     
==========================================
  Files         331      331              
  Lines       17175    17181       +6     
  Branches     4386     4388       +2     
==========================================
+ Hits        12382    12401      +19     
+ Misses       4793     4780      -13     
Files Coverage Δ
packages/wrangler/src/pages/download-config.ts 98.71% <100.00%> (+0.10%) ⬆️

... and 7 files with indirect coverage changes

);
if (!overwrite) {
return;
throw new FatalError("Not overwriting existing `wrangler.toml` file");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this mean the user gets a fatal error if they choose not to overwrite in the interactive prompt? I guess that is kind of OK DX?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok for now. We can revisit if needed.

@penalosa penalosa merged commit 7734f80 into main Apr 4, 2024
@penalosa penalosa deleted the penalosa/add-pages-download-warning branch April 4, 2024 12:59
@workers-devprod workers-devprod mentioned this pull request Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants