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 696d389
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 18 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),

Check warning on line 243 in kube-derive/src/custom_resource.rs

View check run for this annotation

Codecov / codecov/patch

kube-derive/src/custom_resource.rs#L242-L243 

Added lines #L242 - L243 were not covered by tests
);
}
}
"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),

Check warning on line 253 in kube-derive/src/custom_resource.rs

View check run for this annotation

Codecov / codecov/patch

kube-derive/src/custom_resource.rs#L252-L253 

Added lines #L252 - L253 were not covered by tests
);
}
}
"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),

Check warning on line 263 in kube-derive/src/custom_resource.rs

View check run for this annotation

Codecov / codecov/patch

kube-derive/src/custom_resource.rs#L262-L263 

Added lines #L262 - L263 were not covered by tests
);
}
}
other => return Err(darling::Error::unknown_field(other)),
other => errors.push(darling::Error::unknown_field(other)),

Check warning on line 267 in kube-derive/src/custom_resource.rs

View check run for this annotation

Codecov / codecov/patch

kube-derive/src/custom_resource.rs#L267 

Added line #L267 was not covered by tests
}
}
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"));

Check warning on line 277 in kube-derive/src/custom_resource.rs

View check run for this annotation

Codecov / codecov/patch

kube-derive/src/custom_resource.rs#L277 

Added line #L277 was not covered by tests
}
Expand All @@ -270,8 +281,10 @@ impl FromMeta for Scale {
errors.push(darling::Error::missing_field("status_replicas_path"));

Check warning on line 281 in kube-derive/src/custom_resource.rs

View check run for this annotation

Codecov / codecov/patch

kube-derive/src/custom_resource.rs#L281 

Added line #L281 was not covered by tests
}

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
41 changes: 40 additions & 1 deletion 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 @@ -231,6 +245,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 +380,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 +410,6 @@ fn test_crd_schema_matches_expected() {
"type": "object"
}
},
"subresources": {},
}
]
}
Expand Down

0 comments on commit 696d389

Please sign in to comment.