Skip to content
Closed
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
11 changes: 11 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,17 @@ All notable changes to this project will be documented in this file.

## [Unreleased]

### Added

- Add wrapper for `serde_yaml` which adheres to the YAML specification.
Operators should use the `yaml` module instead of `serde_yaml` ([#448]).

### Changed

- serde\_yaml 0.8.26 -> 0.9 ([#448])

[#448]: https://github.com/stackabletech/operator-rs/pull/448

## [0.23.0] - 2022-07-26

### Added
Expand Down
4 changes: 2 additions & 2 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ regex = "1.6.0"
schemars = "0.8.10"
serde = { version = "1.0.140", features = ["derive"] }
serde_json = "1.0.82"
serde_yaml = "0.8.26"
serde_yaml = "0.9"
strum = { version = "0.24.1", features = ["derive"] }
thiserror = "1.0.31"
tokio = { version = "1.20.1", features = ["macros", "rt-multi-thread"] }
Expand All @@ -39,7 +39,7 @@ stackable-operator-derive = { path = "stackable-operator-derive" }
[dev-dependencies]
rstest = "0.15.0"
tempfile = "3.3.0"
serde_yaml = "0.8"
serde_yaml = "0.9"

[features]
default = ["native-tls"]
Expand Down
5 changes: 3 additions & 2 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
//! use serde::{Deserialize, Serialize};
//! use stackable_operator::cli;
//! use stackable_operator::error::OperatorResult;
//! use stackable_operator::yaml;
//!
//! #[derive(Clone, CustomResource, Debug, JsonSchema, Serialize, Deserialize)]
//! #[kube(
Expand Down Expand Up @@ -57,8 +58,8 @@
//! match opts.command {
//! cli::Command::Crd => println!(
//! "{}{}",
//! serde_yaml::to_string(&FooCluster::crd())?,
//! serde_yaml::to_string(&BarCluster::crd())?,
//! yaml::to_string(&FooCluster::crd())?,
//! yaml::to_string(&BarCluster::crd())?,
//! ),
//! cli::Command::Run { .. } => {
//! // Run the operator
Expand Down
7 changes: 5 additions & 2 deletions src/commons/opa.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
//! use stackable_operator::kube::CustomResource;
//! use stackable_operator::commons::opa::{OpaApiVersion, OpaConfig};
//! use stackable_operator::schemars::{self, JsonSchema};
//! use stackable_operator::yaml;
//!
//! #[derive(Clone, CustomResource, Debug, Deserialize, JsonSchema, PartialEq, Serialize)]
//! #[kube(
Expand All @@ -25,7 +26,7 @@
//! opa: Option<OpaConfig>
//! }
//!
//! let cluster: TestCluster = serde_yaml::from_str(
//! let cluster: TestCluster = yaml::from_str(
//! "
//! apiVersion: test.stackable.tech/v1alpha1
//! kind: TestCluster
Expand Down Expand Up @@ -234,6 +235,8 @@ impl OpaConfig {

#[cfg(test)]
mod tests {
use crate::yaml;

use super::*;
use kube::CustomResource;
use schemars::{self, JsonSchema};
Expand Down Expand Up @@ -314,7 +317,7 @@ mod tests {
}

fn build_test_cluster() -> TestCluster {
serde_yaml::from_str(&format!(
yaml::from_str(&format!(
"
apiVersion: test/v1
kind: TestCluster
Expand Down
9 changes: 5 additions & 4 deletions src/commons/resources.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ where
mod tests {
use crate::commons::resources::{PvcConfig, Resources};
use crate::config::merge::Merge;
use crate::yaml;
use k8s_openapi::api::core::v1::{PersistentVolumeClaim, ResourceRequirements};
use rstest::rstest;
use serde::{Deserialize, Serialize};
Expand Down Expand Up @@ -320,12 +321,12 @@ mod tests {
#[case] input: String,
#[case] expected: String,
) {
let input_pvcconfig: PvcConfig = serde_yaml::from_str(&input).expect("illegal test input");
let input_pvcconfig: PvcConfig = yaml::from_str(&input).expect("illegal test input");

let result = input_pvcconfig.build_pvc(&name, access_modes);

let expected_volumeclaim: PersistentVolumeClaim =
serde_yaml::from_str(&expected).expect("illegal expected output");
yaml::from_str(&expected).expect("illegal expected output");

assert_eq!(result, expected_volumeclaim);
}
Expand Down Expand Up @@ -369,11 +370,11 @@ mod tests {
)]
fn test_into_resourcelimits(#[case] input: String, #[case] expected: String) {
let input_resources: Resources<TestStorageConfig> =
serde_yaml::from_str(&input).expect("illegal test input");
yaml::from_str(&input).expect("illegal test input");

let result: ResourceRequirements = input_resources.into();
let expected_requirements: ResourceRequirements =
serde_yaml::from_str(&expected).expect("illegal expected output");
yaml::from_str(&expected).expect("illegal expected output");

assert_eq!(result, expected_requirements);
}
Expand Down
12 changes: 6 additions & 6 deletions src/commons/s3.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,7 @@ impl Default for S3AccessStyle {
mod test {
use crate::commons::s3::{S3AccessStyle, S3ConnectionDef};
use crate::commons::s3::{S3BucketSpec, S3ConnectionSpec};
use crate::yaml;

#[test]
fn test_ser_inline() {
Expand All @@ -235,14 +236,13 @@ mod test {
};

assert_eq!(
serde_yaml::to_string(&bucket).unwrap(),
yaml::to_string(&bucket).unwrap(),
"---
bucketName: test-bucket-name
connection:
inline:
host: host
port: 8080
accessStyle: VirtualHosted
connection: !inline
host: host
port: 8080
accessStyle: VirtualHosted
"
.to_owned()
)
Expand Down
3 changes: 2 additions & 1 deletion src/crd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use schemars::JsonSchema;
use serde::{Deserialize, Serialize};

use crate::error::{Error, OperatorResult};
use crate::yaml;
use std::fs::File;
use std::io::Write;
use std::path::Path;
Expand Down Expand Up @@ -76,7 +77,7 @@ pub trait CustomResourceExt: kube::CustomResourceExt {
where
W: Write,
{
let schema = serde_yaml::to_string(&Self::crd())?;
let schema = yaml::to_string(&Self::crd())?;
writer.write_all(schema.as_bytes())?;
Ok(())
}
Expand Down
4 changes: 2 additions & 2 deletions src/error.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
use crate::product_config_utils;
use crate::{product_config_utils, yaml};
use std::path::PathBuf;

#[derive(Debug, thiserror::Error)]
pub enum Error {
#[error("Failed to serialize YAML: {source}")]
YamlSerializationError {
#[from]
source: serde_yaml::Error,
source: yaml::Error,
},

#[error("Kubernetes reported error: {source}")]
Expand Down
1 change: 1 addition & 0 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ pub mod product_config_utils;
pub mod role_utils;
pub mod utils;
pub mod validation;
pub mod yaml;

pub use crate::crd::CustomResourceExt;

Expand Down
65 changes: 65 additions & 0 deletions src/yaml.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
//! Wrapper around serde_yaml which adheres to the YAML specification

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think strictly speaking the --- are only mandatory if the file starts with directives, to delimit these from the actual content of the file.
So arguably serde_yaml adheres to the spec, even though I have to say that I do not agree with the upstream change.
Besides that, it might be worthwhile just adding one sentence about what exactly the difference is between this and serde_yaml to make it easier to understand.

@siegfriedweber siegfriedweber Aug 2, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In YAML 1.0 and 1.1, a YAML stream can start with an "implicit document" (without dashes) or an "explicit document" (with dashes), see https://yaml.org/spec/1.0/index.html#id2561559 and https://yaml.org/spec/1.1/current.html#id898785.

It is the same in YAML 1.2 but "implicit documents" are called "bare documents", see https://yaml.org/spec/1.2.2/#913-bare-documents.

So serde_yaml is compliant with the YAML specification. I will change my pull request.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth opening an issue against yamllint for this?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a closed issue with this topic: adrienverge/yamllint#302

//!
//! Operators should use this module instead of serde_yaml.
use serde::ser;

use crate::error::OperatorResult;

pub use serde_yaml::from_str;
pub use serde_yaml::Error;

/// Serialize the given data structure as a String of YAML.
///
/// Serialization can fail if `T`'s implementation of `Serialize` decides to
/// return an error.
pub fn to_string<T>(value: &T) -> OperatorResult<String>
where
T: ?Sized + ser::Serialize,
{
let yaml = serde_yaml::to_string(value)?;
Ok(format!("---\n{}", yaml))
}

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

use super::*;

#[test]
fn yaml_with_leading_dashes_can_be_deserialized() {
let yaml = "\
---\n\
key: value";

let actual_value: BTreeMap<_, _> = from_str(yaml).expect("deserializable value");

let expected_value: BTreeMap<_, _> = [("key", "value")].into();

assert_eq!(expected_value, actual_value);
}

#[test]
fn yaml_without_leading_dashes_can_be_deserialized() {
let yaml = "key: value";

let actual_value: BTreeMap<_, _> = from_str(yaml).expect("deserializable value");

let expected_value: BTreeMap<_, _> = [("key", "value")].into();

assert_eq!(expected_value, actual_value);
}

#[test]
fn value_can_be_serialized() {
let value: BTreeMap<_, _> = [("key", "value")].into();

let actual_yaml = to_string(&value).expect("serializable value");

let expected_yaml = "\
---\n\
key: value\n";

assert_eq!(expected_yaml, actual_yaml);
}
}