Skip to content

Commit

Permalink
test: Add schema test, fix FromMeta implementation
Browse files Browse the repository at this point in the history 
Adding this test proved to be very valuable because the FromMeta
implemenetation had a few errors and resulted in different panic
messages coming from the derive macro.

I also added a small note to the #[kube(scale(...))] section stating
that the scale subresource can only be used when the status subresource
is used as well. I plan to further improve the validation in a future
pull request.

Signed-off-by: Techassi <[email protected]>
  • Loading branch information
@Techassi
Techassi committed Jan 25, 2025
1 parent 7a86117 commit cccc052
Show file tree
Hide file tree
Showing 3 changed files with 90 additions and 33 deletions.
47 changes: 30 additions & 17 deletions kube-derive/src/custom_resource.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,18 +235,36 @@ impl FromMeta for Scale {

match name.as_str() {
"label_selector_path" => {
let path = errors.handle(darling::FromMeta::from_meta(meta));
label_selector_path = (true, Some(path))
if !label_selector_path.0 {
let path = errors.handle(darling::FromMeta::from_meta(meta));
label_selector_path = (true, Some(path))
} else {
errors.push(
darling::Error::duplicate_field("label_selector_path").with_span(&meta),
);
}
}
"spec_replicas_path" => {
let path = errors.handle(darling::FromMeta::from_meta(meta));
spec_replicas_path = (true, path)
if !spec_replicas_path.0 {
let path = errors.handle(darling::FromMeta::from_meta(meta));
spec_replicas_path = (true, path)
} else {
errors.push(
darling::Error::duplicate_field("spec_replicas_path").with_span(&meta),
);
}
}
"status_replicas_path" => {
let path = errors.handle(darling::FromMeta::from_meta(meta));
status_replicas_path = (true, path)
if !status_replicas_path.0 {
let path = errors.handle(darling::FromMeta::from_meta(meta));
status_replicas_path = (true, path)
} else {
errors.push(
darling::Error::duplicate_field("status_replicas_path").with_span(&meta),
);
}
}
other => return Err(darling::Error::unknown_field(other)),
other => errors.push(darling::Error::unknown_field(other)),
}
}
darling::ast::NestedMeta::Lit(lit) => {
Expand All @@ -255,13 +273,6 @@ impl FromMeta for Scale {
}
}

if !label_selector_path.0 {
match <Option<String> as darling::FromMeta>::from_none() {
Some(fallback) => label_selector_path.1 = Some(fallback),
None => errors.push(darling::Error::missing_field("spec_replicas_path")),
}
}

if !spec_replicas_path.0 && spec_replicas_path.1.is_none() {
errors.push(darling::Error::missing_field("spec_replicas_path"));
}
Expand All @@ -270,8 +281,10 @@ impl FromMeta for Scale {
errors.push(darling::Error::missing_field("status_replicas_path"));
}

