Merge pull request 'New layout: fixes and UX improvements' (#634) from new-layout-ux into nextv0.9.0-beta3

Reviewed-on: https://git.deuxfleurs.fr/Deuxfleurs/garage/pulls/634

2 files changed, 61 insertions, 67 deletions

diff --git a/src/garage/cli/layout.rs b/src/garage/cli/layout.rs
index 3932f115..ce2b11e0 100644
--- a/src/garage/cli/layout.rs
+++ b/src/garage/cli/layout.rs
@@ -174,16 +174,12 @@ pub async fn cmd_show_layout(
 	let layout = fetch_layout(rpc_cli, rpc_host).await?;
 
 	println!("==== CURRENT CLUSTER LAYOUT ====");
-	if !print_cluster_layout(&layout) {
-		println!("No nodes currently have a role in the cluster.");
-		println!("See `garage status` to view available nodes.");
-	}
+	print_cluster_layout(&layout, "No nodes currently have a role in the cluster.\nSee `garage status` to view available nodes.");
 	println!();
 	println!("Current cluster layout version: {}", layout.version);
 
 	let has_role_changes = print_staging_role_changes(&layout);
-	let has_param_changes = print_staging_parameters_changes(&layout);
-	if has_role_changes || has_param_changes {
+	if has_role_changes {
 		let v = layout.version;
 		let res_apply = layout.apply_staged_changes(Some(v + 1));
 
@@ -193,9 +189,7 @@ pub async fn cmd_show_layout(
 			Ok((layout, msg)) => {
 				println!();
 				println!("==== NEW CLUSTER LAYOUT AFTER APPLYING CHANGES ====");
-				if !print_cluster_layout(&layout) {
-					println!("No nodes have a role in the new layout.");
-				}
+				print_cluster_layout(&layout, "No nodes have a role in the new layout.");
 				println!();
 
 				for line in msg.iter() {
@@ -267,28 +261,35 @@ pub async fn cmd_config_layout(
 	let mut did_something = false;
 	match config_opt.redundancy {
 		None => (),
-		Some(r) => {
-			if r > layout.replication_factor {
-				println!(
-					"The zone redundancy must be smaller or equal to the \
-                replication factor ({}).",
-					layout.replication_factor
-				);
-			} else if r < 1 {
-				println!("The zone redundancy must be at least 1.");
-			} else {
-				layout
-					.staging_parameters
-					.update(LayoutParameters { zone_redundancy: r });
-				println!("The new zone redundancy has been saved ({}).", r);
+		Some(r_str) => {
+			let r = r_str
+				.parse::<ZoneRedundancy>()
+				.ok_or_message("invalid zone redundancy value")?;
+			if let ZoneRedundancy::AtLeast(r_int) = r {
+				if r_int > layout.replication_factor {
+					return Err(Error::Message(format!(
+						"The zone redundancy must be smaller or equal to the \
+                    replication factor ({}).",
+						layout.replication_factor
+					)));
+				} else if r_int < 1 {
+					return Err(Error::Message(
+						"The zone redundancy must be at least 1.".into(),
+					));
+				}
 			}
+
+			layout
+				.staging_parameters
+				.update(LayoutParameters { zone_redundancy: r });
+			println!("The zone redundancy parameter has been set to '{}'.", r);
 			did_something = true;
 		}
 	}
 
 	if !did_something {
 		return Err(Error::Message(
-			"Please specify an action for `garage layout config` to do".into(),
+			"Please specify an action for `garage layout config`".into(),
 		));
 	}
 
@@ -326,7 +327,7 @@ pub async fn send_layout(
 	Ok(())
 }
 
-pub fn print_cluster_layout(layout: &ClusterLayout) -> bool {
+pub fn print_cluster_layout(layout: &ClusterLayout, empty_msg: &str) {
 	let mut table = vec!["ID\tTags\tZone\tCapacity\tUsable capacity".to_string()];
 	for (id, _, role) in layout.roles.items().iter() {
 		let role = match &role.0 {
@@ -356,61 +357,54 @@ pub fn print_cluster_layout(layout: &ClusterLayout) -> bool {
 			));
 		};
 	}
-	println!();
-	println!("Parameters of the layout computation:");
-	println!("Zone redundancy: {}", layout.parameters.zone_redundancy);
-	println!();
-	if table.len() == 1 {
-		false
-	} else {
+	if table.len() > 1 {
 		format_table(table);
-		true
-	}
-}
-
-pub fn print_staging_parameters_changes(layout: &ClusterLayout) -> bool {
-	let has_changes = *layout.staging_parameters.get() != layout.parameters;
-	if has_changes {
-		println!();
-		println!("==== NEW LAYOUT PARAMETERS ====");
-		println!(
-			"Zone redundancy: {}",
-			layout.staging_parameters.get().zone_redundancy
-		);
 		println!();
+		println!("Zone redundancy: {}", layout.parameters.zone_redundancy);
+	} else {
+		println!("{}", empty_msg);
 	}
-	has_changes
 }
 
 pub fn print_staging_role_changes(layout: &ClusterLayout) -> bool {
-	let has_changes = layout
+	let has_role_changes = layout
 		.staging_roles
 		.items()
 		.iter()
 		.any(|(k, _, v)| layout.roles.get(k) != Some(v));
+	let has_layout_changes = *layout.staging_parameters.get() != layout.parameters;
 
-	if has_changes {
+	if has_role_changes || has_layout_changes {
 		println!();
 		println!("==== STAGED ROLE CHANGES ====");
-		let mut table = vec!["ID\tTags\tZone\tCapacity".to_string()];
-		for (id, _, role) in layout.staging_roles.items().iter() {
-			if layout.roles.get(id) == Some(role) {
-				continue;
-			}
-			if let Some(role) = &role.0 {
-				let tags = role.tags.join(",");
-				table.push(format!(
-					"{:?}\t{}\t{}\t{}",
-					id,
-					tags,
-					role.zone,
-					role.capacity_string()
-				));
-			} else {
-				table.push(format!("{:?}\tREMOVED", id));
+		if has_role_changes {
+			let mut table = vec!["ID\tTags\tZone\tCapacity".to_string()];
+			for (id, _, role) in layout.staging_roles.items().iter() {
+				if layout.roles.get(id) == Some(role) {
+					continue;
+				}
+				if let Some(role) = &role.0 {
+					let tags = role.tags.join(",");
+					table.push(format!(
+						"{:?}\t{}\t{}\t{}",
+						id,
+						tags,
+						role.zone,
+						role.capacity_string()
+					));
+				} else {
+					table.push(format!("{:?}\tREMOVED", id));
+				}
 			}
+			format_table(table);
+			println!();
+		}
+		if has_layout_changes {
+			println!(
+				"Zone redundancy: {}",
+				layout.staging_parameters.get().zone_redundancy
+			);
 		}
-		format_table(table);
 		true
 	} else {
 		false
diff --git a/src/garage/cli/structs.rs b/src/garage/cli/structs.rs
index fd37a24e..c4ebeb1a 100644
--- a/src/garage/cli/structs.rs
+++ b/src/garage/cli/structs.rs
@@ -143,9 +143,9 @@ pub struct RemoveRoleOpt {
 
 #[derive(StructOpt, Debug)]
 pub struct ConfigLayoutOpt {
-	/// Zone redundancy parameter
+	/// Zone redundancy parameter ('none'/'max' or integer)
 	#[structopt(short = "r", long = "redundancy")]
-	pub(crate) redundancy: Option<usize>,
+	pub(crate) redundancy: Option<String>,
 }
 
 #[derive(StructOpt, Debug)]