Split upload and delete file around worker deploy

* remove unused KvNamespace bucket field
* return list of files to upload, files to delete from kv::bucket::sync
* call upload, delete files directly from publish, preview

src/commands/kv/bucket/mod.rs

@@ -6,6 +6,7 @@ mod upload;
 
 pub use manifest::AssetManifest;
 pub use sync::sync;
+pub use upload::upload_files;
 
 use std::ffi::OsString;
 use std::fs;

src/commands/kv/bucket/sync.rs

@@ -8,8 +8,6 @@ use cloudflare::endpoints::workerskv::write_bulk::KeyValuePair;
 use crate::commands::kv;
 use crate::commands::kv::bucket::directory_keys_only;
 use crate::commands::kv::bucket::directory_keys_values;
-use crate::commands::kv::bucket::upload::upload_files;
-use crate::commands::kv::bulk::delete::delete_bulk;
 use crate::commands::kv::key::KeyList;
 use crate::settings::global_user::GlobalUser;
 use crate::settings::toml::Target;
@@ -23,10 +21,10 @@ pub fn sync(
     namespace_id: &str,
     path: &Path,
     verbose: bool,
-) -> Result<AssetManifest, failure::Error> {
+) -> Result<(Vec<KeyValuePair>, Vec<String>, AssetManifest), failure::Error> {
     kv::validate_target(target)?;
-    // First, upload all changed files in given local directory (aka replace files
-    // in Workers KV that are now stale).
+    // First, find all changed files in given local directory (aka files that are now stale
+    // in Workers KV).
 
     // Get remote keys, which contain the hash of the file (value) as the suffix.
     // Turn it into a HashSet. This will be used by upload() to figure out which
@@ -42,17 +40,11 @@ pub fn sync(
             Err(e) => failure::bail!(kv::format_error(e)),
         }
     }
-    // First, upload all existing files in given directory
-    if verbose {
-        message::info("Preparing to upload updated files...");
-    }
 
-    let (mut pairs, asset_manifest): (Vec<KeyValuePair>, AssetManifest) =
+    let (pairs, asset_manifest): (Vec<KeyValuePair>, AssetManifest) =
         directory_keys_values(target, path, verbose)?;
 
-    pairs = filter_files(pairs, &remote_keys);
-
-    upload_files(target, &user, namespace_id, pairs)?;
+    let to_upload = filter_files(pairs, &remote_keys);
 
     // Now delete files from Workers KV that exist in remote but no longer exist locally.
     // Get local keys
@@ -65,20 +57,13 @@ pub fn sync(
 
     // Find keys that are present in remote but not present in local, and
     // stage them for deletion.
-    let keys_to_delete: Vec<_> = remote_keys
+    let to_delete: Vec<_> = remote_keys
         .difference(&local_keys)
         .map(|key| key.to_owned())
         .collect();
 
-    if !keys_to_delete.is_empty() {
-        if verbose {
-            message::info("Deleting stale files...");
-        }
-        delete_bulk(target, user, namespace_id, keys_to_delete)?;
-    }
-
     message::success("Success");
-    Ok(asset_manifest)
+    Ok((to_upload, to_delete, asset_manifest))
 }
 
 fn filter_files(pairs: Vec<KeyValuePair>, already_uploaded: &HashSet<String>) -> Vec<KeyValuePair> {

src/commands/kv/mod.rs

@@ -133,12 +133,10 @@ mod tests {
                 KvNamespace {
                     id: "fake".to_string(),
                     binding: "KV".to_string(),
-                    bucket: None,
                 },
                 KvNamespace {
                     id: "fake".to_string(),
                     binding: "KV".to_string(),
-                    bucket: None,
                 },
             ]),
             name: "test-target".to_string(),

src/commands/preview/upload.rs

@@ -1,12 +1,16 @@
-use crate::commands::kv::bucket::AssetManifest;
+use std::path::Path;
+
+use reqwest::blocking::Client;
+use serde::Deserialize;
+
+use crate::commands::kv::bucket::{sync, upload_files, AssetManifest};
+use crate::commands::kv::bulk::delete::delete_bulk;
 use crate::commands::publish;
 use crate::http;
 use crate::settings::global_user::GlobalUser;
 use crate::settings::toml::Target;
 use crate::terminal::message;
 use crate::upload;
-use reqwest::blocking::Client;
-use serde::Deserialize;
 
 use console::style;
 
