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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions justfile
Original file line number Diff line number Diff line change
Expand Up @@ -85,5 +85,13 @@ test-istio-destrule:
# NB: this currently fails because of an empty status object with preserve-unknown-fields
cargo test --test runner -- --nocapture

test-podmon:
kubectl apply --server-side -f tests/podmon-crd.yaml
#curl -sSL https://github.com/prometheus-operator/prometheus-operator/raw/main/example/prometheus-operator-crd/monitoring.coreos.com_podmonitors.yaml > tests/podmon-crd.yaml
cargo run --bin kopium -- --no-rename RelabelingsAction podmonitors.monitoring.coreos.com > tests/gen.rs
echo "pub type CR = PodMonitor;" >> tests/gen.rs
kubectl apply -f tests/podmon.yaml
cargo test --test runner -- --nocapture

release:
cargo release minor --execute
59 changes: 58 additions & 1 deletion src/analyzer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#[derive(Default)]
pub struct Config {
pub no_condition: bool,
pub no_rename: Vec<String>,
}

/// Scan a schema for structs and members, and recurse to find all structs
Expand Down Expand Up @@ -172,7 +173,7 @@
// plain enums do not need to recurse, can collect it here
// ....although this makes it impossible for us to handle enums at the top level
// TODO: move this to the top level
let new_result = analyze_enum_properties(en, &next_stack, level, schema)?;
let new_result = analyze_enum_properties(en, &next_stack, level, schema, cfg)?;
results.push(new_result);
} else {
debug!("..not recursing into {} ('{}' is not a container)", key, x)
Expand All @@ -189,6 +190,7 @@
stack: &str,
level: u8,
schema: &JSONSchemaProps,
cfg: &Config,
) -> Result<Container, anyhow::Error> {
let mut members = vec![];
debug!("analyzing enum {}", serde_json::to_string(&schema).unwrap());
Expand Down Expand Up @@ -223,6 +225,7 @@
level,
docs: schema.description.clone(),
is_enum: true,
no_rename: cfg.no_rename.iter().any(|x| stack.contains(x)),
})
}

Expand Down Expand Up @@ -327,6 +330,7 @@
level,
docs: schema.description.clone(),
is_enum: false,
no_rename: cfg.no_rename.iter().any(|x| stack.contains(x)),
})
}

Expand All @@ -351,7 +355,7 @@
"array" => {
let mut simple_inner = None;
if let Some(JSONSchemaPropsOrArray::Schema(ix)) = &s.items {
simple_inner = ix.type_.clone();

Check warning on line 358 in src/analyzer.rs

View workflow job for this annotation

GitHub Actions / clippy

assigning the result of `Clone::clone()` may be inefficient

warning: assigning the result of `Clone::clone()` may be inefficient --> src/analyzer.rs:358:17 | 358 | simple_inner = ix.type_.clone(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `clone_from()`: `simple_inner.clone_from(&ix.type_)` | = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#assigning_clones = note: `#[warn(clippy::assigning_clones)]` on by default
debug!("additional simple inner type: {:?}", simple_inner);
}
// Simple case: additionalProperties contain: {items: {type: K}}
Expand Down Expand Up @@ -724,6 +728,59 @@
assert_eq!(&op.members[3].type_, "");
}

#[test]
fn avoid_rename_on_duplicated_value_containers() {
init();
let schema_str = r#"
properties:
relabelings:
items:
properties:
action:
default: replace
enum:
- replace
- Replace
- keep
- Keep
- labelkeep
- LabelKeep
type: string
modulus:
format: int64
type: integer
type: object
type: array
type: object
"#;
let cfg = Cfg {
no_rename: vec!["RelabelingsAction".into()],
..Cfg::default()
};
let schema: JSONSchemaProps = serde_yaml::from_str(schema_str).unwrap();
let structs = analyze(schema, "Endpoint", cfg).unwrap().0;
println!("got {:?}", structs);
let root = &structs[0];
assert_eq!(root.name, "Endpoint");
assert_eq!(&root.members[0].type_, "Option<Vec<EndpointRelabelings>>");
assert!(!root.no_rename); // no-rename NOT set on EP (string not similar)

let rel = &structs[1];
assert_eq!(rel.name, "EndpointRelabelings");
assert_eq!(rel.is_enum, false);
assert_eq!(&rel.members[0].name, "action");
assert_eq!(&rel.members[0].type_, "Option<EndpointRelabelingsAction>");
assert!(!rel.no_rename); // no-rename NOT set EPR (action not in string)

// action enum member
let act = &structs[2];
assert_eq!(act.name, "EndpointRelabelingsAction");
assert_eq!(act.is_enum, true);
// no-rename SET! contains partial struct name
assert!(act.no_rename);
// NB: we verify that this causes no renames in output.rs
}

#[test]
fn enum_string_within_container() {
init();
Expand Down
7 changes: 7 additions & 0 deletions src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,12 @@ struct Kopium {
#[arg(long, short = 'e')]
elide: Vec<String>,

/// Avoid renaming structs matching struct names
///
/// This avoids awkward cases where certain CRDs contain duplicate names with the different case
#[arg(long)]
no_rename: Vec<String>,

/// Enable generation of custom Condition APIs.
///
/// If false, it detects if a particular path is an array of Condition objects and uses a standard
Expand Down Expand Up @@ -192,6 +198,7 @@ impl Kopium {
log::debug!("schema: {}", serde_json::to_string_pretty(&schema)?);
let cfg = Config {
no_condition: self.no_condition,
no_rename: self.no_rename.clone(),
};
let structs = analyze(schema, kind, cfg)?
.rename()
Expand Down
47 changes: 46 additions & 1 deletion src/output.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ pub struct Container {
pub docs: Option<String>,
/// Whether this container is an enum
pub is_enum: bool,
/// Elide rename for this container (when passing no-rename)
pub no_rename: bool,
}

/// Output member belonging to an Container
Expand Down Expand Up @@ -111,7 +113,7 @@ impl Container {
.unwrap_or_else(|| panic!("invalid field name '{}' could not be escaped", m.name))
};

if new_name != m.name {
if new_name != m.name && !self.no_rename {
m.serde_annot.push(format!("rename = \"{}\"", m.name));
m.name = new_name;
}
Expand Down Expand Up @@ -177,3 +179,46 @@ impl Output {
self
}
}

// unit tests
#[cfg(test)]
mod test {
use super::{Container, Member};
fn name_only_enum_member(name: &str) -> Member {
Member {
name: name.to_string(),
type_: "".to_string(),
serde_annot: vec![],
extra_annot: vec![],
docs: None,
}
}

#[test]
fn no_rename_is_respected() {
let mut c = Container {
name: "EndpointRelabelingsAction".to_string(),
level: 1,
members: vec![
name_only_enum_member("replace"),
name_only_enum_member("Replace"),
name_only_enum_member("keep"),
name_only_enum_member("Keep"),
name_only_enum_member("labelkeep"),
name_only_enum_member("LabelKeep"),
],
docs: None,
is_enum: true,
no_rename: true,
};

c.rename();
// should have enum members with ORIGINAL names AFTER rename
assert_eq!(&c.members[0].name, "replace");
assert_eq!(&c.members[1].name, "Replace");
assert_eq!(&c.members[2].name, "keep");
assert_eq!(&c.members[3].name, "Keep");
assert_eq!(&c.members[4].name, "labelkeep");
assert_eq!(&c.members[5].name, "LabelKeep");
}
}
Loading
Loading