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
Conversation
src/commons/s3.rs
Outdated
host: host | ||
port: 8080 | ||
accessStyle: VirtualHosted | ||
connection: !inline |
There was a problem hiding this comment.
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…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Does it make sense to simply reexport serde_yaml here as well to have consistent versions? |
@maltesander I believe that usage falls away anyway, if you use the new helper? So you should be able to remove the |
Ok (did not check the PR), thats fine with me then. |
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. |
…entifies the variant name
b6b1c91
to
44edca4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
bors merge |
## 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
Pull request successfully merged into main. Build succeeded: |
Description
crd::CustomResourceExt
are now explicit documents and can be safely concatenated to produce a YAML stream.serde_yaml
0.8.26 -> 0.9.9CRDs can now safely be concatenated by using
CustomResourceExt::print_yaml_schema
in a row:Hint: If this was the only usage of
serde_yaml
in an operator then it can be removed from the dependencies.Review Checklist
Once the review is done, comment
bors r+
(orbors merge
) to merge. Further information