-
-
Notifications
You must be signed in to change notification settings - Fork 331
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
create a client feature in kube #525 #528
Conversation
cc @kflansburg |
kube/Cargo.toml
Outdated
default = ["native-tls", "client"] | ||
native-tls = ["openssl", "hyper-tls", "tokio-native-tls"] | ||
rustls-tls = ["hyper-rustls", "tokio-rustls"] |
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.
default = ["native-tls", "client"] | |
native-tls = ["openssl", "hyper-tls", "tokio-native-tls"] | |
rustls-tls = ["hyper-rustls", "tokio-rustls"] | |
default = ["native-tls"] | |
native-tls = ["client", "openssl", "hyper-tls", "tokio-native-tls"] | |
rustls-tls = ["client", "hyper-rustls", "tokio-rustls"] |
I think this works.
We can also add client
to other features that doesn't make sense without client
like ws
, oauth
, gzip
.
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 does introduce having features depend on features also, which will make them slightly harder to follow if it gets heavily used. it probably isn't a problem either way, put a static assert for this case in lib.
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.
Thinking more about it, this does seem like a better way to declare feature dependencies than with assertions. Will continue tomorrow
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.
Yeah, features should be additive, but some features depends on another implicitly already. I think it's better to make that explicit.
Not sure how common, but I've seen this pattern used in some crates I've used.
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.
Have added client
to most client-using feature sets and removed the static assert.
Also separated the dependencies into client
, __config
and __non_core
in the hopes that we can make a proper config
feature.
FYI, I was able to build a WASM target using this branch with |
Co-authored-by: kazk <kazk.dev@gmail.com>
Co-authored-by: kazk <kazk.dev@gmail.com>
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 treat client
feature internal? Looks good to me other than that.
hiding the client feat in circle and examples
Ah, yeah, should keep in spirit with this now being an unbreaking change. Did all your suggestions in one commit. |
Co-authored-by: kazk <kazk.dev@gmail.com>
Thanks for the very detailed review! I'll merge this now, release it later / tomorrow. |
Makes pretty much every heavy dependency optional (which does make things a bit confusing), but it's now possible to build
kube
without default-features and you'll get something quite similar tokube-core
, something that is small enough thatkube-derive
can depend on it.I think in theory we can actually make every dep here optional because without default features, all we do is re-export kube-core, but I think the ones i left are in kube-core anyway...
Tried a bit to make a
config
andclient
feature (possible in theory to build config without client, but not the other way around), but there are many dependencies in the shared set atm so it would include some more complicated cfg! selectors. Maybe a future issue.Replaces #526 (that fix is included) and #527 (i think).