-
Notifications
You must be signed in to change notification settings - Fork 1k
fix: Generalize autoconfig wording #11249
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
base: main
Are you sure you want to change the base?
fix: Generalize autoconfig wording #11249
Conversation
🦋 Changeset detectedLatest commit: e855561 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: |
petebacondarwin
left a comment
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.
No tests affected by this?
No, because these are just error messages that we were not testing before Also the I'm happy to add tests here if you'd like some 🙂 |
|
I'm not fussed about testing the actual logged messages, but wonder whether we should be testing the logic where we bail out of autoconfig under certain circumstances. |
Yeah good point, I think there are a couple of scenarios that are missing tests here, I can do that as a followup or here, whatever works best for you 🙂 |
|
Has rebase conflicts. |
22da68c to
622560b
Compare
|
Failed to automatically backport this PR's changes to Wrangler v3. Please manually create a PR targeting the Depending on your changes, running Notes:
|
|
@petebacondarwin I've added a test for the output directory error case 🙂 As I mentioned I am not sure if a test for the abort case is needed/correct right now, but I am happy to add one if you disagree 🙂 |
55b9ca9 to
417589a
Compare
|
|
||
| if (!(skipConfirmation || (await confirm("Proceed with setup?")))) { | ||
| throw new FatalError("Deployment aborted"); | ||
| throw new FatalError("Autoconfig process aborted"); |
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 don't think "autoconfig" is a user-facing term.
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.
Maybe "Setup cancelled"?
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.
Framework detection cancelled?
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.
Maybe "Setup cancelled"?
sounds good, updated 👍
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.
Framework detection cancelled?
But the project might be a static one not using a framework, no? 🤔
0376187 to
e855561
Compare
Fixes https://jira.cfdata.org/browse/DEVX-2314
This PR Generalizes the autoconfig wording so that when it doesn't specifically mention "deployment" (since it can be run via
wrangler setupor the autoconfig programmatic API)