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

Refactor environment parsing #534

Merged
merged 13 commits into from
Sep 9, 2019
Merged

Refactor environment parsing #534

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
dfdb094
Initial pass at refactoring parsing
EverlastingBugstopper Sep 6, 2019
244cc7f
Better error message for conflicting workers_dot_dev and route
EverlastingBugstopper Sep 6, 2019
ac7a1f1
Gabbi is literally a genius what the hell
EverlastingBugstopper Sep 6, 2019
4c1fa59
Add semicolons and update error message
EverlastingBugstopper Sep 6, 2019
9a0183d
Clippy
EverlastingBugstopper Sep 6, 2019
8ef1a5a
Separate private check and initial pass at Gabbi's comments
EverlastingBugstopper Sep 9, 2019
62db4da
rename zoneless check to negotiate_zoneless
EverlastingBugstopper Sep 9, 2019
ee5153b
Update warn messaging
EverlastingBugstopper Sep 9, 2019
2939279
Add deprecation version to messaging
EverlastingBugstopper Sep 9, 2019
ae8b25b
Separate long warning messages across multiple newlines
EverlastingBugstopper Sep 9, 2019
e4042ed
Update private messaging
EverlastingBugstopper Sep 9, 2019
8fea63e
change private single quotes to backticks
EverlastingBugstopper Sep 9, 2019
ea5b759
Add comment about not inheriting kv namespaces
EverlastingBugstopper Sep 9, 2019
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
2 changes: 1 addition & 1 deletion src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ fn run() -> Result<(), failure::Error> {
Arg::with_name("release")
.long("release")
.takes_value(false)
.help("[this will be deprecated, use --env instead]\nshould this be published to a workers.dev subdomain or a domain name you have registered"),
.help("[planned deprecation in v1.5.0, use --env instead. see https://github.com/cloudflare/wrangler/blob/master/docs/environments.md for more information]\nshould this be published to a workers.dev subdomain or a domain name you have registered"),
)
.arg(
Arg::with_name("env")
Expand Down
301 changes: 140 additions & 161 deletions src/settings/project/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,61 +85,45 @@ impl Manifest {
Ok(manifest)
}

pub fn get_target(
fn get_environment(
&self,
environment_name: Option<&str>,
release: bool,
) -> Result<Target, failure::Error> {
if release && self.workers_dot_dev.is_some() {
failure::bail!(
"The --release flag is not compatible with use of the workers_dot_dev field"
)
}

if release {
message::warn("--release will be deprecated");
}

let environment = match environment_name {
Some(environment_name) => match &self.env {
Some(environment_table) => {
let environment = environment_table.get(environment_name);
match environment {
Some(environment) => Some(environment),
None => failure::bail!(format!(
"{} Could not find environment with name {}",
emoji::WARN,
environment_name
)),
}
) -> Result<Option<&Environment>, failure::Error> {
// check for user-specified environment name
if let Some(environment_name) = environment_name {
EverlastingBugstopper marked this conversation as resolved.
Show resolved Hide resolved
if let Some(environment_table) = &self.env {
if let Some(environment) = environment_table.get(environment_name) {
Ok(Some(environment))
} else {
failure::bail!(format!(
"{} Could not find environment with name {}",
emoji::WARN,
environment_name
))
}
None => failure::bail!(format!(
} else {
failure::bail!(format!(
"{} There are no environments specified in your wrangler.toml",
emoji::WARN
)),
},
None => None,
};

let deprecate_private_warning = "The 'private' field is now considered deprecated; please use \
workers_dot_dev to toggle between publishing to your workers.dev subdomain and your own domain.";

// Check for the presence of the 'private' field in top-level config; if present, warn.
if self.private.is_some() {
message::warn(deprecate_private_warning);
}

// Also check for presence of 'private' field in a provided environment; if present, warn
if let Some(e) = environment {
if e.private.is_some() {
message::warn(deprecate_private_warning);
))
}
} else {
Ok(None)
}
}

// TODO: when --release is deprecated, this will be much easier
fn negotiate_zoneless(
&self,
environment: Option<&Environment>,
release: bool,
) -> Result<(Option<String>, bool), failure::Error> {
let use_dot_dev_failure =
"Please specify the workers_dot_dev boolean in the top level of your wrangler.toml.";
let use_dot_dev_warning =
"Please specify the workers_dot_dev boolean in the top level of your wrangler.toml";
format!("{}\n{} This command will fail in v1.5.0. Please see https://github.com/cloudflare/wrangler/blob/master/docs/environments.md for more information.", use_dot_dev_failure, emoji::WARN);
let wdd_failure = format!(
"{} Your environment should only include `workers_dot_dev` or `route`",
"{} Your environment should only include `workers_dot_dev` or `route`. If you are trying to publish to workers.dev, remove `route` from your wrangler.toml, if you are trying to publish to your own domain, remove `workers_dot_dev`.",
emoji::WARN
);

Expand All @@ -151,152 +135,147 @@ impl Manifest {
// top level configuration
None => {
if release {
// --release means zoned, not workers.dev
match self.workers_dot_dev {
Some(_) => failure::bail!(use_dot_dev_warning),
Some(_) => {
failure::bail!(format!("{} {}", emoji::WARN, use_dot_dev_failure))
}
None => {
message::warn(use_dot_dev_warning);
false // workers_dot_dev defaults to false when it's top level and --release is passed
message::warn(&use_dot_dev_warning);
false // wrangler publish --release w/o workers_dot_dev is zoned deploy
}
}
} else {
match self.workers_dot_dev {
Some(wdd) => {
if wdd {
match &self.route {
Some(route) => {
if !route.is_empty() {
failure::bail!(wdd_failure)
}
}
None => (),
}
} else if let Some(wdd) = self.workers_dot_dev {
if wdd {
if let Some(route) = &self.route {
if !route.is_empty() {
failure::bail!(wdd_failure)
}
wdd
}
None => {
message::warn(use_dot_dev_warning);
true // workers_dot_dev defaults to true when it's top level and --release is not passed
}
}
wdd
} else {
message::warn(&use_dot_dev_warning);
true // wrangler publish w/o workers_dot_dev is zoneless deploy
}
}

// environment configuration
Some(environment) => match environment.workers_dot_dev {
Some(wdd) => {
Some(environment) => {
if let Some(wdd) = environment.workers_dot_dev {
if wdd && environment.route.is_some() {
failure::bail!(wdd_failure)
}
wdd
}
None => {
match self.workers_dot_dev {
Some(wdd) => {
if wdd && environment.route.is_some() {
false // use route if workers_dot_dev = true is inherited
} else {
wdd // inherit from top level
}
}
None => false,
} else if let Some(wdd) = self.workers_dot_dev {
if wdd && environment.route.is_some() {
false // allow route to override workers_dot_dev = true if wdd is inherited
} else {
wdd // inherit from top level
}
} else {
false // if absent -> false
}
},
};

let kv_namespaces = match environment {
Some(environment) => match &environment.kv_namespaces {
Some(kv) => Some(kv.clone()),
None => None,
},
None => self.kv_namespaces.clone(),
};

let account_id = match environment {
Some(environment) => match &environment.account_id {
Some(a) => a.clone(),
None => self.account_id.clone(),
},
None => self.account_id.clone(),
}
};

let name = match environment {
Some(environment) => match &environment.name {
Some(name) => {
let name = name.clone();
if name == self.name {
failure::bail!(format!(
"{} Each `name` in your wrangler.toml must be unique",
emoji::WARN
))
let route = if let Some(environment) = environment {
EverlastingBugstopper marked this conversation as resolved.
Show resolved Hide resolved
if let Some(route) = &environment.route {
if let Some(wdd) = environment.workers_dot_dev {
if wdd {
failure::bail!(wdd_failure);
}
name
}
None => match environment_name {
Some(environment_name) => format!("{}-{}", self.name, environment_name),
None => failure::bail!("You must specify `name` in your wrangler.toml"),
},
},
None => self.name.clone(),
Some(route.clone())
} else {
None
}
} else {
self.route.clone()
};

let route = match environment {
Some(environment) => match &environment.route {
Some(route) => match environment.workers_dot_dev {
Some(wdd) => {
if wdd {
failure::bail!(wdd_failure);
} else {
Some(route.clone())
}
}
None => Some(route.clone()),
},
None => None,
},
None => self.route.clone(),
};
Ok((route, workers_dot_dev))
}

let routes = match environment {
Some(environment) => match &environment.routes {
Some(routes) => Some(routes.clone()),
None => None,
},
None => self.routes.clone(),
};
fn check_private(&self, environment: Option<&Environment>) {
let deprecate_private_warning = "The `private` field is deprecated; please use \
`workers_dot_dev` to toggle between publishing to your workers.dev subdomain and your own domain.";

let webpack_config = match environment {
Some(environment) => match &environment.webpack_config {
Some(webpack_config) => Some(webpack_config.clone()),
None => self.webpack_config.clone(),
},
None => self.webpack_config.clone(),
};
// Check for the presence of the 'private' field in top-level config; if present, warn.
if self.private.is_some() {
message::warn(deprecate_private_warning);
}

let zone_id = match environment {
Some(environment) => match &environment.zone_id {
Some(zone_id) => Some(zone_id.clone()),
None => self.zone_id.clone(),
},
None => self.zone_id.clone(),
};
// Also check for presence of 'private' field in a provided environment; if present, warn
if let Some(e) = environment {
if e.private.is_some() {
message::warn(deprecate_private_warning);
}
}
}

let project_type = self.project_type.clone();
pub fn get_target(
&self,
environment_name: Option<&str>,
release: bool,
) -> Result<Target, failure::Error> {
if release && self.workers_dot_dev.is_some() {
failure::bail!(format!(
"{} The --release flag is not compatible with use of the workers_dot_dev field.",
emoji::WARN
))
}

Ok(Target {
project_type, // MUST inherit
account_id, // MAY inherit
webpack_config, // MAY inherit
zone_id, // MAY inherit
workers_dot_dev, // MAY inherit,
if release {
message::warn("--release will be deprecated.");
}

let mut target = Target {
project_type: self.project_type.clone(), // MUST inherit
account_id: self.account_id.clone(), // MAY inherit
webpack_config: self.webpack_config.clone(), // MAY inherit
zone_id: self.zone_id.clone(), // MAY inherit
workers_dot_dev: true, // MAY inherit,
// importantly, the top level name will be modified
// to include the name of the environment
name, // MAY inherit
kv_namespaces, // MUST NOT inherit
route, // MUST NOT inherit
routes, // MUST NOT inherit
})
name: self.name.clone(), // MAY inherit
kv_namespaces: self.kv_namespaces.clone(), // MUST NOT inherit
route: None, // MUST NOT inherit
routes: self.routes.clone(), // MUST NOT inherit
};

let environment = self.get_environment(environment_name)?;

self.check_private(environment);

let (route, workers_dot_dev) = self.negotiate_zoneless(environment, release)?;
target.route = route;
target.workers_dot_dev = workers_dot_dev;
if let Some(environment) = environment {
target.name = if let Some(name) = &environment.name {
name.clone()
} else {
match environment_name {
Some(environment_name) => format!("{}-{}", self.name, environment_name),
None => failure::bail!("You must specify `name` in your wrangler.toml"),
}
};
if let Some(account_id) = &environment.account_id {
EverlastingBugstopper marked this conversation as resolved.
Show resolved Hide resolved
target.account_id = account_id.clone();
}
if environment.routes.is_some() {
target.routes = environment.routes.clone();
}
if environment.webpack_config.is_some() {
target.webpack_config = environment.webpack_config.clone();
}
if environment.zone_id.is_some() {
target.zone_id = environment.zone_id.clone();
}
// don't inherit kv namespaces because it is an anti-pattern to use the same namespaces across multiple environments
target.kv_namespaces = environment.kv_namespaces.clone();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to wrap environment.kv_namespaces in the same conditionals as above? I would think kv inheritance would work the same as other toml fields...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ashleymichal and I talked about this and said that it doesn't really make sense to inherit kv namespaces because it's an anti-pattern to use multiple namespaces across different environments. I don't want my staging worker to use my production kv namespaces. In this case - if you wanted to use the same namespace for both, you would just have to copy + paste to explicitly use the same namespaces for both environments. Open to discussion about this.

Copy link
Contributor

@gabbifish gabbifish Sep 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This makes sense. It could be worth adding this as a comment to clarify why this logic diverges from the rest of the logic preceding it. Other than that, LGTM + I will approve this!

}

Ok(target)
}

pub fn generate(
Expand Down