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

Allow opt-out to PodMonitor duplicate case-clashes #205

Closed
wants to merge 3 commits into from
Closed

Conversation

clux
Copy link
Member

@clux clux commented Mar 9, 2024

Running kopium on the PodMonitor prometheus crd we encounter the strange enum that has two variants (with different cases) that by default produces this:

#[derive(Serialize, Deserialize, Clone, Debug, JsonSchema)]
pub enum PodMonitorPodMetricsEndpointsMetricRelabelingsAction {
    #[serde(rename = "replace")]
    Replace,
    Replace,
    #[serde(rename = "keep")]
    Keep,
    Keep,
    /// .....
}

we can now pass a substring of struct names to opt-out the rename behaviour so that if we run kopium here on podmonitors with:

  kubectl apply --server-side -f podmonitor.crd.yaml
  kopium --no-rename RelabelingsAction podmonitors.monitoring.coreos.com

we end up with a dumber enum that looks like:

#[derive(Serialize, Deserialize, Clone, Debug)]
pub enum PodMonitorPodMetricsEndpointsRelabelingsAction {
    replace,
    Replace,
    keep,
    Keep,
    drop,
    Drop,
    /// ...
}

of course, this will cause rust warnings for non_camel_case_types that will have to be ignored.

@clux clux linked an issue Mar 9, 2024 that may be closed by this pull request
@clux clux changed the title Failing tests for PodMonitor struct case-clashes Allow overriding PodMonitor duplicate case-clashes in enum with opt-out Mar 9, 2024
@clux clux changed the title Allow overriding PodMonitor duplicate case-clashes in enum with opt-out Allow opt-out to PodMonitor duplicate case-clashes Mar 9, 2024
Signed-off-by: clux <[email protected]>
@clux clux marked this pull request as ready for review March 9, 2024 12:02
@clux clux mentioned this pull request Mar 9, 2024
@clux clux requested a review from Dav1dde March 9, 2024 12:04
@clux
Copy link
Member Author

clux commented Mar 9, 2024

some counterpoints to this;

  • the generated output is awkward (due to the case ignore)
  • it requires users to explicitly pass a complicated path and the fix is not super discoverable (not sure if there are good places to make suggestions)
  • it won't fix ALL cases where the original case is invalid in rust ($action and action in the policy from comment)
  • semi-complex interplay between the analyzer and the output module

@clux
Copy link
Member Author

clux commented Mar 9, 2024

..maybe it is better to look for duplicate keys on the Container side (as per comment) and if we find any duplicates, THEN remove duplicates instead.. EDIT: trying this in #206

@clux
Copy link
Member Author

clux commented Mar 9, 2024

After examining approaches, this PR here would force a hard-to-discover user opt-in to an inferior solution (with warnings generated) that would likely need further massaging on the user side to be nice.

Closing this in favour of #206 which is actually usable by default despite sometimes producing anachronistic member names.

@clux clux closed this Mar 9, 2024
@clux clux deleted the multiple-names branch March 9, 2024 20:33
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
1 participant