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

Alewis/refactor routes #918

Merged
merged 6 commits into from
Dec 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ clap = "2.32.0"
config = "0.9.2"
console = "0.7.5"
dirs = "1.0.5"
cloudflare = "0.4.1"
cloudflare = { path = "/Users/ashleylewis/cloudflare-rs" }
env_logger = "0.6.1"
failure = "0.1.5"
log = "0.4.6"
Expand Down
8 changes: 3 additions & 5 deletions src/commands/publish/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ mod route;
pub mod upload_form;

pub use package::Package;
use route::{publish_route, Route};
use route::publish_route;

use std::env;
use std::path::Path;
Expand Down Expand Up @@ -101,17 +101,15 @@ fn publish_script(
.send()?;

let res_status = res.status();
let res_text = res.text()?;

if !res_status.is_success() {
let res_text = res.text()?;
failure::bail!(error_msg(res_status, res_text))
}

let pattern = if target.route.is_some() {
let route = Route::new(&target)?;
publish_route(&user, &target, &route)?;
log::info!("publishing to route");
route.pattern
publish_route(&user, &target)?
} else {
log::info!("publishing to subdomain");
publish_to_subdomain(target, user)?
Expand Down
61 changes: 10 additions & 51 deletions src/commands/publish/route.rs
Original file line number Diff line number Diff line change
@@ -1,64 +1,23 @@
use serde::{Deserialize, Serialize};

use cloudflare::endpoints::workers::{CreateRoute, CreateRouteParams, ListRoutes, WorkersRoute};
use cloudflare::endpoints::workers::{CreateRoute, CreateRouteParams, ListRoutes};
use cloudflare::framework::apiclient::ApiClient;
use cloudflare::framework::HttpApiClientConfig;

use crate::http::{api_client, format_error};
use crate::settings::global_user::GlobalUser;
use crate::settings::target::Target;
use crate::terminal::emoji;

#[derive(Deserialize, PartialEq, Serialize)]
pub struct Route {
script: Option<String>,
pub pattern: String,
}

impl From<&WorkersRoute> for Route {
fn from(api_route: &WorkersRoute) -> Route {
Route {
script: api_route.script.clone(),
pattern: api_route.pattern.clone(),
}
}
}

impl Route {
pub fn new(target: &Target) -> Result<Route, failure::Error> {
if target
.route
.clone()
.expect("You must provide a zone_id in your wrangler.toml before publishing!")
.is_empty()
{
failure::bail!("You must provide a zone_id in your wrangler.toml before publishing!");
}
let msg_config_error = format!(
"{} Your project config has an error, check your `wrangler.toml`: `route` must be provided.",
emoji::WARN
);
Ok(Route {
script: Some(target.name.to_string()),
pattern: target.route.clone().expect(&msg_config_error),
})
}
}
use crate::settings::target::{Route, Target};

pub fn publish_route(
user: &GlobalUser,
target: &Target,
route: &Route,
) -> Result<(), failure::Error> {
if route_exists(user, target, route)? {
Ok(())
pub fn publish_route(user: &GlobalUser, target: &Target) -> Result<String, failure::Error> {
let route = Route::new(&target)?;
if route_exists(user, target, &route)? {
Ok(route.pattern)
} else {
create(user, target, route)
create(user, target, &route)?;
Ok(route.pattern)
}
}

fn route_exists(user: &GlobalUser, target: &Target, route: &Route) -> Result<bool, failure::Error> {
let routes = get_routes(user, target)?;
let routes = fetch_all(user, target)?;

for remote_route in routes {
if remote_route == *route {
Expand All @@ -68,7 +27,7 @@ fn route_exists(user: &GlobalUser, target: &Target, route: &Route) -> Result<boo
Ok(false)
}

fn get_routes(user: &GlobalUser, target: &Target) -> Result<Vec<Route>, failure::Error> {
fn fetch_all(user: &GlobalUser, target: &Target) -> Result<Vec<Route>, failure::Error> {
let client = api_client(user, HttpApiClientConfig::default())?;

let routes: Vec<Route> = match client.request(&ListRoutes {
Expand Down
4 changes: 1 addition & 3 deletions src/settings/target/environment.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
use super::kv_namespace::KvNamespace;
use super::site::Site;

use std::collections::HashMap;

use serde::{Deserialize, Serialize};

#[derive(Clone, Debug, Deserialize, Serialize)]
Expand All @@ -11,7 +9,7 @@ pub struct Environment {
pub account_id: Option<String>,
pub workers_dev: Option<bool>,
pub route: Option<String>,
pub routes: Option<HashMap<String, String>>,
pub routes: Option<Vec<String>>,
pub zone_id: Option<String>,
pub webpack_config: Option<String>,
pub private: Option<bool>,
Expand Down
2 changes: 1 addition & 1 deletion src/settings/target/manifest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub struct Manifest {
pub workers_dev: Option<bool>,
#[serde(default = "some_string")]
pub route: Option<String>,
pub routes: Option<HashMap<String, String>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

interested in why this was a hashmap to begin with - i know we never used it anywhere but still interested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because technically routes are a map between a pattern and a script, and it is potentially useful within a Wrangler project if you wanted to "negate" a route by setting it to a null worker. For the purposes of this refactor, I am starting with the simplest implementation - a list of patterns - to reach parity with serverless.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense - will this bite us at all when we try to migrate? will the future require breaking changes to the toml? would like to avoid that as much as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

predicting the future of routes is about as easy as predicting next year's weather (probably will be hotter than today, but who knows if it'll be raining). That being said, if it is clear before release that we need this data structure to change we can do that.

pub routes: Option<Vec<String>>,
#[serde(default = "some_string")]
pub zone_id: Option<String>,
pub webpack_config: Option<String>,
Expand Down
2 changes: 2 additions & 0 deletions src/settings/target/mod.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
mod environment;
mod kv_namespace;
mod manifest;
mod route;
mod site;
mod target;
mod target_type;

pub use environment::Environment;
pub use kv_namespace::KvNamespace;
pub use manifest::Manifest;
pub use route::Route;
pub use site::Site;
pub use target::Target;
pub use target_type::TargetType;
Expand Down
42 changes: 42 additions & 0 deletions src/settings/target/route.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
use serde::{Deserialize, Serialize};

use cloudflare::endpoints::workers::WorkersRoute;

use crate::settings::target::target::Target;
use crate::terminal::emoji;

#[derive(Deserialize, PartialEq, Serialize)]
pub struct Route {
pub script: Option<String>,
pub pattern: String,
}

impl From<&WorkersRoute> for Route {
fn from(api_route: &WorkersRoute) -> Route {
Route {
script: api_route.script.clone(),
pattern: api_route.pattern.clone(),
}
}
}

impl Route {
pub fn new(target: &Target) -> Result<Route, failure::Error> {
if target
.route
.clone()
.expect("You must provide a zone_id in your wrangler.toml before publishing!")
.is_empty()
{
failure::bail!("You must provide a zone_id in your wrangler.toml before publishing!");
}
let msg_config_error = format!(
"{} Your project config has an error, check your `wrangler.toml`: `route` must be provided.",
emoji::WARN
);
Ok(Route {
script: Some(target.name.to_string()),
pattern: target.route.clone().expect(&msg_config_error),
})
}
}
32 changes: 29 additions & 3 deletions src/settings/target/target.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use super::kv_namespace::KvNamespace;

use super::site::Site;
use super::target_type::TargetType;
use super::Route;

use std::collections::HashMap;
use std::env;

use std::path::PathBuf;
Expand All @@ -19,7 +18,7 @@ pub struct Target {
#[serde(rename = "type")]
pub target_type: TargetType,
pub route: Option<String>,
pub routes: Option<HashMap<String, String>>,
pub routes: Option<Vec<String>>,
pub webpack_config: Option<String>,
pub zone_id: Option<String>,
pub site: Option<Site>,
Expand Down Expand Up @@ -47,4 +46,31 @@ impl Target {
}
}
}

pub fn routes(&self) -> Result<Vec<Route>, failure::Error> {
let mut routes = Vec::new();

// we should assert that only one of the two keys is specified in the user's toml.
if self.route.is_some() && self.routes.is_some() {
failure::bail!("You can specify EITHER `route` or `routes` in your wrangler.toml");
}

// everything outside of this module should consider `target.routes()` to be a Vec;
// the fact that you can specify singular or plural is a detail of the wrangler.toml contract.
if let Some(single_route) = &self.route {
routes.push(Route {
script: Some(self.name.to_owned()),
pattern: single_route.to_string(),
});
} else if let Some(multi_route) = &self.routes {
for pattern in multi_route {
routes.push(Route {
script: Some(self.name.to_owned()),
pattern: pattern.to_string(),
});
}
}

Ok(routes)
}
}