Skip to content
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 storage and served argument to derive macro #1644

Merged
merged 5 commits into from
Nov 22, 2024

Conversation

Techassi
Copy link
Contributor

This PR adds two new arguments to the derive macro: storage and served. This allows users to customize these two values in the schema.

Motivation

Currently it is not possible to customize these two properties using the derive macro because they are hard-coded to be both true. Adding support for setting these two properties is highly valuable when a CRD with multiple versions is used.

Solution

Add two new arguments to make the properties customizable.

@Techassi Techassi force-pushed the feat/derive-add-served-storage-arg branch from 5c10fce to 77b699f Compare November 21, 2024 12:33
@Techassi Techassi force-pushed the feat/derive-add-served-storage-arg branch from 77b699f to fda8d52 Compare November 21, 2024 12:55
Copy link

codecov bot commented Nov 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.6%. Comparing base (fa7b7f2) to head (99d3ebc).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1644     +/-   ##
=======================================
+ Coverage   75.5%   75.6%   +0.1%     
=======================================
  Files         82      82             
  Lines       7403    7405      +2     
=======================================
+ Hits        5589    5591      +2     
  Misses      1814    1814             
Files with missing lines Coverage Δ
kube-derive/src/custom_resource.rs 82.5% <100.0%> (+0.2%) ⬆️
kube-derive/src/lib.rs 0.0% <ø> (ø)
kube-derive/tests/crd_schema_test.rs 96.9% <ø> (ø)
---- 🚨 Try these New Features:

@clux
Copy link
Member

clux commented Nov 21, 2024

Hey, thanks for this.

I think this is reasonable, but just wondering if you've seen the merge_crd fn that aims to help with doing multi-version. If you are setting these flags, then presumably you need to merge your multiple crd schemas anyway, and if you are, perhaps there are things we can do to help with the merge behaviour instead? If not, it would at least be interesting to hear about the use-case.

@Techassi
Copy link
Contributor Author

but just wondering if you've seen the merge_crd fn that aims to help with doing multi-version.

Yes, and I'm using it. Afaik the merge_crd function only touches the storage property and sets all versions to false other than the specified apiversion.

The Kubernetes docs recommend setting served to false for removed versions. That's why I introduced the served argument. In addition to that I also added the storage argument while at it.

it would at least be interesting to hear about the use-case.

Sure, you can check my current work out here: https://github.com/stackabletech/operator-rs/tree/main/crates/stackable-versioned-macros

@clux clux added the changelog-add changelog added category for prs label Nov 21, 2024
@clux clux added this to the 0.98.0 milestone Nov 21, 2024
@clux
Copy link
Member

clux commented Nov 21, 2024

Okay, yeah, that's way more advanced than I've had to deal with. Happy to send this through. Thank you!

@Techassi
Copy link
Contributor Author

Techassi commented Nov 21, 2024

Cool, thanks!

Seems like I need to re-format using an older Rust edition (I used my default formatter, which uses newer formatting rules). Just out of curiosity, why do you still format the code according to the 2018 edition?

@clux
Copy link
Member

clux commented Nov 21, 2024

Just out of curiosity, why do you still format the code according to the 2018 edition?

The important part there is that it's nightly rather than stable, and this forces a custom command rather than LSP defaults. I don't think the edition matters a whole deal, although we should also bump it (EDIT: done).

The nightly things we use are here #707 . Sadly they've taken a very long time to stabilise.

@Techassi Techassi force-pushed the feat/derive-add-served-storage-arg branch from 7ad3329 to 99d3ebc Compare November 22, 2024 08:47
@Techassi
Copy link
Contributor Author

The important part there is that it's nightly rather than stable

Ah, okay! Some of these rules are available in other tooling tho, such as rust-analyzer, but I can see that you don't want to rely on that.

I re-applied the formatting and the CI checks should now succeed.

@clux
Copy link
Member

clux commented Nov 22, 2024

Looks great, thank you!

@clux clux merged commit 9f93e2f into kube-rs:main Nov 22, 2024
17 checks passed
@Techassi Techassi deleted the feat/derive-add-served-storage-arg branch November 22, 2024 10:39
@Techassi
Copy link
Contributor Author

Lovely, thank you as well!

@Techassi
Copy link
Contributor Author

As I know of no better way, I'll just ping you here that I have at least one more PR planned for the derive macro. The upcoming change could also be included in the 0.98.0 release.

Also, what is the timeline for 0.98.0 so that I can plan accordingly?

@clux
Copy link
Member

clux commented Nov 22, 2024

Sounds good! Normally I only really try to do roughly one minor a month, but it's not set in stone. If there's an k8s-openapi release (at some point after kubernetes 1.32 releases) we always release regardless. That might be a good one to aim for.

If not, happy to do to one when you are at a reasonable checkpoint with derive.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants