Skip to content
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

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

Closed
wants to merge 4 commits into from

Conversation

siegfriedweber
Copy link
Member

@siegfriedweber siegfriedweber commented Aug 2, 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

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
src/yaml.rs Outdated Show resolved Hide resolved
host: host
port: 8080
accessStyle: VirtualHosted
connection: !inline
Copy link
Member

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
Member

Choose a reason for hiding this comment

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

Copy link
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
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
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
Copy link
Member

nightkr commented Aug 2, 2022

@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
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
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
Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

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

Nice!

@siegfriedweber
Copy link
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
Copy link
Contributor

bors bot commented Aug 22, 2022

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.

None yet

3 participants