Skip to content

[Merged by Bors] - Ensure that explicit YAML documents are generated for CRDs - #450

Closed
siegfriedweber wants to merge 4 commits into
mainfrom
yaml_module
Closed

[Merged by Bors] - Ensure that explicit YAML documents are generated for CRDs#450
siegfriedweber wants to merge 4 commits into
mainfrom
yaml_module

Conversation

@siegfriedweber

@siegfriedweber siegfriedweber commented Aug 2, 2022

Copy link
Copy Markdown
Member

Description

  • YAML module added with a function to serialize a data structure as an explicit YAML document. The YAML documents generated by the functions in crd::CustomResourceExt are now explicit documents and can be safely concatenated to produce a YAML stream.
  • serde_yaml 0.8.26 -> 0.9.9

CRDs can now safely be concatenated by using CustomResourceExt::print_yaml_schema in a row:

-//! use kube::CustomResourceExt;
+//! use stackable_operator::CustomResourceExt;

 //! let opts = Opts::from_args();
 //!
 //! match opts.command {
-//!     cli::Command::Crd => println!(
-//!         "{}{}",
-//!         serde_yaml::to_string(&FooCluster::crd())?,
-//!         serde_yaml::to_string(&BarCluster::crd())?,
-//!     ),
+//!     cli::Command::Crd => {
+//!         FooCluster::print_yaml_schema()?;
+//!         BarCluster::print_yaml_schema()?;
+//!     },
 //!     cli::Command::Run { .. } => {
 //!         // Run the operator
 //!     }

Hint: If this was the only usage of serde_yaml in an operator then it can be removed from the dependencies.

Review Checklist

  • Code contains useful comments
  • (Integration-)Test cases added (or not applicable)
  • Documentation added (or not applicable)
  • Changelog updated (or not applicable)
  • Cargo.toml only contains references to git tags (not specific commits or branches)

Once the review is done, comment bors r+ (or bors merge) to merge. Further information

@siegfriedweber
siegfriedweber requested a review from a team August 2, 2022 10:16
@siegfriedweber siegfriedweber self-assigned this Aug 2, 2022
@nightkr nightkr changed the title Ensure that explicit YAML documents are generated Ensure that explicit YAML documents are generated for schemas Aug 2, 2022
Comment thread src/yaml.rs Outdated
Comment thread src/commons/s3.rs Outdated
host: host
port: 8080
accessStyle: VirtualHosted
connection: !inline

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, does this come from serde_yaml? We might need to go back to the old tagging format somehow…

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@siegfriedweber siegfriedweber Aug 3, 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.

The proposed solution in serde_yaml:0.9.4 is to annotate every enum field with #[serde(with = "serde_yaml::with::singleton_map")], see dtolnay/serde-yaml#300.

But this solution does not work together with JsonSchema, see GREsau/schemars#89.

One workaround is to use the serde parameters serialize_with and deserialize_with:

#[serde(
    default,
    skip_serializing_if = "Option::is_none",
    serialize_with = "serde_yaml::with::singleton_map::serialize",
    deserialize_with = "serde_yaml::with::singleton_map::deserialize"
)]
pub connection: Option<S3ConnectionDef>,

This becomes quite verbose.

In which cases do the YAML tags actually pose a problem?

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.

serde_yaml:0.9.9 introduced singleton_map_recursive which allows to serialize enums as YAML maps containing one entry in which the key identifies the variant name.

This is now used in stackable_operator::yaml::serialize_to_explicit_document. Further annotations are not necessary in the operators.

@siegfriedweber siegfriedweber mentioned this pull request Aug 2, 2022
5 tasks
@maltesander

Copy link
Copy Markdown
Member

Does it make sense to simply reexport serde_yaml here as well to have consistent versions?
In the OPA PR stackabletech/opa-operator#326 i think we have a problem with different versions (e.g. 0.8.25 in the operator-rs and 0.9 in OPA). The operator-rs has a YamlSerilizationError with source on the serde_yaml error which changed from 0.8 to 0.9.

@nightkr

nightkr commented Aug 2, 2022

Copy link
Copy Markdown
Contributor

@maltesander I believe that usage falls away anyway, if you use the new helper? So you should be able to remove the serde_yaml dependency entirely.

@nightkr nightkr changed the title Ensure that explicit YAML documents are generated for schemas Ensure that explicit YAML documents are generated for CRDs Aug 2, 2022
@maltesander

Copy link
Copy Markdown
Member

@maltesander I believe that usage falls away anyway, if you use the new helper? So you should be able to remove the serde_yaml dependency entirely.

Ok (did not check the PR), thats fine with me then.

@siegfriedweber

Copy link
Copy Markdown
Member Author

We assume that the changes in serde_yaml, especially the generated YAML tags, have a high impact on a lot of projects and that there will adaptations in one of the next serde_yaml releases. So we decided to postpone this pull request until the next release and then reevaluate the situation.

@siegfriedweber
siegfriedweber marked this pull request as ready for review August 22, 2022 10:10
@siegfriedweber
siegfriedweber requested a review from nightkr August 22, 2022 10:10

@nightkr nightkr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice!

@siegfriedweber

Copy link
Copy Markdown
Member Author

bors merge

bors Bot pushed a commit that referenced this pull request Aug 22, 2022
## Description

* YAML module added with a function to serialize a data structure as an explicit YAML document. The YAML documents generated by the functions in `crd::CustomResourceExt` are now explicit documents and can be safely concatenated to produce a YAML stream.
* `serde_yaml` 0.8.26 -> 0.9.9
@bors

bors Bot commented Aug 22, 2022

Copy link
Copy Markdown
Contributor

Pull request successfully merged into main.

Build succeeded:

@bors bors Bot changed the title Ensure that explicit YAML documents are generated for CRDs [Merged by Bors] - Ensure that explicit YAML documents are generated for CRDs Aug 22, 2022
@bors bors Bot closed this Aug 22, 2022
@bors
bors Bot deleted the yaml_module branch August 22, 2022 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants