Skip to content
This repository has been archived by the owner on Aug 3, 2023. It is now read-only.

Feature: Multi-route support #916

Merged
merged 62 commits into from
Feb 6, 2020
Merged

Feature: Multi-route support #916

merged 62 commits into from
Feb 6, 2020

Conversation

ashleymichal
Copy link
Contributor

@ashleymichal ashleymichal commented Nov 27, 2019 •
edited
Loading

Addresses phase one of #866

This PR seeks to unblock users who require Wrangler to publish to multiple routes on a Cloudflare zone. It supports:

  • Specifying a list of route patterns in wrangler.toml for publish
  • a command to list the routes active on a given zone
  • a command to delete a route on a given zone

It does not support:

  • declaring "null worker" routes
  • "syncing" routing tables for a project

Testing this branch

see https://gist.github.com/ashleymichal/b1a3f0d881fed2544b17c3e7ee1ecc17 for testing instructions.

@ashleymichal ashleymichal added status - work in progress regression Something is broken, but works in previous releases labels Nov 27, 2019
@ashleymichal ashleymichal added this to the 1.7.0 milestone Nov 27, 2019
@ashleymichal ashleymichal requested a review from a team November 27, 2019 14:03
@ashleymichal ashleymichal self-assigned this Nov 27, 2019
@ashleymichal ashleymichal changed the title Feat multi route Feature: Multi-route support Nov 27, 2019
@ashleymichal
Move Route struct into settings
26eb615
@ashleymichal
Get route from target from within publish_route fn
6bdca72
@ashleymichal
Make routes a Vec<String> rather than HashMap
580f7b6
@ashleymichal
Add 'routes' method to Target
2649300
@ashleymichal
code review - extract failure case to early bail
acb0fcb
@ashleymichal
WIP
7996fb8
@ashleymichal
Merge pull request #918 from cloudflare/alewis/refactor-routes
1b99230 
Alewis/refactor routes
@ashleymichal
Revert "WIP"
e1b30b3 
This reverts commit 7996fb8.
@ashleymichal
Update cloudflare-rs version for routes endpoints
03607d5
@ashleymichal
Merge pull request #921 from cloudflare/alewis/update-cloudflare-rs-v…
c01293a 
…ersion

Update cloudflare-rs version for routes endpoints
@ashleymichal
WIP - Publish multiple routes, error on conflict
775cf15
@ashleymichal
refactor: split upload_script and deploy into two functions
868f94b
@ashleymichal
Refine multiroute publish handling
ec008c8 
* include (optional) id in Route struct
* remove unused Route::new() method
* include upload result information in output of routes deploy function
* split upload script and deploy into two functions
@ashleymichal
Merge pull request #922 from cloudflare/alewis/publish-multiple-routes
f64b55a 
Alewis/publish multiple routes
@ashleymichal
Code review improvements
99d3504 
* pull up test consts
* use FromStr trait in manifest
* use variable names for empty string arguments to test toml constructors
@ashleymichal
Merge pull request #967 from cloudflare/alewis/deploy-target-with-tests
6bfb173 
deploy target tests
@ashleymichal
Copy link
Contributor Author

this is only marked WIP because we are waiting to release 1.7.0 before merge

@EverlastingBugstopper
Copy link
Contributor

can we mark it as blocked instead of WIP if it is ready for review?

@ashleymichal ashleymichal changed the title [WIP] - Feature: Multi-route support Feature: Multi-route support Jan 22, 2020
EverlastingBugstopper
EverlastingBugstopper suggested changes Jan 22, 2020
View reviewed changes
Copy link
Contributor

@EverlastingBugstopper EverlastingBugstopper left a comment

Choose a reason for hiding this comment

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

requesting changes to prevent early merge

@EverlastingBugstopper EverlastingBugstopper mentioned this pull request Jan 27, 2020
Closed
@EverlastingBugstopper
Copy link
Contributor

EverlastingBugstopper commented Jan 27, 2020 •
edited
Loading

The tests seem to include a lot of compiler warnings of "this function is never used" which we need to really should address before merging

@ashleymichal
Copy link
Contributor Author

The tests seem to include a lot of compiler warnings of "this function is never used" which we need to really should address before merging

these are a result of the test fixture code being shared between the tests/ directory and the src tests. we can move all the deploy target tests to tests/ though, that might be better.

@EverlastingBugstopper
Copy link
Contributor

👍

@EverlastingBugstopper EverlastingBugstopper mentioned this pull request Feb 5, 2020
Merged
@ashleymichal ashleymichal merged commit 1a93246 into master Feb 6, 2020
@delete-merged-branch delete-merged-branch bot deleted the feat-multi-route branch February 6, 2020 22:00
@EverlastingBugstopper EverlastingBugstopper mentioned this pull request Feb 6, 2020
Closed
7 tasks
@AaronO
Copy link
Contributor

AaronO commented Feb 13, 2020 •
edited
Loading

@ashleygwilliams @EverlastingBugstopper I wanted to try the 1.8.0-rc.0 release, however the install fails (at least on macOS) with a TAR_BAD_ARCHIVE exception.

Checking the URL it fetches (https://workers.cloudflare.com/get-npm-wrangler-binary/1.8.0-rc.0/x86_64-apple-darwin), I realized that it's actually a .zip file (see the PK magic-number):

❯ curl https://workers.cloudflare.com/get-npm-wrangler-binary/1.8.0-rc.0/x86_64-apple-darwin 2>/dev/null | head -c2 | hexdump -C
00000000  50 4b                                             |PK|
00000002

Anychance you could re-upload the build artifacts as .tar.gz or make a corrective 1.8.0-rc.1 release ?

@ashleygwilliams
Copy link
Contributor

@AaronO yup! we noticed that as we did the release at the end of the day- we're hoping to fix it this morning :)

@lawred2
Copy link

lawred2 commented Feb 13, 2020

@ashleygwilliams @EverlastingBugstopper I wanted to try the 1.8.0-rc.0 release, however the install fails (at least on macOS) with a TAR_BAD_ARCHIVE exception.

Checking the URL it fetches (https://workers.cloudflare.com/get-npm-wrangler-binary/1.8.0-rc.0/x86_64-apple-darwin), I realized that it's actually a .zip file (see the PK magic-number):

❯ curl https://workers.cloudflare.com/get-npm-wrangler-binary/1.8.0-rc.0/x86_64-apple-darwin 2>/dev/null | head -c2 | hexdump -C
00000000  50 4b                                             |PK|
00000002

Anychance you could re-upload the build artifacts as .tar.gz or make a corrective 1.8.0-rc.1 release ?

fwiw - seeing the same on windows

@EverlastingBugstopper
Copy link
Contributor

Hey @AaronO and @lawred2 this should be fixed now 😄 We switched our CI provider which caused some issues for us (which is why we do beta releases 👍 )

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@EverlastingBugstopper EverlastingBugstopper EverlastingBugstopper approved these changes

Assignees

@ashleymichal ashleymichal

Labels
blocked regression Something is broken, but works in previous releases
Projects
None yet
Milestone
1.8.0-rc.0
Development

Successfully merging this pull request may close these issues.
5 participants
@ashleymichal @EverlastingBugstopper @AaronO @ashleygwilliams @lawred2