From 696d38945cefc87343dc56d441f869f627382158 Mon Sep 17 00:00:00 2001 From: Techassi Date: Sat, 25 Jan 2025 13:48:24 +0100 Subject: [PATCH] test: Add schema test, fix FromMeta implementation 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 --- kube-derive/src/custom_resource.rs | 47 ++++++++++++++++++---------- kube-derive/src/lib.rs | 2 ++ kube-derive/tests/crd_schema_test.rs | 41 +++++++++++++++++++++++- 3 files changed, 72 insertions(+), 18 deletions(-) diff --git a/kube-derive/src/custom_resource.rs b/kube-derive/src/custom_resource.rs index 12bb9888d..8c760864e 100644 --- a/kube-derive/src/custom_resource.rs +++ b/kube-derive/src/custom_resource.rs @@ -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) => { @@ -255,13 +273,6 @@ impl FromMeta for Scale { } } - if !label_selector_path.0 { - match 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")); } @@ -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(), }) @@ -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; diff --git a/kube-derive/src/lib.rs b/kube-derive/src/lib.rs index 45db03408..f0049282c 100644 --- a/kube-derive/src/lib.rs +++ b/kube-derive/src/lib.rs @@ -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( diff --git a/kube-derive/tests/crd_schema_test.rs b/kube-derive/tests/crd_schema_test.rs index 8e8c5cf07..e7c65bfce 100644 --- a/kube-derive/tests/crd_schema_test.rs +++ b/kube-derive/tests/crd_schema_test.rs @@ -1,3 +1,4 @@ +#![allow(missing_docs)] #![recursion_limit = "256"] use assert_json_diff::assert_json_eq; @@ -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")] @@ -62,6 +69,13 @@ struct FooSpec { set: HashSet, } +#[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() } @@ -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", @@ -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": [ @@ -370,7 +410,6 @@ fn test_crd_schema_matches_expected() { "type": "object" } }, - "subresources": {}, } ] }