Handle duplicate route error gracefully #813
Conversation
pradovic
force-pushed
the
fix/duplicate-error-routes
branch
from
c65ca93
to
fab680f
Compare
There was a problem hiding this comment.
hey @pradovic ! thank you so much for filing this! i think it looks great! as for your ask re: tests, i think those would be fantastic and if you wanted you could add the first test for the condition you wrote this PR for! we could then do some more in another PR. how does that sound? thanks again for the thoughtful PR! we really appreciate it!
ashleygwilliams
added
status - waiting on response
and removed
status - needs review
labels
|
hey @ashleygwilliams, thanks for the super-fast and super-kind response! Specifically, it directly calls Much appreciated! |
|
@pradovic sounds like a plan to me! do you want to file an issue for the plan you have? i think my team would be interested in discussing it with you :) ... in the meantime, we should be able to get this in for the next release, 1.6.0- which is currently aiming to be released on nov 8! thanks again for contributing!! <3 |
|
@ashleygwilliams Sounds good to me! I will open an issue very soon! Thanks a lot! |
src/commands/publish/route.rs
Outdated
| let msg; | ||
| if res.status().as_u16() == 10020 { | ||
| msg = format!( | ||
| "{} There was an error creating your route. Worker with a different name was found on {} route.\n", |
There was a problem hiding this comment.
Hey @pradovic - thanks so much for this thoughtful PR. I have a minor suggestion for the wording of the error message to make it a bit more actionable. Let me know what you think!
| "{} There was an error creating your route. Worker with a different name was found on {} route.\n", | |
| "{} A worker with a different name was previously deployed to `{}`. If you would like to overwrite that worker, you will need to change `name` in your `wrangler.toml` to match the currently deployed worker, or navigate to https://dash.cloudflare.com/workers and rename or delete that worker.\n", |
There was a problem hiding this comment.
Hey @EverlastingBugstopper, thanks for the feedback, I was wondering how to phrase it better! Sounds good to me! I have updated the PR with suggested change.
…gler into fix/duplicate-error-routes
|
@pradovic Thanks for the PR! We're definitely looking at improving the testability of a lot of our codebase. One of my big initiatives for this quarter is to shift all of our API calls into cloudflare-rs, which provides a lovely mock api client struct for exactly these kinds of unit tests. i'll probably link to that initiative from this issue: #63, so keep an eye out if you'd like to help with that! |
ashleygwilliams
added
changelog - feature
and removed
status - waiting on response
labels
|
@ashleymichal Thanks a lot for the info, that sounds awesome! I will keep an eye on that one! |
Should fix #519. I took a really easy & straightforward approach here, as this is my first PR. Let me know if you guys would like it to be solved differently, and I will be more then happy to update the PR or create a new one.
🌷 Maybe it would be useful to add tests for
route.rs. Would be happy to do it.