errors.finish_with(Self {
label_selector_path: label_selector_path.1.unwrap(),
errors.finish()?;

Ok(Self {
label_selector_path: label_selector_path.1.unwrap_or_default(),
spec_replicas_path: spec_replicas_path.1.unwrap(),
status_replicas_path: status_replicas_path.1.unwrap(),
})
Expand All @@ -287,7 +300,7 @@ impl Scale {
let label_selector_path = self
.label_selector_path
.as_ref()
.map_or_else(|| quote! { None }, |p| quote! { #p.into() });
.map_or_else(|| quote! { None }, |p| quote! { Some(#p.into()) });
let spec_replicas_path = &self.spec_replicas_path;
let status_replicas_path = &self.status_replicas_path;

Expand Down
2 changes: 2 additions & 0 deletions kube-derive/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,8 @@ mod resource;
/// ## `#[kube(scale(...))]`
///
/// Allow customizing the scale struct for the [scale subresource](https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#subresources).
/// It should be noted, that the status subresource must also be enabled to use the scale subresource. This is because
/// the `statusReplicasPath` only accepts JSONPaths under `.status`.
///
/// ```ignore
/// #[kube(scale(
Expand Down
74 changes: 58 additions & 16 deletions kube-derive/tests/crd_schema_test.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#![allow(missing_docs)]
#![recursion_limit = "256"]

use assert_json_diff::assert_json_eq;
Expand Down Expand Up @@ -29,6 +30,12 @@ use std::collections::{HashMap, HashSet};
label("clux.dev", "cluxingv1"),
label("clux.dev/persistence", "disabled"),
rule = Rule::new("self.metadata.name == 'singleton'"),
status = "Status",
scale(
spec_replicas_path = ".spec.replicas",
status_replicas_path = ".status.replicas",
label_selector_path = ".status.labelSelector"
),
)]
#[cel_validate(rule = Rule::new("has(self.nonNullable)"))]
#[serde(rename_all = "camelCase")]
Expand Down Expand Up @@ -62,6 +69,13 @@ struct FooSpec {
set: HashSet<String>,
}

#[derive(Clone, Debug, PartialEq, Deserialize, Serialize, JsonSchema)]
#[serde(rename_all = "camelCase")]
pub struct Status {
replicas: usize,
label_selector: String,
}

fn default_value() -> String {
"default_value".into()
}
Expand Down Expand Up @@ -140,21 +154,24 @@ fn test_shortnames() {
#[test]
fn test_serialized_matches_expected() {
assert_json_eq!(
serde_json::to_value(Foo::new("bar", FooSpec {
non_nullable: "asdf".to_string(),
non_nullable_with_default: "asdf".to_string(),
nullable_skipped: None,
nullable: None,
nullable_skipped_with_default: None,
nullable_with_default: None,
timestamp: DateTime::from_timestamp(0, 0).unwrap(),
complex_enum: ComplexEnum::VariantOne { int: 23 },
untagged_enum_person: UntaggedEnumPerson::GenderAndAge(GenderAndAge {
age: 42,
gender: Gender::Male,
}),
set: HashSet::from(["foo".to_owned()])
}))
serde_json::to_value(Foo::new(
"bar",
FooSpec {
non_nullable: "asdf".to_string(),
non_nullable_with_default: "asdf".to_string(),
nullable_skipped: None,
nullable: None,
nullable_skipped_with_default: None,
nullable_with_default: None,
timestamp: DateTime::from_timestamp(0, 0).unwrap(),
complex_enum: ComplexEnum::VariantOne { int: 23 },
untagged_enum_person: UntaggedEnumPerson::GenderAndAge(GenderAndAge {
age: 42,
gender: Gender::Male,
}),
set: HashSet::from(["foo".to_owned()])
}
))
.unwrap(),
serde_json::json!({
"apiVersion": "clux.dev/v1",
Expand Down Expand Up @@ -231,6 +248,14 @@ fn test_crd_schema_matches_expected() {
}, {
"jsonPath": ".spec.nullable"
}],
"subresources": {
"status": {},
"scale": {
"specReplicasPath": ".spec.replicas",
"labelSelectorPath": ".status.labelSelector",
"statusReplicasPath": ".status.replicas"
}
},
"schema": {
"openAPIV3Schema": {
"description": "Custom resource representing a Foo",
Expand Down Expand Up @@ -358,6 +383,24 @@ fn test_crd_schema_matches_expected() {
"rule": "has(self.nonNullable)",
}],
"type": "object"
},
"status": {
"properties": {
"replicas": {
"type": "integer",
"format": "uint",
"minimum": 0.0,
},
"labelSelector": {
"type": "string"
}
},
"required": [
"labelSelector",
"replicas"
],
"nullable": true,
"type": "object"
}
},
"required": [
Expand All @@ -370,7 +413,6 @@ fn test_crd_schema_matches_expected() {
"type": "object"
}
},
"subresources": {},
}
]
}
Expand Down

0 comments on commit cccc052

Please sign in to comment.