-
-
Notifications
You must be signed in to change notification settings - Fork 325
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?
Conversation
This allows cutomizing the scale subresource by providing key-value items instead of a raw JSON string. For backwards-compatibility, it is still supported to provide a JSON string. However, all examples and tests were converted to the new format. Signed-off-by: Techassi <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1656 +/- ##
=======================================
+ Coverage 75.9% 76.0% +0.1%
=======================================
Files 84 84
Lines 7611 7664 +53
=======================================
+ Hits 5771 5817 +46
- Misses 1840 1847 +7
|
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.
Hey there. Thanks for this! Apologise for the delay.
I think this makes sense, given you've made it non-breaking, but ideally the dependency on the openapi crate should be severed if it's just for the interface. Have added a few suggestive comments.
// - To enable backwards-compatibility. Up to version 0.97.0 it was only possible to set scale | ||
// subresource values as a JSON string. |
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.
Maybe there's a way to add a deprecated message to the old way?
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.
As far as I know, there is no way to emit warning from within derive macros, only hard-errors, aka compile_error!()
.
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.
I instead opted to add a note to the relevant setion in the doc comment for the derive macro. See a56835e.
No worries. I will shortly take a look at your comments. I will also see if we can get rid of the |
Sorry for the delayed work on this, but I think I have addressed both of your biggest concerns in d93d708. |
Signed-off-by: Techassi <[email protected]>
Signed-off-by: Techassi <[email protected]>
Signed-off-by: Techassi <[email protected]>
Signed-off-by: Techassi <[email protected]>
Signed-off-by: Techassi <[email protected]>
0a98def
to
7a86117
Compare
/// 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> { | ||
serde_json::from_str(value).map_err(darling::Error::custom) |
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.
I see most of the lines for this impl is showing up as untested. I think we can get easy coverage by adding the invocation to kube-derive/tests/crd_schema_test.rs and updating the schema changes in that file.
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.
Sure thing, I can add some tests to test both the old and the new way.
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.
Adding a test proved very valuable! Take a look at 696d389.
The addition also helps with coverage and greatly reduces the amount of missing lines. I have a few ideas how this can be improved even further in the future. I will create a PR for it soon.
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]>
cccc052
to
696d389
Compare
Motivation
Customizing the
scale
subresource via#[kube(scale = r#"{}"#)]
can feel very out-of-place and requires writing a raw JSON string. In addition, this can lead to a very long line, eg:Solution
This allows customizing the scale subresource by providing key-value items instead of a raw JSON string. For backwards-compatibility, it is still supported to provide a JSON string. However, all examples and tests were converted to the new format.