@@ -57,11 +61,32 @@ pub fn upload(
                 let client = http::auth_client(None, &user);
 
                 if let Some(site_config) = target.site.clone() {
-                    publish::bind_static_site_contents(user, target, &site_config, true)?;
-                }
+                    let site_namespace = publish::add_site_namespace(user, target, true)?;
 
-                let asset_manifest = publish::upload_static_site_content(target, user, verbose)?;
-                authenticated_upload(&client, &target, asset_manifest)?
+                    let path = Path::new(&site_config.bucket);
+                    let (to_upload, to_delete, asset_manifest) =
+                        sync(target, user, &site_namespace.id, path, verbose)?;
+
+                    // First, upload all existing files in given directory
+                    if verbose {
+                        message::info("Uploading updated files...");
+                    }
+
+                    upload_files(target, user, &site_namespace.id, to_upload)?;
+
+                    let preview = authenticated_upload(&client, &target, Some(asset_manifest))?;
+                    if !to_delete.is_empty() {
+                        if verbose {
+                            message::info("Deleting stale files...");
+                        }
+
+                        delete_bulk(target, user, &site_namespace.id, to_delete)?;
+                    }
+
+                    preview
+                } else {
+                    authenticated_upload(&client, &target, None)?
+                }
             } else {
                 message::warn(&format!(
                     "Your wrangler.toml is missing the following fields: {:?}",

src/commands/publish.rs

@@ -1,12 +1,13 @@
 use std::env;
-use std::path::Path;
+use std::path::{Path, PathBuf};
 
 use crate::commands;
 use crate::commands::kv;
-use crate::commands::kv::bucket::AssetManifest;
+use crate::commands::kv::bucket::{sync, upload_files};
+use crate::commands::kv::bulk::delete::delete_bulk;
 use crate::deploy;
 use crate::settings::global_user::GlobalUser;
-use crate::settings::toml::{DeployConfig, KvNamespace, Site, Target};
+use crate::settings::toml::{DeployConfig, KvNamespace, Target};
 use crate::terminal::{emoji, message};
 use crate::upload;
 
@@ -18,20 +19,42 @@ pub fn publish(
 ) -> Result<(), failure::Error> {
     validate_target_required_fields_present(target)?;
 
-    // TODO: write a separate function for publishing a site
-    if let Some(site_config) = &target.site.clone() {
+    // Build the script before uploading.
+    commands::build(&target)?;
+
+    if let Some(site_config) = &target.site {
+        let path = &site_config.bucket.clone();
+        assert_non_root_bucket(path)?;
         warn_site_incompatible_route(&deploy_config);
-        bind_static_site_contents(user, target, &site_config, false)?;
-    }
+        let site_namespace = add_site_namespace(user, target, false)?;
 
-    let asset_manifest = upload_static_site_content(target, user, verbose)?;
+        let (to_upload, to_delete, asset_manifest) =
+            sync(target, user, &site_namespace.id, &path, verbose)?;
 
-    // Build the script before uploading.
-    commands::build(&target)?;
+        // First, upload all existing files in given directory
+        if verbose {
+            message::info("Preparing to upload updated files...");
+        }
+        upload_files(target, user, &site_namespace.id, to_upload)?;
+
+        // Next, upload and deploy the worker with the updated asset_manifest
+        upload::script(&user, &target, Some(asset_manifest))?;
 
-    upload::script(&user, &target, asset_manifest)?;
+        deploy::worker(&user, &deploy_config)?;
 
-    deploy::worker(&user, &deploy_config)?;
+        // Finally, remove any stale files
+        if !to_delete.is_empty() {
+            if verbose {
+                message::info("Deleting stale files...");
+            }
+
+            delete_bulk(target, user, &site_namespace.id, to_delete)?;
+        }
+    } else {
+        upload::script(&user, &target, None)?;
+
+        deploy::worker(&user, &deploy_config)?;
+    }
 
     Ok(())
 }
@@ -57,76 +80,57 @@ fn warn_site_incompatible_route(deploy_config: &DeployConfig) {
 }
 
 // Updates given Target with kv_namespace binding for a static site assets KV namespace.
-pub fn bind_static_site_contents(
+pub fn add_site_namespace(
     user: &GlobalUser,
     target: &mut Target,
-    site_config: &Site,
     preview: bool,
-) -> Result<(), failure::Error> {
+) -> Result<KvNamespace, failure::Error> {
     let site_namespace = kv::namespace::site(target, &user, preview)?;
 
     // Check if namespace already is in namespace list
     for namespace in target.kv_namespaces() {
         if namespace.id == site_namespace.id {
-            return Ok(()); // Sites binding already exists; ignore
+            return Ok(namespace); // Sites binding already exists; ignore
         }
     }
 
-    target.add_kv_namespace(KvNamespace {
+    let site_namespace = KvNamespace {
         binding: "__STATIC_CONTENT".to_string(),
         id: site_namespace.id,
-        bucket: Some(site_config.bucket.to_owned()),
-    });
-    Ok(())
+    };
+
+    target.add_kv_namespace(site_namespace.clone());
+
+    Ok(site_namespace)
 }
 
-pub fn upload_static_site_content(
-    target: &Target,
-    user: &GlobalUser,
-    verbose: bool,
-) -> Result<Option<AssetManifest>, failure::Error> {
-    let mut asset_manifest = None;
-    if let Some(site_namespace) = target.kv_namespaces().iter().find(|ns| ns.bucket.is_some()) {
-        if let Some(bucket) = &site_namespace.bucket {
-            // We don't want folks setting their bucket to the top level directory,
-            // which is where wrangler commands are always called from.
-            let current_dir = env::current_dir()?;
-            if bucket.as_os_str() == current_dir {
-                failure::bail!(
-                    "{} You need to specify a bucket directory in your wrangler.toml",
-                    emoji::WARN
-                )
-            }
-            let path = Path::new(&bucket);
-            if !path.exists() {
-                failure::bail!(
-                    "{} bucket directory \"{}\" does not exist",
-                    emoji::WARN,
-                    path.display()
-                )
-            } else if !path.is_dir() {
-                failure::bail!(
-                    "{} bucket \"{}\" is not a directory",
-                    emoji::WARN,
-                    path.display()
-                )
-            }
-            let manifest_result =
-                kv::bucket::sync(target, user, &site_namespace.id, path, verbose)?;
-            if target.site.is_some() {
-                if asset_manifest.is_none() {
-                    asset_manifest = Some(manifest_result)
-                } else {
-                    // only site manifest should be returned
-                    unreachable!()
-                }
-            }
-        } else {
-            unreachable!()
-        }
+// We don't want folks setting their bucket to the top level directory,
+// which is where wrangler commands are always called from.
+pub fn assert_non_root_bucket(bucket: &PathBuf) -> Result<(), failure::Error> {
+    // TODO: this should really use a convenience function for "Wrangler Project Root"
+    let current_dir = env::current_dir()?;
+    if bucket.as_os_str() == current_dir {
+        failure::bail!(
+            "{} You need to specify a bucket directory in your wrangler.toml",
+            emoji::WARN
+        )
+    }
+    let path = Path::new(&bucket);
+    if !path.exists() {
+        failure::bail!(
+            "{} bucket directory \"{}\" does not exist",
+            emoji::WARN,
+            path.display()
+        )
+    } else if !path.is_dir() {
+        failure::bail!(
+            "{} bucket \"{}\" is not a directory",
+            emoji::WARN,
+            path.display()
+        )
     }
 
-    Ok(asset_manifest)
+    Ok(())
 }
 
 fn validate_target_required_fields_present(target: &Target) -> Result<(), failure::Error> {

src/settings/toml/kv_namespace.rs

@@ -1,5 +1,4 @@
 use std::fmt;
-use std::path::PathBuf;
 
 use serde::{Deserialize, Serialize};
 
@@ -9,7 +8,6 @@ use crate::settings::binding::Binding;
 pub struct KvNamespace {
     pub id: String,
     pub binding: String,
-    pub bucket: Option<PathBuf>,
 }
 
 impl fmt::Display for KvNamespace {

src/settings/toml/tests/mod.rs

@@ -48,12 +48,10 @@ fn it_builds_from_environments_config_with_kv() {
     let kv_1 = KvNamespace {
         id: "somecrazylongidentifierstring".to_string(),
         binding: "prodKV-1".to_string(),
-        bucket: None,
     };
     let kv_2 = KvNamespace {
         id: "anotherwaytoolongidstring".to_string(),
         binding: "prodKV-2".to_string(),
-        bucket: None,
     };
 
     match target.kv_namespaces {
@@ -69,12 +67,10 @@ fn it_builds_from_environments_config_with_kv() {
     let kv_1 = KvNamespace {
         id: "somecrazylongidentifierstring".to_string(),
         binding: "stagingKV-1".to_string(),
-        bucket: None,
     };
     let kv_2 = KvNamespace {
         id: "anotherwaytoolongidstring".to_string(),
         binding: "stagingKV-2".to_string(),
-        bucket: None,
     };
     match target.kv_namespaces {
         Some(kv_namespaces) => {