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

create a client feature in kube #525 #528

Merged
merged 21 commits into from
May 20, 2021
Merged

create a client feature in kube #525 #528

merged 21 commits into from
May 20, 2021

Conversation

clux
Copy link
Member

@clux clux commented May 19, 2021

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 to kube-core, something that is small enough that kube-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 and client 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).

@clux clux requested a review from kazk May 19, 2021 20:50
@clux
Copy link
Member Author

clux commented May 19, 2021

cc @kflansburg

kube/Cargo.toml Outdated
Comment on lines 19 to 21
default = ["native-tls", "client"]
native-tls = ["openssl", "hyper-tls", "tokio-native-tls"]
rustls-tls = ["hyper-rustls", "tokio-rustls"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Member Author

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.

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

@clux clux May 20, 2021

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.

@kflansburg
Copy link
Contributor

FYI, I was able to build a WASM target using this branch with default-features = false. 👍

Co-authored-by: kazk <kazk.dev@gmail.com>
@kazk kazk linked an issue May 20, 2021 that may be closed by this pull request
Co-authored-by: kazk <kazk.dev@gmail.com>
Copy link
Member

@kazk kazk left a 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
@clux
Copy link
Member Author

clux commented May 20, 2021

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>
@clux
Copy link
Member Author

clux commented May 20, 2021

Thanks for the very detailed review! I'll merge this now, release it later / tomorrow.

@clux clux merged commit 74e4e85 into master May 20, 2021
@clux clux deleted the feature-wrap-kube branch May 20, 2021 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kube-derive now requires explicit kube-core dependency
3 participants