Skip to content

refactor!: Make KeyValuePairs an alias for BTreeMap #889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
246 changes: 123 additions & 123 deletions Cargo.lock

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const_format = "0.2.33"
const-oid = { version = "0.9.6", features = ["db"] }
convert_case = "0.8.0"
darling = "0.20.10"
delegate = "0.13.0"
derivative = "2.2.0"
dockerfile-parser = "0.9.0"
ecdsa = { version = "0.16.9", features = ["digest", "pem"] }
educe = { version = "0.6.0", default-features = false, features = ["Clone", "Debug", "Default", "PartialEq", "Eq"] }
Expand Down
17 changes: 17 additions & 0 deletions crates/stackable-operator/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,23 @@ All notable changes to this project will be documented in this file.

## [Unreleased]

### Changed

- BREAKING: `KeyValuePairs<V>` (and `Annotations`/`Labels`) is now an alias for `BTreeMap<Key, V>` ([#889]).
- Generally, this means that the API now behaves like a map rather than a set. For example, duplicate keys are no longer allowed (as was already documented before).
- Some `KeyValuePairs` methods have been renamed for certain use cases:
- `KeyValuePairs::insert(&mut self, kvp)`: use `::extend(&mut self, [kvp])` instead.
- `KeyValuePairs::try_from`: you may need to use `::try_from_iter` instead.
- `KeyValuePairs::contains_key`: unvalidated keys will need to use `::contains_str_key` instead.
- `Into<BTreeMap<String, String>>`: use `::to_unvalidated` instead.
- Well-known annotations have been moved from `kvp::Annotation` to `kvp::annotation::well_known`.
- Well-known labels have been moved from `kvp::Label` to `kvp::label::well_known`.
- Well-known label sets have been moved from `kvp::Labels` to `kvp::label::well_known::sets`.

### Fixed

- `KeyValuePairs` will now consistently use the last-written value for a given key ([#889]).

## [0.94.0] - 2025-07-10

### Added
Expand Down
2 changes: 1 addition & 1 deletion crates/stackable-operator/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ stackable-versioned = { path = "../stackable-versioned" }
chrono.workspace = true
clap.workspace = true
const_format.workspace = true
delegate.workspace = true
derivative.workspace = true
dockerfile-parser.workspace = true
either.workspace = true
educe.workspace = true
Expand Down
15 changes: 9 additions & 6 deletions crates/stackable-operator/src/builder/meta.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ use kube::{Resource, ResourceExt};
use snafu::{ResultExt, Snafu};
use tracing::warn;

use crate::kvp::{Annotation, Annotations, Label, LabelError, Labels, ObjectLabels};
use crate::kvp::{
Annotation, Annotations, KeyValuePairs, KeyValuePairsExt, Label, LabelError, Labels,
ObjectLabels, label,
};

type Result<T, E = Error> = std::result::Result<T, E>;

Expand Down Expand Up @@ -106,7 +109,7 @@ impl ObjectMetaBuilder {
pub fn with_annotation(&mut self, annotation: Annotation) -> &mut Self {
self.annotations
.get_or_insert(Annotations::new())
.insert(annotation);
.extend([annotation]);
self
}

Expand All @@ -128,7 +131,7 @@ impl ObjectMetaBuilder {
/// This adds a single label to the existing labels.
/// It'll override a label with the same key.
pub fn with_label(&mut self, label: Label) -> &mut Self {
self.labels.get_or_insert(Labels::new()).insert(label);
self.labels.get_or_insert(Labels::new()).extend([label]);
self
}

Expand All @@ -154,7 +157,7 @@ impl ObjectMetaBuilder {
object_labels: ObjectLabels<T>,
) -> Result<&mut Self> {
let recommended_labels =
Labels::recommended(object_labels).context(RecommendedLabelsSnafu)?;
label::well_known::sets::recommended(object_labels).context(RecommendedLabelsSnafu)?;

self.labels
.get_or_insert(Labels::new())
Expand Down Expand Up @@ -185,8 +188,8 @@ impl ObjectMetaBuilder {
.ownerreference
.as_ref()
.map(|ownerreference| vec![ownerreference.clone()]),
labels: self.labels.clone().map(|l| l.into()),
annotations: self.annotations.clone().map(|a| a.into()),
labels: self.labels.as_ref().map(KeyValuePairs::to_unvalidated),
annotations: self.annotations.as_ref().map(KeyValuePairs::to_unvalidated),
..ObjectMeta::default()
}
}
Expand Down
12 changes: 6 additions & 6 deletions crates/stackable-operator/src/builder/pdb.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use snafu::{ResultExt, Snafu};

use crate::{
builder::meta::ObjectMetaBuilder,
kvp::{Label, Labels},
kvp::{KeyValuePairsExt, label},
};

type Result<T, E = Error> = std::result::Result<T, E>;
Expand Down Expand Up @@ -85,10 +85,10 @@ impl PodDisruptionBudgetBuilder<(), (), ()> {
operator_name: &str,
controller_name: &str,
) -> 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 role_selector_labels = label::well_known::sets::role_selector(owner, app_name, role)
.context(RoleSelectorLabelsSnafu)?;
let managed_by_label = label::well_known::managed_by(operator_name, controller_name)
.context(ManagedByLabelSnafu)?;
let metadata = ObjectMetaBuilder::new()
.namespace_opt(owner.namespace())
.name(format!("{}-{}", owner.name_any(), role))
Expand All @@ -102,7 +102,7 @@ impl PodDisruptionBudgetBuilder<(), (), ()> {
metadata,
selector: LabelSelector {
match_expressions: None,
match_labels: Some(role_selector_labels.into()),
match_labels: Some(role_selector_labels.to_unvalidated()),
},
..PodDisruptionBudgetBuilder::default()
})
Expand Down
14 changes: 8 additions & 6 deletions crates/stackable-operator/src/builder/pod/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -330,16 +330,17 @@ impl PodBuilder {
/// ```
/// # use stackable_operator::builder::pod::PodBuilder;
/// # use stackable_operator::builder::pod::container::ContainerBuilder;
/// # use stackable_operator::iter::TryFromIterator;
/// # use stackable_operator::kvp::Labels;
/// # use k8s_openapi::{
/// # apimachinery::pkg::apis::meta::v1::ObjectMeta,
/// # };
/// # use std::collections::BTreeMap;
///
/// let labels: Labels = Labels::try_from(
/// BTreeMap::from([("app.kubernetes.io/component", "test-role"),
/// let labels: Labels = Labels::try_from_iter(
/// [("app.kubernetes.io/component", "test-role"),
/// ("app.kubernetes.io/instance", "test"),
/// ("app.kubernetes.io/name", "test")]))
/// ("app.kubernetes.io/name", "test")])
/// .unwrap();
///
/// let pod = PodBuilder::new()
Expand Down Expand Up @@ -416,16 +417,17 @@ impl PodBuilder {
/// ```
/// # use stackable_operator::builder::pod::PodBuilder;
/// # use stackable_operator::builder::pod::container::ContainerBuilder;
/// # use stackable_operator::iter::TryFromIterator;
/// # use stackable_operator::kvp::Labels;
/// # use k8s_openapi::{
/// # apimachinery::pkg::apis::meta::v1::ObjectMeta,
/// # };
/// # use std::collections::BTreeMap;
///
/// let labels: Labels = Labels::try_from(
/// BTreeMap::from([("app.kubernetes.io/component", "test-role"),
/// let labels: Labels = Labels::try_from_iter(
/// [("app.kubernetes.io/component", "test-role"),
/// ("app.kubernetes.io/instance", "test"),
/// ("app.kubernetes.io/name", "test")]))
/// ("app.kubernetes.io/name", "test")])
/// .unwrap();
///
/// let pod = PodBuilder::new()
Expand Down
53 changes: 31 additions & 22 deletions crates/stackable-operator/src/builder/pod/volume.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use tracing::warn;

use crate::{
builder::meta::ObjectMetaBuilder,
kvp::{Annotation, AnnotationError, Annotations, LabelError, Labels},
kvp::{Annotation, AnnotationError, Annotations, LabelError, Labels, annotation},
time::Duration,
};

Expand Down Expand Up @@ -340,42 +340,53 @@ impl SecretOperatorVolumeSourceBuilder {
pub fn build(&self) -> Result<EphemeralVolumeSource, SecretOperatorVolumeSourceBuilderError> {
let mut annotations = Annotations::new();

annotations
.insert(Annotation::secret_class(&self.secret_class).context(ParseAnnotationSnafu)?);
annotations.extend([annotation::well_known::secret_volume::secret_class(
&self.secret_class,
)
.context(ParseAnnotationSnafu)?]);

if !self.scopes.is_empty() {
annotations
.insert(Annotation::secret_scope(&self.scopes).context(ParseAnnotationSnafu)?);
annotations.extend([
annotation::well_known::secret_volume::secret_scope(&self.scopes)
.context(ParseAnnotationSnafu)?,
]);
}

if let Some(format) = &self.format {
annotations
.insert(Annotation::secret_format(format.as_ref()).context(ParseAnnotationSnafu)?);
annotations.extend([annotation::well_known::secret_volume::secret_format(
format.as_ref(),
)
.context(ParseAnnotationSnafu)?]);
}

if !self.kerberos_service_names.is_empty() {
annotations.insert(
Annotation::kerberos_service_names(&self.kerberos_service_names)
.context(ParseAnnotationSnafu)?,
);
annotations.extend([
annotation::well_known::secret_volume::kerberos_service_names(
&self.kerberos_service_names,
)
.context(ParseAnnotationSnafu)?,
]);
}

if let Some(password) = &self.tls_pkcs12_password {
// The `tls_pkcs12_password` is only used for PKCS12 stores.
if Some(SecretFormat::TlsPkcs12) != self.format {
warn!(format.actual = ?self.format, format.expected = ?Some(SecretFormat::TlsPkcs12), "A TLS PKCS12 password was set but ignored because another format was requested")
} else {
annotations.insert(
Annotation::tls_pkcs12_password(password).context(ParseAnnotationSnafu)?,
);
annotations.extend([annotation::well_known::secret_volume::tls_pkcs12_password(
password,
)
.context(ParseAnnotationSnafu)?]);
}
}

if let Some(lifetime) = &self.auto_tls_cert_lifetime {
annotations.insert(
Annotation::auto_tls_cert_lifetime(&lifetime.to_string())
.context(ParseAnnotationSnafu)?,
);
annotations.extend([
annotation::well_known::secret_volume::auto_tls_cert_lifetime(
&lifetime.to_string(),
)
.context(ParseAnnotationSnafu)?,
]);
}

Ok(EphemeralVolumeSource {
Expand Down Expand Up @@ -464,7 +475,7 @@ pub enum ListenerOperatorVolumeSourceBuilderError {
/// # use std::collections::BTreeMap;
/// let mut pod_builder = PodBuilder::new();
///
/// let labels: Labels = Labels::try_from(BTreeMap::<String, String>::new()).unwrap();
/// let labels: Labels = Labels::new();
///
/// let volume_source =
/// ListenerOperatorVolumeSourceBuilder::new(
Expand Down Expand Up @@ -566,8 +577,6 @@ impl ListenerOperatorVolumeSourceBuilder {

#[cfg(test)]
mod tests {
use std::collections::BTreeMap;

use k8s_openapi::apimachinery::pkg::api::resource::Quantity;

use super::*;
Expand Down Expand Up @@ -630,7 +639,7 @@ mod tests {

#[test]
fn listener_operator_volume_source_builder() {
let labels: Labels = Labels::try_from(BTreeMap::<String, String>::new()).unwrap();
let labels: Labels = Labels::new();

let builder = ListenerOperatorVolumeSourceBuilder::new(
&ListenerReference::ListenerClass("public".into()),
Expand Down
10 changes: 6 additions & 4 deletions crates/stackable-operator/src/cluster_resources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,9 @@ use crate::{
},
crd::listener,
kvp::{
Label, LabelError, Labels,
LabelError, Labels,
consts::{K8S_APP_INSTANCE_KEY, K8S_APP_MANAGED_BY_KEY, K8S_APP_NAME_KEY},
label,
},
utils::format_full_controller_name,
};
Expand Down Expand Up @@ -500,16 +501,17 @@ impl ClusterResources {
/// Return required labels for cluster resources to be uniquely identified for clean up.
// TODO: This is a (quick-fix) helper method but should be replaced by better label handling
pub fn get_required_labels(&self) -> Result<Labels, LabelError> {
let mut labels = Labels::common(&self.app_name, &self.app_instance)?;
let mut labels = label::well_known::sets::common(&self.app_name, &self.app_instance)?;

labels.insert(Label::managed_by(
labels.extend([label::well_known::managed_by(
&self.operator_name,
&self.controller_name,
)?);
)?]);

Ok(labels)
}

// TODO (@Techassi): T should guarantee (by it's type) that required labels are set.
/// Adds a resource to the cluster resources.
///
/// The resource will be patched and the patched resource will be returned.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use serde::{Deserialize, Serialize};
use strum::AsRefStr;

#[cfg(doc)]
use crate::kvp::Labels;
use crate::kvp::label;

pub const STACKABLE_DOCKER_REPO: &str = "oci.stackable.tech/sdp";

Expand Down Expand Up @@ -67,7 +67,7 @@ pub struct ResolvedProductImage {
/// Version of the product, e.g. `1.4.1`.
pub product_version: String,

/// App version as formatted for [`Labels::recommended`]
/// App version as formatted for [`label::sets::recommended`]
pub app_version_label: String,

/// Image to be used for the product image e.g. `oci.stackable.tech/sdp/superset:1.4.1-stackable2.1.0`
Expand Down
Loading
Loading