-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
feat: Add typed scale argument to derive macro #1656
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
#![allow(clippy::manual_unwrap_or_default)] | ||
|
||
use darling::{FromDeriveInput, FromMeta}; | ||
use k8s_openapi::apiextensions_apiserver::pkg::apis::apiextensions::v1::CustomResourceSubresourceScale; | ||
use proc_macro2::{Ident, Literal, Span, TokenStream}; | ||
use quote::{ToTokens, TokenStreamExt}; | ||
use syn::{parse_quote, Data, DeriveInput, Path, Visibility}; | ||
|
@@ -34,7 +35,12 @@ | |
printcolums: Vec<String>, | ||
#[darling(multiple)] | ||
selectable: Vec<String>, | ||
scale: Option<String>, | ||
|
||
/// Customize the scale subresource, see [Kubernetes docs][1]. | ||
/// | ||
/// [1]: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#scale-subresource | ||
scale: Option<Scale>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the dependency is just for this struct, then maybe we should inline that for the interface instead. It would also avoid having to do the newtyping you do below. |
||
|
||
#[darling(default)] | ||
crates: Crates, | ||
#[darling(multiple, rename = "annotation")] | ||
|
@@ -185,6 +191,107 @@ | |
} | ||
} | ||
|
||
/// A new-type wrapper around [`CustomResourceSubresourceScale`] to support parsing from the | ||
/// `#[kube]` attribute. | ||
#[derive(Debug)] | ||
struct Scale(CustomResourceSubresourceScale); | ||
|
||
// This custom FromMeta implementation is needed for two reasons: | ||
// | ||
// - To enable backwards-compatibility. Up to version 0.97.0 it was only possible to set scale | ||
// subresource values as a JSON string. | ||
Comment on lines
+201
to
+202
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe there's a way to add a deprecated message to the old way? |
||
// - k8s_openapi types don't support being parsed directly from attributes using darling. This | ||
// would require an upstream change, which is highly unlikely to occur. The from_list impl uses | ||
// the derived implementation as inspiration. | ||
impl FromMeta for Scale { | ||
/// This is implemented for backwards-compatibility. It allows that the scale subresource can | ||
/// be deserialized from a JSON string. | ||
fn from_string(value: &str) -> darling::Result<Self> { | ||
let scale = serde_json::from_str(value).map_err(|err| darling::Error::custom(err))?; | ||
Check warning on line 210 in kube-derive/src/custom_resource.rs GitHub Actions / clippy_nightlyredundant closure
Check warning on line 210 in kube-derive/src/custom_resource.rs GitHub Actions / clippy_nightlyredundant closure
|
||
Ok(Self(scale)) | ||
} | ||
|
||
fn from_list(items: &[darling::ast::NestedMeta]) -> darling::Result<Self> { | ||
let mut errors = darling::Error::accumulator(); | ||
|
||
let mut label_selector_path: (bool, Option<Option<String>>) = (false, None); | ||
let mut spec_replicas_path: (bool, Option<String>) = (false, None); | ||
let mut status_replicas_path: (bool, Option<String>) = (false, None); | ||
|
||
for item in items { | ||
match item { | ||
darling::ast::NestedMeta::Meta(meta) => { | ||
let name = darling::util::path_to_string(meta.path()); | ||
|
||
match name.as_str() { | ||
"label_selector_path" => { | ||
let path = errors.handle(darling::FromMeta::from_meta(meta)); | ||
label_selector_path = (true, Some(path)) | ||
} | ||
"spec_replicas_path" => { | ||
let path = errors.handle(darling::FromMeta::from_meta(meta)); | ||
spec_replicas_path = (true, path) | ||
} | ||
"status_replicas_path" => { | ||
let path = errors.handle(darling::FromMeta::from_meta(meta)); | ||
status_replicas_path = (true, path) | ||
} | ||
other => return Err(darling::Error::unknown_field(other)), | ||
} | ||
} | ||
darling::ast::NestedMeta::Lit(lit) => { | ||
errors.push(darling::Error::unsupported_format("literal").with_span(&lit.span())) | ||
} | ||
} | ||
} | ||
|
||
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")); | ||
} | ||
|
||
if !status_replicas_path.0 && status_replicas_path.1.is_none() { | ||
errors.push(darling::Error::missing_field("status_replicas_path")); | ||
} | ||
|
||
errors.finish_with(Self(CustomResourceSubresourceScale { | ||
label_selector_path: label_selector_path.1.unwrap(), | ||
spec_replicas_path: spec_replicas_path.1.unwrap(), | ||
status_replicas_path: status_replicas_path.1.unwrap(), | ||
})) | ||
} | ||
} | ||
|
||
impl Scale { | ||
fn to_tokens(&self, k8s_openapi: &Path) -> TokenStream { | ||
let apiext = quote! { | ||
#k8s_openapi::apiextensions_apiserver::pkg::apis::apiextensions::v1 | ||
}; | ||
|
||
let label_selector_path = self | ||
.0 | ||
.label_selector_path | ||
.as_ref() | ||
.map_or_else(|| quote! { None }, |p| quote! { #p.into() }); | ||
let spec_replicas_path = &self.0.spec_replicas_path; | ||
let status_replicas_path = &self.0.status_replicas_path; | ||
|
||
quote! { | ||
#apiext::CustomResourceSubresourceScale { | ||
label_selector_path: #label_selector_path, | ||
spec_replicas_path: #spec_replicas_path.into(), | ||
status_replicas_path: #status_replicas_path.into() | ||
} | ||
} | ||
} | ||
} | ||
|
||
pub(crate) fn derive(input: proc_macro2::TokenStream) -> proc_macro2::TokenStream { | ||
let derive_input: DeriveInput = match syn::parse2(input) { | ||
Err(err) => return err.to_compile_error(), | ||
|
@@ -439,7 +546,13 @@ | |
.map(|s| format!(r#"{{ "jsonPath": "{s}" }}"#)) | ||
.collect(); | ||
let fields = format!("[ {} ]", fields.join(",")); | ||
let scale_code = if let Some(s) = scale { s } else { "".to_string() }; | ||
let scale = scale.map_or_else( | ||
|| quote! { None }, | ||
|s| { | ||
let scale = s.to_tokens(&k8s_openapi); | ||
quote! { Some(#scale) } | ||
}, | ||
); | ||
|
||
// Ensure it generates for the correct CRD version (only v1 supported now) | ||
let apiext = quote! { | ||
|
@@ -551,11 +664,7 @@ | |
#k8s_openapi::k8s_if_ge_1_30! { | ||
let fields : Vec<#apiext::SelectableField> = #serde_json::from_str(#fields).expect("valid selectableField column json"); | ||
} | ||
let scale: Option<#apiext::CustomResourceSubresourceScale> = if #scale_code.is_empty() { | ||
None | ||
} else { | ||
#serde_json::from_str(#scale_code).expect("valid scale subresource json") | ||
}; | ||
let scale: Option<#apiext::CustomResourceSubresourceScale> = #scale; | ||
let categories: Vec<String> = #serde_json::from_str(#categories_json).expect("valid categories"); | ||
let shorts : Vec<String> = #serde_json::from_str(#short_json).expect("valid shortnames"); | ||
let subres = if #has_status { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels unfortunate. don't really want a derive crate (which is hard to optimise compile wise) to depend on such a big crate.