-
Notifications
You must be signed in to change notification settings - Fork 361
Update all dependencies and fix examples #959
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
Conversation
Hi @s0l0ist , thanks for your work on this! It makes sense to me to bump the crates that have newer major versions (if >=1.x) or minor versions (0.x) since that will help simplify consumer dependency graphs and let us benefit from new features. I see you did some legwork to migrate some of the apis, such as for the aws sdk dependencies, which is awesome. Likewise the cargo machete usage is great, pleased to see us rip out some larger dependencies like tokio from some of the workspace crates. However I'm not sold on the bumps to pre-1.0 patch versions, and post-1.0 minor versions. Since, anyway consumers are resolving to the latest version that their closure allows, but moving forward the floor might create headaches for consumers that need to pin back to an older dependency version for some reason (as then they need to pin us as well). Would it be possible to leave those types of version bumps out unless there is a new API or feature we specifically want in the newest semver compatible version? Glad to discuss it further if there is an angle I'm missing here. The main downside I'm aware of is accidentally using features that are dependent on newer nonbreaking versions - but we could add ci validations with |
Hi @jlizen, thanks for your feedback! I agree with you on not bumping the floor unnecessarily. I've reverted the version changes to workspace dependencies where applicable. There's now only the two dev-deps that have been bumped: However, I chose not to revert the changes in the examples directory. There doesn't appear to be a consistent convention for how dependencies are specified there - I found a mix of Since these examples are meant to be illustrative (not consumed directly as libraries), I believe it's reasonable for them to track the latest compatible versions. This makes them easier to maintain and helps users see working, up-to-date code. In most cases, users would copy the examples and manage their own dependencies independently of the workspace anyway. Let me know if you'd prefer a different approach, but my recommendation is to keep the version bumps in the examples for now. (I also ran |
Thanks!
Seems fine,
I agree with this choice, good call. I'll give the changes a deeper read through today to get you a full review! |
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.
Looks great! Thanks again for the cleanup.
@@ -20,7 +20,6 @@ serde = { version = "1.0.204", features = ["derive"] } | |||
|
|||
[dev-dependencies] | |||
reqwest = { version = "0.12.5", features = ["blocking"] } | |||
openssl = { version = "0.10", features = ["vendored"] } |
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.
Ooh, this was a good machete ❤️
@@ -33,8 +33,6 @@ hyper-util = { workspace = true, features = [ | |||
"tokio", | |||
] } | |||
tower = { workspace = true, features = ["util"] } | |||
tower-service = { workspace = true } |
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.
Two more great ones
http-body-util = { workspace = true } | ||
http-serde = { workspace = true } | ||
hyper = { workspace = true, features = ["http1", "client"] } | ||
hyper-util = { workspace = true, features = [ |
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.
Wow, this too. I guess anyway most consumers would be pulling them in via sibling dependencies, but still.
📬 Issue #, if available:
✍️ Description of changes:
cargo upgrade
andcargo update
cargo machete --with-metadata
cargo +nightly fmt
cargo clippy --fix
All tests/examples compile and pass. I have not tested e2e integration and was hoping CI would cover that path.
🔏 By submitting this pull request
cargo +nightly fmt
.cargo clippy --fix
.