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().
stackabletech · Dec 18, 2024 · 01915ed · 01915ed
1 parent a9bd54d
commit 01915ed
Show file tree

Hide file tree

Showing 8 changed files with 49 additions and 81 deletions.
diff --git 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<S> {
-    fn inner(&self) -> &IndexMap<String, S>;
+    fn inner(self) -> IndexMap<String, S>;
 }
 
 /// 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<String, S> {
-        &self.inner
-    }
+impl<L, S> Deref for List<L, S>
+where
+    L: for<'a> Deserialize<'a> + Serialize + SpecIter<S>,
+    S: for<'a> Deserialize<'a> + Serialize + Clone,
+{
+    type Target = IndexMap<String, S>;
 
-    /// Returns an optional reference to a single spec of type `S` by `name`
-    pub fn get<T>(&self, name: T) -> Option<&S>
-    where
-        T: AsRef<str>,
-    {
-        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
@@ -18,8 +18,8 @@ pub struct DemosV2 {
 }
 
 impl SpecIter<DemoSpec> for DemosV2 {
-    fn inner(&self) -> &IndexMap<String, DemoSpec> {
-        &self.demos
+    fn inner(self) -> IndexMap<String, DemoSpec> {
+        self.demos
     }
 }
 

diff --git 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<ReleaseSpec> for Releases {
-    fn inner(&self) -> &IndexMap<String, ReleaseSpec> {
-        &self.releases
+    fn inner(self) -> IndexMap<String, ReleaseSpec> {
+        self.releases
     }
 }
 
 pub type ReleaseList = crate::common::list::List<Releases, ReleaseSpec>;
-
-#[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
@@ -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<T, E = Error> = std::result::Result<T, E>;
 
 #[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<String, Error> {
-        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
@@ -18,8 +18,8 @@ pub struct StacksV2 {
 }
 
 impl SpecIter<StackSpec> for StacksV2 {
-    fn inner(&self) -> &IndexMap<String, StackSpec> {
-        &self.stacks
+    fn inner(self) -> IndexMap<String, StackSpec> {
+        self.stacks
     }
 }
 

diff --git a/rust/stackablectl/src/cmds/demo.rs b/rust/stackablectl/src/cmds/demo.rs
@@ -3,15 +3,14 @@ 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};
 
 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,20 +172,20 @@ 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 {
                     format!("release-{release}")
                 }
             }
             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<St
                 .set_content_arrangement(arrangement)
                 .load_preset(preset);
 
-            for (index, (demo_name, demo_spec)) in list.inner().iter().enumerate() {
+            for (index, (demo_name, demo_spec)) in list.iter().enumerate() {
                 let row = Row::from(vec![
                     (index + 1).to_string(),
                     demo_name.clone(),
@@ -253,8 +252,8 @@ async fn list_cmd(args: &DemoListArgs, cli: &Cli, list: demo::List) -> Result<St
 
             Ok(result.render())
         }
-        OutputType::Json => 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
@@ -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
@@ -3,15 +3,15 @@ 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};
 
 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,20 +158,20 @@ 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 {
                     format!("release-{release}")
                 }
             }
             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(),