From 01915ed526d81ef956e1cd7792f82df0080d18f9 Mon Sep 17 00:00:00 2001 From: Techassi Date: Wed, 18 Dec 2024 12:18:44 +0100 Subject: [PATCH] refactor: Simplify List type This add a Deref impl for the List type, which removed the need to call .inner() to retrieve the IndexMap. Instead IndexMap's methods can now be used by deref'ing. Also, the SpecIter trait now consumes self, which removes two superflous calls to .clone(). --- rust/stackable-cockpit/src/common/list.rs | 25 ++++++++-------- .../src/platform/demo/mod.rs | 4 +-- .../src/platform/release/mod.rs | 7 ++--- .../src/platform/release/spec.rs | 29 +------------------ .../src/platform/stack/mod.rs | 4 +-- rust/stackablectl/src/cmds/demo.rs | 29 +++++++++---------- rust/stackablectl/src/cmds/release.rs | 6 ++-- rust/stackablectl/src/cmds/stack.rs | 26 ++++++++--------- 8 files changed, 49 insertions(+), 81 deletions(-) diff --git a/rust/stackable-cockpit/src/common/list.rs b/rust/stackable-cockpit/src/common/list.rs index 62b269bd..66b721e4 100644 --- a/rust/stackable-cockpit/src/common/list.rs +++ b/rust/stackable-cockpit/src/common/list.rs @@ -1,4 +1,4 @@ -use std::marker::PhantomData; +use std::{marker::PhantomData, ops::Deref}; use indexmap::IndexMap; use serde::{Deserialize, Serialize}; @@ -18,7 +18,7 @@ pub enum Error { } pub trait SpecIter { - fn inner(&self) -> &IndexMap; + fn inner(self) -> IndexMap; } /// A [`List`] describes a list of specs. The list can contain any specs, for @@ -54,7 +54,7 @@ where .context(FileTransferSnafu)?; for (spec_name, spec) in specs.inner() { - map.insert(spec_name.clone(), spec.clone()); + map.insert(spec_name, spec); } } @@ -63,17 +63,16 @@ where inner: map, }) } +} - /// Returns a reference to the inner [`IndexMap`] - pub fn inner(&self) -> &IndexMap { - &self.inner - } +impl Deref for List +where + L: for<'a> Deserialize<'a> + Serialize + SpecIter, + S: for<'a> Deserialize<'a> + Serialize + Clone, +{ + type Target = IndexMap; - /// Returns an optional reference to a single spec of type `S` by `name` - pub fn get(&self, name: T) -> Option<&S> - where - T: AsRef, - { - self.inner.get(name.as_ref()) + fn deref(&self) -> &Self::Target { + &self.inner } } diff --git a/rust/stackable-cockpit/src/platform/demo/mod.rs b/rust/stackable-cockpit/src/platform/demo/mod.rs index 9bcd9548..2b745a48 100644 --- a/rust/stackable-cockpit/src/platform/demo/mod.rs +++ b/rust/stackable-cockpit/src/platform/demo/mod.rs @@ -18,8 +18,8 @@ pub struct DemosV2 { } impl SpecIter for DemosV2 { - fn inner(&self) -> &IndexMap { - &self.demos + fn inner(self) -> IndexMap { + self.demos } } diff --git a/rust/stackable-cockpit/src/platform/release/mod.rs b/rust/stackable-cockpit/src/platform/release/mod.rs index 318ec406..78ca0495 100644 --- a/rust/stackable-cockpit/src/platform/release/mod.rs +++ b/rust/stackable-cockpit/src/platform/release/mod.rs @@ -15,12 +15,9 @@ pub struct Releases { } impl SpecIter for Releases { - fn inner(&self) -> &IndexMap { - &self.releases + fn inner(self) -> IndexMap { + self.releases } } pub type ReleaseList = crate::common::list::List; - -#[derive(Default)] -pub struct Release {} diff --git a/rust/stackable-cockpit/src/platform/release/spec.rs b/rust/stackable-cockpit/src/platform/release/spec.rs index 9b3dd250..ec0199a4 100644 --- a/rust/stackable-cockpit/src/platform/release/spec.rs +++ b/rust/stackable-cockpit/src/platform/release/spec.rs @@ -1,8 +1,7 @@ use futures::{StreamExt as _, TryStreamExt}; use indexmap::IndexMap; -use regex::Regex; use serde::{Deserialize, Serialize}; -use snafu::{OptionExt, ResultExt, Snafu}; +use snafu::{ResultExt, Snafu}; use tokio::task::JoinError; use tracing::{info, instrument}; @@ -17,8 +16,6 @@ use crate::{ }, }; -use super::ReleaseList; - type Result = std::result::Result; #[derive(Debug, Snafu)] @@ -34,12 +31,6 @@ pub enum Error { #[snafu(display("failed to launch background task"))] BackgroundTask { source: JoinError }, - - #[snafu(display("release list is empty"))] - EmptyReleaseList, - - #[snafu(display("latest release doesn't have expected format"))] - LatestReleaseFormat, } #[derive(Clone, Debug, Deserialize, Serialize)] @@ -128,21 +119,3 @@ impl ReleaseSpec { .collect() } } - -impl ReleaseList { - /// Checks if a value provided in the '--release' argument is in the release list - pub fn contains(&self, release: &str) -> bool { - self.inner().contains_key(release) - } - - /// Retrieves the latest release from the list and applies a sanity check to the release format. - pub fn latest_release(&self) -> Result { - let release = self.inner().first().context(EmptyReleaseListSnafu)?.0; - let sanity_check = Regex::new("^[0-9]{2}.[0-9]{1,2}$").unwrap(); - if sanity_check.is_match(release) { - Ok(release.to_string()) - } else { - LatestReleaseFormatSnafu {}.fail() - } - } -} diff --git a/rust/stackable-cockpit/src/platform/stack/mod.rs b/rust/stackable-cockpit/src/platform/stack/mod.rs index 0c649c05..4883f3af 100644 --- a/rust/stackable-cockpit/src/platform/stack/mod.rs +++ b/rust/stackable-cockpit/src/platform/stack/mod.rs @@ -18,8 +18,8 @@ pub struct StacksV2 { } impl SpecIter for StacksV2 { - fn inner(&self) -> &IndexMap { - &self.stacks + fn inner(self) -> IndexMap { + self.stacks } } diff --git a/rust/stackablectl/src/cmds/demo.rs b/rust/stackablectl/src/cmds/demo.rs index 39a7609f..0fbb2b76 100644 --- a/rust/stackablectl/src/cmds/demo.rs +++ b/rust/stackablectl/src/cmds/demo.rs @@ -3,7 +3,7 @@ use comfy_table::{ presets::{NOTHING, UTF8_FULL}, ContentArrangement, Row, Table, }; -use snafu::{ResultExt, Snafu}; +use snafu::{ensure, OptionExt as _, ResultExt, Snafu}; use stackable_operator::kvp::{LabelError, Labels}; use tracing::{debug, info, instrument}; @@ -11,7 +11,6 @@ use stackable_cockpit::{ common::list, constants::{DEFAULT_OPERATOR_NAMESPACE, DEFAULT_PRODUCT_NAMESPACE}, platform::{ - self, demo::{self, DemoInstallParameters}, release, stack, }, @@ -131,11 +130,11 @@ pub enum CmdError { #[snafu(display("no stack with name '{name}'"))] NoSuchStack { name: String }, - #[snafu(display("no release with name '{name}'"))] - NoSuchRelease { name: String }, + #[snafu(display("no release '{release}'"))] + NoSuchRelease { release: String }, #[snafu(display("failed to get latest release"))] - LatestRelease { source: platform::release::Error }, + LatestRelease, #[snafu(display("failed to build demo/stack/release list"))] BuildList { source: list::Error }, @@ -173,9 +172,11 @@ impl DemoArgs { let release_branch = match &self.release { Some(release) => { - if !release_list.contains(release) { - return NoSuchReleaseSnafu { name: release }.fail(); - } + ensure!( + release_list.contains_key(release), + NoSuchReleaseSnafu { release } + ); + if release == "dev" { "main".to_string() } else { @@ -183,10 +184,8 @@ impl DemoArgs { } } None => { - format!( - "release-{release}", - release = release_list.latest_release().context(LatestReleaseSnafu)? - ) + let (release_name, _) = release_list.first().context(LatestReleaseSnafu)?; + format!("release-{release}", release = release_name,) } }; @@ -228,7 +227,7 @@ async fn list_cmd(args: &DemoListArgs, cli: &Cli, list: demo::List) -> Result Result serde_json::to_string(&list.inner()).context(SerializeJsonOutputSnafu), - OutputType::Yaml => serde_yaml::to_string(&list.inner()).context(SerializeYamlOutputSnafu), + OutputType::Json => serde_json::to_string(&*list).context(SerializeJsonOutputSnafu), + OutputType::Yaml => serde_yaml::to_string(&*list).context(SerializeYamlOutputSnafu), } } diff --git a/rust/stackablectl/src/cmds/release.rs b/rust/stackablectl/src/cmds/release.rs index 2b644f5a..5a22bfc6 100644 --- a/rust/stackablectl/src/cmds/release.rs +++ b/rust/stackablectl/src/cmds/release.rs @@ -138,7 +138,7 @@ impl ReleaseArgs { .await .context(BuildListSnafu)?; - if release_list.inner().is_empty() { + if release_list.is_empty() { return Ok("No releases".into()); } @@ -161,7 +161,7 @@ async fn list_cmd( match args.output_type { OutputType::Plain | OutputType::Table => { - if release_list.inner().is_empty() { + if release_list.is_empty() { return Ok("No releases".into()); } @@ -176,7 +176,7 @@ async fn list_cmd( .set_content_arrangement(arrangement) .load_preset(preset); - for (index, (release_name, release_spec)) in release_list.inner().iter().enumerate() { + for (index, (release_name, release_spec)) in release_list.iter().enumerate() { table.add_row(vec![ (index + 1).to_string(), release_name.to_string(), diff --git a/rust/stackablectl/src/cmds/stack.rs b/rust/stackablectl/src/cmds/stack.rs index 2231dc95..a2ca9423 100644 --- a/rust/stackablectl/src/cmds/stack.rs +++ b/rust/stackablectl/src/cmds/stack.rs @@ -3,7 +3,7 @@ use comfy_table::{ presets::{NOTHING, UTF8_FULL}, ContentArrangement, Table, }; -use snafu::{ResultExt, Snafu}; +use snafu::{ensure, OptionExt as _, ResultExt, Snafu}; use stackable_operator::kvp::{LabelError, Labels}; use tracing::{debug, info, instrument}; @@ -11,7 +11,7 @@ use stackable_cockpit::{ common::list, constants::{DEFAULT_OPERATOR_NAMESPACE, DEFAULT_PRODUCT_NAMESPACE}, platform::{ - self, release, + release, stack::{self, StackInstallParameters}, }, utils::{ @@ -120,11 +120,11 @@ pub enum CmdError { #[snafu(display("failed to serialize JSON output"))] SerializeJsonOutput { source: serde_json::Error }, - #[snafu(display("no release with name '{name}'"))] - NoSuchRelease { name: String }, + #[snafu(display("no release '{release}'"))] + NoSuchRelease { release: String }, #[snafu(display("failed to get latest release"))] - LatestRelease { source: platform::release::Error }, + LatestRelease, #[snafu(display("failed to build stack/release list"))] BuildList { source: list::Error }, @@ -158,9 +158,11 @@ impl StackArgs { let release_branch = match &self.release { Some(release) => { - if !release_list.contains(release) { - return NoSuchReleaseSnafu { name: release }.fail(); - } + ensure!( + release_list.contains_key(release), + NoSuchReleaseSnafu { release } + ); + if release == "dev" { "main".to_string() } else { @@ -168,10 +170,8 @@ impl StackArgs { } } None => { - format!( - "release-{release}", - release = release_list.latest_release().context(LatestReleaseSnafu)? - ) + let (release_name, _) = release_list.first().context(LatestReleaseSnafu)?; + format!("release-{release}", release = release_name,) } }; @@ -213,7 +213,7 @@ fn list_cmd( .set_content_arrangement(arrangement) .load_preset(preset); - for (index, (stack_name, stack)) in stack_list.inner().iter().enumerate() { + for (index, (stack_name, stack)) in stack_list.iter().enumerate() { table.add_row(vec![ (index + 1).to_string(), stack_name.clone(),