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

Remove kube Dependency in kube-derive #517

Merged
merged 5 commits into from
May 17, 2021

Conversation

kflansburg
Copy link
Contributor

Closes #516

This gives an idea of how the kube dependency can be removed. A few sharp edges still:

  • We can only reference one path in the derive macro, so they either have to always add kube or always add kube-core to their Cargo manifest.
  • I'm sure there are CI changes that need to be made due to the new crate.

I've also taken the liberty of fixing some Clippy lints that were preventing compilation (in a separate commit).

@clux
Copy link
Member

clux commented May 16, 2021

Hey, thanks a lot for doing this. I was thinking something like this might be helpful for breaking dependencies, but was never sure what it was going to benefit directly.

Fundamentally, I have no qualms with doing this, looks like we can do this in ways that don't break anything and have kube re-export from kube-core. Although, it will probably be a bit of duelling with tests before it's all good. I'm here to help.

@@ -28,7 +28,7 @@ schema = []
[dev-dependencies]
serde = { version = "1.0.118", features = ["derive"] }
serde_yaml = "0.8.17"
kube = { path = "../kube", version = "<1.0.0, >=0.51.0" }
kube-core = { path = "../kube-core" }
Copy link
Member

Choose a reason for hiding this comment

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

Note to self: the version keys need to be in there to avoid cyclic dependencies in publishing, but maybe the first release needs to be special cased.

@kflansburg
Copy link
Contributor Author

Great, I will address your comment and take a pass at fixing the tests in a bit.

@clux
Copy link
Member

clux commented May 16, 2021

There's potentially three more things we could include in this crate. Like the part of the api bindings that doesn't use hyper, I'm thinking:

  • ResourceExt trait (mentioned before)
  • api/params.rs
  • api/request.rs (does not use a client lib)

That way we have a more cohesive story for the non-tokio/non-client wanting crowd.

@kflansburg
Copy link
Contributor Author

Ok, I moved the things you mentioned. Moving params.rs involved redefining a few Error variants, and moving Request involved splitting subresource.rs across the two crates.

I ran most of the tests locally, will see if it passes on Circle.

@kflansburg kflansburg force-pushed the kflansburg/kube-core branch from 5245a28 to a5148f7 Compare May 16, 2021 21:21
@kflansburg kflansburg force-pushed the kflansburg/kube-core branch from a5148f7 to 23fc91b Compare May 16, 2021 21:25
@@ -26,7 +26,7 @@ http = "0.2.2"
[dependencies.k8s-openapi]
version = "0.11.0"
default-features = false
features = []
features = ["v1_20"]
Copy link
Member

Choose a reason for hiding this comment

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

oh, this isn't ideal, we can't select this feature for users

Copy link
Member

Choose a reason for hiding this comment

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

it's possible that the implementations of Resource and ResourceExt needs to be in kube to avoid having this feature issue here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, that's unfortunate because that's the type that is causing issues for me.

This only caused problems for running the tests, so maybe we can just specify the feature in CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made this change if that works for you.

Copy link
Member

@clux clux May 17, 2021

Choose a reason for hiding this comment

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

Ah, yeah, that works. The standard pattern we have is to select the feature* internally:

[dependencies.k8s-openapi]
version = "0.11.0"
default-features = false
features = []
[dev-dependencies.k8s-openapi]
version = "0.11.0"
default-features = false
features = ["v1_20"]

@clux
Copy link
Member

clux commented May 17, 2021

I'll merge this and play around a bit with it in master. Don't see anything obvious here, and CI is passing. Maybe this is just a quick release :-)

@clux clux merged commit ef3664f into kube-rs:master May 17, 2021
@kflansburg kflansburg deleted the kflansburg/kube-core branch May 17, 2021 20:19
@clux clux mentioned this pull request May 17, 2021
@clux
Copy link
Member

clux commented May 17, 2021

Ended up moving a bunch more core types in there into kube-core in #519 without adding any deps.

@clux
Copy link
Member

clux commented May 18, 2021

Released with modifications in 0.54.0 (#522 additional imports). Thanks again!

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.

Eliminate kube-derive Dependency on kube
2 participants