From c7c5997ea9dd763def8ffc06edebf1a9dca11a33 Mon Sep 17 00:00:00 2001 From: "malonso@cloudflare.com" Date: Tue, 29 Jun 2021 09:22:36 -0500 Subject: [PATCH 1/2] add back in value names for rename and transfer class lost during migration to structopt --- src/cli/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 49f768498..e02dd078f 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -255,12 +255,12 @@ pub struct AdhocMigration { delete_class: Vec, /// Rename a durable object class - #[structopt(name = "rename-class", long, number_of_values = 2)] + #[structopt(name = "rename-class", long, number_of_values = 2, value_names(&["from class", "to class"]))] rename_class: Vec, /// Transfer all durable objects associated with a class in another script to a class in /// this script - #[structopt(name = "transfer-class", long, number_of_values = 3)] + #[structopt(name = "transfer-class", long, number_of_values = 3, value_names(&["from script", "from class", "to class"]))] transfer_class: Vec, } From 9c55dd308e2ae9d2f6c4a1182d66c93fd15adaf5 Mon Sep 17 00:00:00 2001 From: "malonso@cloudflare.com" Date: Mon, 12 Jul 2021 16:59:13 -0500 Subject: [PATCH 2/2] Implement tagged adhoc migrations, and tagged migrations in wrangler.toml --- src/cli/mod.rs | 46 +++-- src/cli/publish.rs | 8 +- src/commands/publish.rs | 56 ++++- src/settings/toml/manifest.rs | 7 +- src/settings/toml/migrations.rs | 352 ++++++++++++++++++++++++++++++-- src/upload/form/mod.rs | 2 +- 6 files changed, 430 insertions(+), 41 deletions(-) diff --git a/src/cli/mod.rs b/src/cli/mod.rs index e02dd078f..fe84c0e91 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -36,7 +36,7 @@ use std::path::PathBuf; use crate::commands::dev::Protocol; use crate::preview::HttpMethod; use crate::settings::toml::migrations::{ - DurableObjectsMigration, Migration, MigrationConfig, Migrations, RenameClass, TransferClass, + DurableObjectsMigration, Migration, MigrationTag, Migrations, RenameClass, TransferClass, }; use crate::settings::toml::TargetType; @@ -262,10 +262,18 @@ pub struct AdhocMigration { /// this script #[structopt(name = "transfer-class", long, number_of_values = 3, value_names(&["from script", "from class", "to class"]))] transfer_class: Vec, + + /// Specify the existing migration tag for the script. + #[structopt(name = "old-tag", long)] + old_tag: Option, + + /// Specify the new migration tag for the script + #[structopt(name = "new-tag", long)] + new_tag: Option, } impl AdhocMigration { - pub fn into_migration_config(self) -> Option { + pub fn into_migrations(self) -> Option { let migration = DurableObjectsMigration { new_classes: self.new_class, deleted_classes: self.delete_class, @@ -305,12 +313,20 @@ impl AdhocMigration { && migration.renamed_classes.is_empty() && migration.transferred_classes.is_empty(); - if !is_migration_empty { - Some(MigrationConfig { - tag: None, - migration: Migration { + if !is_migration_empty || self.old_tag.is_some() || self.new_tag.is_some() { + let migration = if !is_migration_empty { + Some(Migration { durable_objects: migration, - }, + }) + } else { + None + }; + + Some(Migrations::Adhoc { + script_tag: MigrationTag::Unknown, + provided_old_tag: self.old_tag, + new_tag: self.new_tag, + migration, }) } else { None @@ -342,6 +358,10 @@ mod tests { let command = Cli::from_iter(&[ "wrangler", "publish", + "--old-tag", + "oldTag", + "--new-tag", + "newTag", "--new-class", "newA", "--new-class", @@ -369,17 +389,19 @@ mod tests { if let Command::Publish { migration, .. } = command { assert_eq!( - migration.into_migration_config(), - Some(MigrationConfig { - tag: None, - migration: Migration { + migration.into_migrations(), + Some(Migrations::Adhoc { + script_tag: MigrationTag::Unknown, + provided_old_tag: Some(String::from("oldTag")), + new_tag: Some(String::from("newTag")), + migration: Some(Migration { durable_objects: DurableObjectsMigration { new_classes: vec![String::from("newA"), String::from("newB")], deleted_classes: vec![String::from("deleteA"), String::from("deleteB")], renamed_classes: vec![rename_class("A"), rename_class("B")], transferred_classes: vec![transfer_class("A"), transfer_class("B")], } - } + }) }) ); } else { diff --git a/src/cli/publish.rs b/src/cli/publish.rs index 81b3001e7..7b7bdcb65 100644 --- a/src/cli/publish.rs +++ b/src/cli/publish.rs @@ -1,5 +1,5 @@ +use super::AdhocMigration; use super::Cli; -use super::{AdhocMigration, Migrations}; use crate::commands; use crate::settings::{global_user::GlobalUser, toml::Manifest}; use crate::terminal::message::{Message, Output, StdOut}; @@ -30,10 +30,8 @@ pub fn publish( let manifest = Manifest::new(&cli_params.config)?; let mut target = manifest.get_target(cli_params.environment.as_deref(), false)?; - if let Some(migration) = migration.into_migration_config() { - target.migrations = Some(Migrations { - migrations: vec![migration], - }); + if let Some(migration) = migration.into_migrations() { + target.migrations = Some(migration); } let output = if output.as_deref() == Some("json") { diff --git a/src/commands/publish.rs b/src/commands/publish.rs index 18c789148..e15950360 100644 --- a/src/commands/publish.rs +++ b/src/commands/publish.rs @@ -3,6 +3,7 @@ use std::path::Path; use anyhow::Result; use indicatif::{ProgressBar, ProgressStyle}; +use reqwest::blocking::Client; use serde::{Deserialize, Serialize}; use crate::build::build_target; @@ -10,6 +11,7 @@ use crate::deploy::{self, DeploymentSet}; use crate::http::{self, Feature}; use crate::kv::bulk; use crate::settings::global_user::GlobalUser; +use crate::settings::toml::migrations::{MigrationTag, Migrations}; use crate::settings::toml::Target; use crate::sites; use crate::terminal::emoji; @@ -50,11 +52,21 @@ pub fn publish( Err(e) => Err(e), }?; - // We verify early here, so we don't perform pre-upload tasks if the upload will fail if let Some(build_config) = &target.build { build_config.verify_upload_dir()?; } + if target.migrations.is_some() { + // Can't do this in the if below, since that one takes a mutable borrow on target + let client = http::legacy_auth_client(user); + let script_migration_tag = get_migration_tag(&client, target)?; + + match target.migrations.as_mut().unwrap() { + Migrations::Adhoc { script_tag, .. } => *script_tag = script_migration_tag, + Migrations::List { script_tag, .. } => *script_tag = script_migration_tag, + }; + } + if let Some(site_config) = &target.site { let path = &site_config.bucket.clone(); validate_bucket_location(path)?; @@ -216,3 +228,45 @@ fn validate_target_required_fields_present(target: &Target) -> Result<()> { Ok(()) } + +fn get_migration_tag(client: &Client, target: &Target) -> Result { + // Today, the easiest way to get metadata about a script (including the migration tag) + // is the list endpoint, as the individual script endpoint just returns the source code for a + // given script (and doesn't work at all for DOs). Once we add an individual script metadata + // endpoint, we could use that here instead of listing all of the scripts. Listing isn't too bad + // today though, as most accounts are limited to 30 scripts anyways. + + let addr = format!( + "https://api.cloudflare.com/client/v4/accounts/{}/workers/scripts", + target.account_id.load()? + ); + + let res: ListScriptsV4ApiResponse = client.get(&addr).send()?.json()?; + + let tag = match res.result.into_iter().find(|s| s.id == target.name) { + Some(ScriptResponse { + migration_tag: Some(tag), + .. + }) => MigrationTag::HasTag(tag), + Some(ScriptResponse { + migration_tag: None, + .. + }) => MigrationTag::NoTag, + None => MigrationTag::NoScript, + }; + + log::info!("Current MigrationTag: {:#?}", tag); + + Ok(tag) +} + +#[derive(Debug, Deserialize)] +struct ListScriptsV4ApiResponse { + pub result: Vec, +} + +#[derive(Debug, Deserialize)] +struct ScriptResponse { + pub id: String, + pub migration_tag: Option, +} diff --git a/src/settings/toml/manifest.rs b/src/settings/toml/manifest.rs index ec88dfb33..748b71e8c 100644 --- a/src/settings/toml/manifest.rs +++ b/src/settings/toml/manifest.rs @@ -11,6 +11,7 @@ use once_cell::sync::OnceCell; use serde::{Deserialize, Serialize}; use serde_with::rust::string_empty_as_none; +use super::migrations::{MigrationConfig, MigrationTag, Migrations}; use super::UsageModel; use crate::commands::whoami::fetch_accounts; use crate::commands::{validate_worker_name, whoami, DEFAULT_CONFIG_PATH}; @@ -61,6 +62,7 @@ pub struct Manifest { pub wasm_modules: Option>, pub triggers: Option, pub durable_objects: Option, + pub migrations: Option>, #[serde(default, with = "string_empty_as_none")] pub usage_model: Option, } @@ -346,7 +348,10 @@ impl Manifest { name: self.name.clone(), // Inherited kv_namespaces: get_namespaces(self.kv_namespaces.clone(), preview)?, // Not inherited durable_objects: self.durable_objects.clone(), // Not inherited - migrations: None, // TODO(soon) Allow migrations in wrangler.toml + migrations: self.migrations.as_ref().map(|migrations| Migrations::List { + script_tag: MigrationTag::Unknown, + migrations: migrations.clone(), + }), // Top Level site: self.site.clone(), // Inherited vars: self.vars.clone(), // Not inherited text_blobs: self.text_blobs.clone(), // Inherited diff --git a/src/settings/toml/migrations.rs b/src/settings/toml/migrations.rs index ed26ed149..d80997fc0 100644 --- a/src/settings/toml/migrations.rs +++ b/src/settings/toml/migrations.rs @@ -1,44 +1,175 @@ +use std::collections::HashSet; + use serde::{Deserialize, Serialize}; -#[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)] -pub struct Migrations { - pub migrations: Vec, +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub enum MigrationTag { + HasTag(String), + NoTag, + NoScript, + Unknown, +} + +#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] +pub enum Migrations { + Adhoc { + // The actual existing migration tag for the script, filled in by + // the publish command before form building. Used to validate provided_old_tag + script_tag: MigrationTag, + // The expected migration tag for the script if provided on the command line using --old-tag. + provided_old_tag: Option, + // The new migration tag for the script if provided on the command line using --new-tag. + new_tag: Option, + // The actual migration, if any directives are provided + migration: Option, + }, + List { + // The actual existing migration tag for the script, filled in by + // the publish command before form building + script_tag: MigrationTag, + migrations: Vec, + }, } impl Migrations { - pub fn api_migration(&self) -> Result { - // TODO: make api call to get most recent tag, and coalesce tags afterwards. - // For now, migrations will only ever have a single adhoc migration in it. - match &self.migrations { - migrations if migrations.len() == 1 => Ok(ApiMigration { - old_tag: None, + fn validate(&self) -> Result<(), anyhow::Error> { + // validate migration tags + match &self { + Migrations::Adhoc { + script_tag: MigrationTag::HasTag(script_tag), + provided_old_tag: Some(provided_tag), + .. + } if provided_tag != script_tag => anyhow::bail!( + "The migration tag provided with your migration (\"{}\") does not match the script's current migration tag of \"{}\", \ + please check that you are applying the correct migration", provided_tag, script_tag + ), + Migrations::Adhoc { + script_tag: MigrationTag::NoTag, + provided_old_tag: Some(_), + .. + } => anyhow::bail!("You provided an existing migration tag with your migration but the script does not have a migration tag set."), + Migrations::Adhoc { + script_tag: MigrationTag::NoScript, + provided_old_tag: Some(_), + .. + } => anyhow::bail!("You provided an existing migration tag with your migration but the script does not exist."), + Migrations::Adhoc { + script_tag: MigrationTag::HasTag(existing_tag), + provided_old_tag: None, + .. + } => anyhow::bail!("You didn't provide an existing tag with your migration, but the script currently has a migration tag set to \"{}\"", existing_tag), + Migrations::Adhoc { + provided_old_tag: Some(_), new_tag: None, - migration: migrations[0].migration.clone(), - }), - migrations => Self::coalesce_migrations(migrations), - } + .. + } => anyhow::bail!("You must provide a --new-tag when --old-tag is set."), + Migrations::Adhoc { + script_tag: MigrationTag::Unknown, .. + } => anyhow::bail!("The migration tag for the script was never fetched. This is a bug in wrangler, and we'd appreciate an issue report!"), + Migrations::List { + script_tag: MigrationTag::Unknown, .. + } => anyhow::bail!("The migration tag for the script was never fetched. This is a bug in wrangler, and we'd appreciate an issue report!"), + _ => () + }; + + // validate migration list + if let Migrations::List { migrations, .. } = &self { + let mut seen = HashSet::new(); + for migration in migrations { + if seen.contains(&migration.tag) { + anyhow::bail!("The migration tag \"{}\" appears multiple times in the list of migrations in your wrangler.toml", migration.tag); + } + seen.insert(&migration.tag); + } + }; + + Ok(()) } - fn coalesce_migrations(_migrations: &[MigrationConfig]) -> Result { - unimplemented!() + pub fn api_migration(&self) -> Result, anyhow::Error> { + self.validate()?; + match &self { + Migrations::Adhoc { + provided_old_tag, + new_tag, + migration, + .. + } => { + let api_migration = ApiMigration { + old_tag: provided_old_tag.to_owned(), + new_tag: new_tag.to_owned(), + steps: migration + .as_ref() + .map_or_else(Vec::new, |m| vec![m.clone()]), + }; + + log::info!("API Migration: {:#?}", api_migration); + + Ok(Some(api_migration)) + } + Migrations::List { + script_tag, + migrations, + } => { + let mut migrations = migrations.clone(); + let migrations = match script_tag { + MigrationTag::HasTag(tag) => { + let position = + migrations + .iter() + .position(|m| &m.tag == tag) + .ok_or_else(|| { + anyhow::format_err!( + "The script's current migration tag of \"{}\" was not \ + found in the list of migrations in wrangler.toml", + tag + ) + })?; + migrations.drain(0..=position); + migrations + } + _ => migrations, + }; + + if migrations.is_empty() { + return Ok(None); + } + + let old_tag = match script_tag { + MigrationTag::HasTag(old_tag) => Some(old_tag.clone()), + _ => None, + }; + let new_tag = migrations.last().map(|m| m.tag.clone()); + let steps = migrations.into_iter().map(|m| m.migration).collect(); + + let api_migration = ApiMigration { + old_tag, + new_tag, + steps, + }; + + log::info!("API Migration: {:#?}", api_migration); + + Ok(Some(api_migration)) + } + } } } #[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)] pub struct MigrationConfig { - pub tag: Option, + pub tag: String, #[serde(flatten)] pub migration: Migration, } -#[derive(Clone, Debug, Default, PartialEq, Serialize)] +#[derive(Clone, Debug, PartialEq, Serialize)] pub struct ApiMigration { #[serde(skip_serializing_if = "Option::is_none")] - pub old_tag: Option, + old_tag: Option, #[serde(skip_serializing_if = "Option::is_none")] - pub new_tag: Option, - #[serde(flatten)] - pub migration: Migration, + new_tag: Option, + steps: Vec, } #[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)] @@ -49,9 +180,13 @@ pub struct Migration { #[derive(Clone, Debug, Default, Deserialize, PartialEq, Serialize)] pub struct DurableObjectsMigration { + #[serde(default)] pub new_classes: Vec, + #[serde(default)] pub deleted_classes: Vec, + #[serde(default)] pub renamed_classes: Vec, + #[serde(default)] pub transferred_classes: Vec, } @@ -67,3 +202,178 @@ pub struct TransferClass { pub from_script: String, pub to: String, } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn adhoc_only_change_tag() -> Result<(), anyhow::Error> { + let migrations = Migrations::Adhoc { + script_tag: MigrationTag::HasTag(String::from("v1")), + provided_old_tag: Some(String::from("v1")), + new_tag: Some(String::from("v2")), + migration: None, + }; + + let api_migration = migrations.api_migration()?; + + assert_eq!( + api_migration, + Some(ApiMigration { + old_tag: Some(String::from("v1")), + new_tag: Some(String::from("v2")), + steps: vec![] + }) + ); + + Ok(()) + } + + #[test] + fn adhoc_full() -> Result<(), anyhow::Error> { + let migrations = Migrations::Adhoc { + script_tag: MigrationTag::HasTag(String::from("v1")), + provided_old_tag: Some(String::from("v1")), + new_tag: Some(String::from("v2")), + migration: Some(Migration { + durable_objects: DurableObjectsMigration { + new_classes: vec![String::from("A")], + deleted_classes: vec![], + renamed_classes: vec![], + transferred_classes: vec![], + }, + }), + }; + + let api_migration = migrations.api_migration()?; + + assert_eq!( + api_migration, + Some(ApiMigration { + old_tag: Some(String::from("v1")), + new_tag: Some(String::from("v2")), + steps: vec![Migration { + durable_objects: DurableObjectsMigration { + new_classes: vec![String::from("A")], + deleted_classes: vec![], + renamed_classes: vec![], + transferred_classes: vec![], + } + }] + }) + ); + + Ok(()) + } + + #[test] + fn migration_list_fresh() -> Result<(), anyhow::Error> { + let migrations = Migrations::List { + script_tag: MigrationTag::NoScript, + migrations: vec![MigrationConfig { + tag: String::from("v1"), + migration: Migration { + durable_objects: DurableObjectsMigration { + new_classes: vec![String::from("A")], + deleted_classes: vec![], + renamed_classes: vec![], + transferred_classes: vec![], + }, + }, + }], + }; + let api_migration = migrations.api_migration()?; + + assert_eq!( + api_migration, + Some(ApiMigration { + old_tag: None, + new_tag: Some(String::from("v1")), + steps: vec![Migration { + durable_objects: DurableObjectsMigration { + new_classes: vec![String::from("A")], + deleted_classes: vec![], + renamed_classes: vec![], + transferred_classes: vec![], + } + }] + }) + ); + + Ok(()) + } + + #[test] + fn migration_list_hastag_noop() -> Result<(), anyhow::Error> { + let migrations = Migrations::List { + script_tag: MigrationTag::HasTag(String::from("v1")), + migrations: vec![MigrationConfig { + tag: String::from("v1"), + migration: Migration { + durable_objects: DurableObjectsMigration { + new_classes: vec![String::from("A")], + deleted_classes: vec![], + renamed_classes: vec![], + transferred_classes: vec![], + }, + }, + }], + }; + let api_migration = migrations.api_migration()?; + + assert_eq!(api_migration, None); + + Ok(()) + } + + #[test] + fn migration_list_hastag() -> Result<(), anyhow::Error> { + let migrations = Migrations::List { + script_tag: MigrationTag::HasTag(String::from("v1")), + migrations: vec![ + MigrationConfig { + tag: String::from("v1"), + migration: Migration { + durable_objects: DurableObjectsMigration { + new_classes: vec![String::from("A")], + deleted_classes: vec![], + renamed_classes: vec![], + transferred_classes: vec![], + }, + }, + }, + MigrationConfig { + tag: String::from("v2"), + migration: Migration { + durable_objects: DurableObjectsMigration { + new_classes: vec![String::from("B")], + deleted_classes: vec![], + renamed_classes: vec![], + transferred_classes: vec![], + }, + }, + }, + ], + }; + let api_migration = migrations.api_migration()?; + + assert_eq!( + api_migration, + Some(ApiMigration { + old_tag: Some(String::from("v1")), + new_tag: Some(String::from("v2")), + steps: vec![Migration { + durable_objects: DurableObjectsMigration { + new_classes: vec![String::from("B")], + deleted_classes: vec![], + renamed_classes: vec![], + transferred_classes: vec![], + } + }] + }) + ); + + Ok(()) + } +} diff --git a/src/upload/form/mod.rs b/src/upload/form/mod.rs index 15c24a0e0..97f8860e5 100644 --- a/src/upload/form/mod.rs +++ b/src/upload/form/mod.rs @@ -117,7 +117,7 @@ pub fn build( } UploadFormat::Modules { main, dir, rules } => { let migration = match &target.migrations { - Some(migrations) => Some(migrations.api_migration()?), + Some(migrations) => migrations.api_migration()?, None => None, };