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

Disambiguate duplicate Member names #206

Merged
merged 10 commits into from
Mar 10, 2024
Merged

Disambiguate duplicate Member names #206

merged 10 commits into from
Mar 10, 2024

Conversation

clux
Copy link
Member

@clux clux commented Mar 9, 2024

This adds a safer name mutation strategy in output.rs, by checking to see if a pascal/snake rename would generate a duplicate name, and then suffixing the name if it does to keep it rust compatible.

This is to fix certain pathological cases like #165.

This solution does not drop information, and always manages to include everything in the struct (even if we don't know which one will eventually go away, like the case with relabeling actions).

Annotated Example

Using the PodMonitor CRD with the clashing relabeling actions, this is what we now produce on the the struct by default (minus my explaining comments about where the member name came from):

#[derive(Serialize, Deserialize, Clone, Debug)]
pub enum PodMonitorPodMetricsEndpointsMetricRelabelingsAction {
    #[serde(rename = "replace")]
    Replace, // originally named "replace" in the spec (a deprecated name)
    #[serde(rename = "Replace")]
    ReplaceX, // originally named "Replace" in the spec
    #[serde(rename = "keep")]
    Keep, // originally named "keep" in the spec (deprecated)
    #[serde(rename = "Keep")]
    KeepX, // originally named "Keep" in the spec

   
    #[serde(rename = "labelmap")]
    Labelmap, // originally named "labelmap" in the spec (deprecated)
    LabelMap, // originally named "LabelMap" in the spec
    /// ....
}

Caveats

  • You might have to rename the code down the line if "deprecated" attributes got removed (removing the need for the suffix)
  • You can end up with extremely similar member names, but there's also no way to avoid this without actually removing variants we would ideally want to remove (no deprecated attrs). You reap what you sow.

Maybe in the future if the OpenAPI spec can include deprecated attrs, we can remove this logic.

(This PR was originally trying to deduplicate members by name, but this ended up picking the wrong names, leaving the good ones unreachable by chance.)

clux added 6 commits March 9, 2024 11:16
…nt case

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux linked an issue Mar 9, 2024 that may be closed by this pull request
clux added 2 commits March 9, 2024 20:10
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux changed the title Deduplicate clashing member names Disambiguate duplicate Member names Mar 9, 2024
@clux clux marked this pull request as ready for review March 9, 2024 20:30
@clux clux mentioned this pull request Mar 9, 2024
sebhoss added a commit to metio/kube-custom-resources-rs that referenced this pull request Mar 10, 2024
Signed-off-by: Sebastian Hoß <seb@xn--ho-hia.de>
@sebhoss
Copy link
Contributor

sebhoss commented Mar 10, 2024

I like this a lot & it completely removes the need for my home grown solution (I have used yq to remove duplicate fields before passing the modified YAML to kopium). Running this against all CRDs in kube-custom-resources-rs yields a single warning though: The natOutgoing and nat-outgoing fields in an Calico IPPool resource are not enums and thus should use snake_case. The generated code is here: https://github.com/metio/kube-custom-resources-rs/blob/kopium-206/kube-custom-resources-rs/src/crd_projectcalico_org/v1/ippools.rs#L38

Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented Mar 10, 2024

thanks @sebhoss , have added a smarter suffixing var to deal with the differences between struct and enum members now!

@clux
Copy link
Member Author

clux commented Mar 10, 2024

also nice to see all that manual override code go away 😄

@sebhoss
Copy link
Contributor

sebhoss commented Mar 10, 2024

yeah this solution here is a lot better since it retains all enums/fields so people can actually use all possible values (even though they are deprecated upstream).

Great stuff - thanks a lot!

@clux clux merged commit 98e79d0 into main Mar 10, 2024
6 checks passed
@clux clux deleted the multiple-names2 branch March 10, 2024 13:18
sebhoss added a commit to metio/kube-custom-resources-rs that referenced this pull request Mar 11, 2024
Signed-off-by: Sebastian Hoß <seb@xn--ho-hia.de>
sebhoss added a commit to metio/kube-custom-resources-rs that referenced this pull request Mar 11, 2024
Signed-off-by: Sebastian Hoß <seb@xn--ho-hia.de>
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.

duplicate fields generated
2 participants