chore!: remove thiserror (add module-scoped errors with the help of snafu) (#761)

* remove deprecated exports

* refactor: remove thiserror

* move error enum to below the Result type-alias

* update changelog

* use managed_by_labels variable in pdb builder

* reorder imports

* don't match on metadata

* replace map_err with context and remove Box from the error source

* remove Ok(..)?

* tidy up kube client construction

* use proper resource kinds in error

* use String Debug impl in error message

* rename error variants

* upgrade change to breaking

* Apply suggestions

Co-authored-by: Techassi <[email protected]>

* replace map_err with context

* remove associated error type

* simplify error message

* non-java error variants

* return Snafu fail()

* Apply suggestions

Co-authored-by: Techassi <[email protected]>

---------

Co-authored-by: Techassi <[email protected]>

Author: NickLarsenNZ

Cargo.toml

@@ -54,7 +54,6 @@ stackable-operator-derive = { path = "stackable-operator-derive" }
 strum = { version = "0.26.2", features = ["derive"] }
 syn = "2.0.55"
 tempfile = "3.10.1"
-thiserror = "1.0.58"
 time = { version = "0.3.34" }
 tokio = { version = "1.36.0", features = ["macros", "rt-multi-thread", "fs"] }
 tokio-rustls = "0.26.0"

crates/stackable-operator/CHANGELOG.md

@@ -8,6 +8,11 @@ All notable changes to this project will be documented in this file.
 
 - Bump kube to 0.89.0 and update all dependencies ([#762]).
 
+### Removed
+
+- BREAKING: Remove `thiserror` dependency, and deprecated builder exports ([#761])
+
+[#761]: https://github.com/stackabletech/operator-rs/pull/761
 [#762]: https://github.com/stackabletech/operator-rs/pull/762
 
 ## [0.66.0] - 2024-03-26

crates/stackable-operator/Cargo.toml

@@ -36,7 +36,6 @@ serde_yaml.workspace = true
 serde.workspace = true
 snafu.workspace = true
 strum.workspace = true
-thiserror.workspace = true
 time = { workspace = true, optional = true }
 tokio.workspace = true
 tracing-opentelemetry.workspace = true

crates/stackable-operator/src/builder/configmap.rs

@@ -1,7 +1,15 @@
-use crate::error::{Error, OperatorResult};
 use k8s_openapi::{api::core::v1::ConfigMap, apimachinery::pkg::apis::meta::v1::ObjectMeta};
+use snafu::{OptionExt, Snafu};
 use std::collections::BTreeMap;
 
+type Result<T, E = Error> = std::result::Result<T, E>;
+
+#[derive(Snafu, Debug)]
+pub enum Error {
+    #[snafu(display("object is missing key {key:?}"))]
+    MissingObjectKey { key: &'static str },
+}
+
 /// A builder to build [`ConfigMap`] objects.
 #[derive(Clone, Default)]
 pub struct ConfigMapBuilder {
@@ -41,12 +49,13 @@ impl ConfigMapBuilder {
         self
     }
 
-    pub fn build(&self) -> OperatorResult<ConfigMap> {
+    pub fn build(&self) -> Result<ConfigMap> {
+        let metadata = self
+            .metadata
+            .clone()
+            .context(MissingObjectKeySnafu { key: "metadata" })?;
         Ok(ConfigMap {
-            metadata: match self.metadata {
-                None => return Err(Error::MissingObjectKey { key: "metadata" }),
-                Some(ref metadata) => metadata.clone(),
-            },
+            metadata,
             data: self.data.clone(),
             ..ConfigMap::default()
         })

crates/stackable-operator/src/builder/meta.rs

@@ -3,16 +3,17 @@ use kube::{Resource, ResourceExt};
 use snafu::{ResultExt, Snafu};
 use tracing::warn;
 
-use crate::{
-    error::{Error, OperatorResult},
-    kvp::{Annotation, Annotations, Label, LabelError, Labels, ObjectLabels},
-};
+use crate::kvp::{Annotation, Annotations, Label, LabelError, Labels, ObjectLabels};
+
+type Result<T, E = Error> = std::result::Result<T, E>;
 
-// NOTE (Techassi): Think about that name
 #[derive(Debug, PartialEq, Snafu)]
-pub enum ObjectMetaBuilderError {
+pub enum Error {
     #[snafu(display("failed to set recommended labels"))]
     RecommendedLabels { source: LabelError },
+
+    #[snafu(display("object is missing key {key:?}"))]
+    MissingObjectKey { key: &'static str },
 }
 
 /// A builder to build [`ObjectMeta`] objects.
@@ -89,7 +90,7 @@ impl ObjectMetaBuilder {
         resource: &T,
         block_owner_deletion: Option<bool>,
         controller: Option<bool>,
-    ) -> OperatorResult<&mut Self> {
+    ) -> Result<&mut Self> {
         self.ownerreference = Some(
             OwnerReferenceBuilder::new()
                 .initialize_from_resource(resource)
@@ -151,7 +152,7 @@ impl ObjectMetaBuilder {
     pub fn with_recommended_labels<T: Resource>(
         &mut self,
         object_labels: ObjectLabels<T>,
-    ) -> Result<&mut Self, ObjectMetaBuilderError> {
+    ) -> Result<&mut Self> {
         let recommended_labels =
             Labels::recommended(object_labels).context(RecommendedLabelsSnafu)?;
 
@@ -284,24 +285,24 @@ impl OwnerReferenceBuilder {
         self
     }
 
-    pub fn build(&self) -> OperatorResult<OwnerReference> {
+    pub fn build(&self) -> Result<OwnerReference> {
         Ok(OwnerReference {
             api_version: match self.api_version {
-                None => return Err(Error::MissingObjectKey { key: "api_version" }),
+                None => return MissingObjectKeySnafu { key: "api_version" }.fail(),
                 Some(ref api_version) => api_version.clone(),
             },
             block_owner_deletion: self.block_owner_deletion,
             controller: self.controller,
             kind: match self.kind {
-                None => return Err(Error::MissingObjectKey { key: "kind" }),
+                None => return MissingObjectKeySnafu { key: "kind" }.fail(),
                 Some(ref kind) => kind.clone(),
             },
             name: match self.name {
-                None => return Err(Error::MissingObjectKey { key: "name" }),
+                None => return MissingObjectKeySnafu { key: "name" }.fail(),
                 Some(ref name) => name.clone(),
             },
             uid: match self.uid {
-                None => return Err(Error::MissingObjectKey { key: "uid" }),
+                None => return MissingObjectKeySnafu { key: "uid" }.fail(),
                 Some(ref uid) => uid.clone(),
             },
         })

crates/stackable-operator/src/builder/mod.rs

@@ -8,33 +8,3 @@ pub mod event;
 pub mod meta;
 pub mod pdb;
 pub mod pod;
-
-#[deprecated(since = "0.15.0", note = "Please use `builder::configmap::*` instead.")]
-pub use configmap::*;
-
-#[deprecated(since = "0.15.0", note = "Please use `builder::event::*` instead.")]
-pub use event::*;
-
-#[deprecated(since = "0.15.0", note = "Please use `builder::meta::*` instead.")]
-pub use meta::*;
-
-#[deprecated(
-    since = "0.15.0",
-    note = "Please use `builder::pod::container::*` instead."
-)]
-pub use pod::container::*;
-
-#[deprecated(
-    since = "0.15.0",
-    note = "Please use `builder::pod::security::*` instead."
-)]
-pub use pod::security::*;
-
-#[deprecated(
-    since = "0.15.0",
-    note = "Please use `builder::pod::volume::*` instead."
-)]
-pub use pod::volume::*;
-
-#[deprecated(since = "0.15.0", note = "Please use `builder::pod::*` instead.")]
-pub use pod::*;

crates/stackable-operator/src/builder/pdb.rs

@@ -6,13 +6,27 @@ use k8s_openapi::{
     },
 };
 use kube::{Resource, ResourceExt};
+use snafu::{ResultExt, Snafu};
 
 use crate::{
-    builder::ObjectMetaBuilder,
-    error::OperatorResult,
+    builder::meta::ObjectMetaBuilder,
     kvp::{Label, Labels},
 };
 
+type Result<T, E = Error> = std::result::Result<T, E>;
+
+#[derive(Debug, PartialEq, Snafu)]
+pub enum Error {
+    #[snafu(display("failed to create role selector labels"))]
+    RoleSelectorLabels { source: crate::kvp::LabelError },
+
+    #[snafu(display("failed to set owner reference from resource"))]
+    OwnerReferenceFromResource { source: crate::builder::meta::Error },
+
+    #[snafu(display("failed to create app.kubernetes.io/managed-by label"))]
+    ManagedByLabel { source: crate::kvp::LabelError },
+}
+
 /// This builder is used to construct [`PodDisruptionBudget`]s.
 /// If you are using this to create [`PodDisruptionBudget`]s according to [ADR 30 on Allowed Pod disruptions][adr],
 /// the use of [`PodDisruptionBudgetBuilder::new_with_role`] is recommended.
@@ -70,14 +84,18 @@ impl PodDisruptionBudgetBuilder<(), (), ()> {
         role: &str,
         operator_name: &str,
         controller_name: &str,
-    ) -> OperatorResult<PodDisruptionBudgetBuilder<ObjectMeta, LabelSelector, ()>> {
-        let role_selector_labels = Labels::role_selector(owner, app_name, role)?;
+    ) -> Result<PodDisruptionBudgetBuilder<ObjectMeta, LabelSelector, ()>> {
+        let role_selector_labels =
+            Labels::role_selector(owner, app_name, role).context(RoleSelectorLabelsSnafu)?;
+        let managed_by_label =
+            Label::managed_by(operator_name, controller_name).context(ManagedByLabelSnafu)?;
         let metadata = ObjectMetaBuilder::new()
             .namespace_opt(owner.namespace())
             .name(format!("{}-{}", owner.name_any(), role))
-            .ownerreference_from_resource(owner, None, Some(true))?
+            .ownerreference_from_resource(owner, None, Some(true))
+            .context(OwnerReferenceFromResourceSnafu)?
             .with_labels(role_selector_labels.clone())
-            .with_label(Label::managed_by(operator_name, controller_name)?)
+            .with_label(managed_by_label)
             .build();
 
         Ok(PodDisruptionBudgetBuilder {
@@ -191,7 +209,7 @@ mod test {
     use schemars::JsonSchema;
     use serde::{Deserialize, Serialize};
 
-    use crate::builder::{ObjectMetaBuilder, OwnerReferenceBuilder};
+    use crate::builder::meta::{ObjectMetaBuilder, OwnerReferenceBuilder};
 
     use super::PodDisruptionBudgetBuilder;
 

crates/stackable-operator/src/builder/pod/container.rs

@@ -1,15 +1,27 @@
+use std::fmt;
+
 use k8s_openapi::api::core::v1::{
     ConfigMapKeySelector, Container, ContainerPort, EnvVar, EnvVarSource, Lifecycle,
     LifecycleHandler, ObjectFieldSelector, Probe, ResourceRequirements, SecretKeySelector,
     SecurityContext, VolumeMount,
 };
-use std::fmt;
+use snafu::Snafu;
 
 use crate::{
-    commons::product_image_selection::ResolvedProductImage, error::Error,
-    validation::is_rfc_1123_label,
+    commons::product_image_selection::ResolvedProductImage, validation::is_rfc_1123_label,
 };
 
+type Result<T, E = Error> = std::result::Result<T, E>;
+
+#[derive(Debug, PartialEq, Snafu)]
+pub enum Error {
+    #[snafu(display("container name {container_name:?} is invalid: {violation:?}"))]
+    InvalidContainerName {
+        container_name: String,
+        violation: String,
+    },
+}
+
 /// A builder to build [`Container`] objects.
 ///
 /// This will automatically create the necessary volumes and mounts for each `ConfigMap` which is added.
@@ -32,7 +44,7 @@ pub struct ContainerBuilder {
 }
 
 impl ContainerBuilder {
-    pub fn new(name: &str) -> Result<Self, Error> {
+    pub fn new(name: &str) -> Result<Self> {
         Self::validate_container_name(name)?;
         Ok(ContainerBuilder {
             name: name.to_string(),
@@ -264,7 +276,7 @@ impl ContainerBuilder {
 
     /// Validates a container name is according to the [RFC 1123](https://www.ietf.org/rfc/rfc1123.txt) standard.
     /// Returns [Ok] if the name is according to the standard, and [Err] if not.
-    fn validate_container_name(name: &str) -> Result<(), Error> {
+    fn validate_container_name(name: &str) -> Result<()> {
         let validation_result = is_rfc_1123_label(name);
 
         match validation_result {
@@ -355,8 +367,8 @@ mod tests {
 
     use super::*;
     use crate::{
-        builder::{
-            pod::container::{ContainerBuilder, ContainerPortBuilder, FieldPathEnvVar},
+        builder::pod::{
+            container::{ContainerBuilder, ContainerPortBuilder, FieldPathEnvVar},
             resources::ResourceRequirementsBuilder,
         },
         commons::resources::ResourceRequirementsType,
@@ -490,16 +502,13 @@ mod tests {
                 panic!("Container name exceeding 63 characters should cause an error");
             }
             Err(error) => match error {
-                crate::error::Error::InvalidContainerName {
+                Error::InvalidContainerName {
                     container_name,
                     violation,
                 } => {
                     assert_eq!(container_name.as_str(), long_container_name);
                     assert_eq!(violation.as_str(), "must be no more than 63 characters")
                 }
-                _ => {
-                    panic!("InvalidContainerName error expected")
-                }
             },
         }
         // One characters shorter name is valid
@@ -569,16 +578,13 @@ mod tests {
                 panic!("Container name exceeding 63 characters should cause an error");
             }
             Err(error) => match error {
-                crate::error::Error::InvalidContainerName {
+                Error::InvalidContainerName {
                     container_name: _,
                     violation,
                 } => {
                     println!("{violation}");
                     assert!(violation.contains(expected_err_contains));
                 }
-                _ => {
-                    panic!("InvalidContainerName error expected");
-                }
             },
         }
     }