Commit the Cargo.lock file

[rukai]

shotover is an application not a library so we should check in the Cargo.lock as per https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries

[rukai]

Lets also pin the version of clap we are using as its not following semver:

--- a/shotover-proxy/Cargo.toml
+++ b/shotover-proxy/Cargo.toml
@@ -16,7 +16,7 @@ futures-core = "0.3.1"
 chrono = {version = "0.4.10",  features = ["serde"]}
 async-trait = "0.1.30"
 byteorder = "1.3.2"
-clap = "3.0.0-beta.1"
+clap = "=3.0.0-beta.1"
 derivative = "2.1.1"
 #evmap = {version = "11.0.0-alpha.7"}
 itertools = "0.9.0"

Not strictly needed once we have a cargo.lock file checked in. But still nice to have cargo update not break our build.

[Claudenw]

I think that the Cargo.lock should be committed by the build process on the tag/branch that is the release. During development I think it should float, except in notable conditions like @rukai noted above. The idea is that we pick up changes as we go though the development process so that we stay at the front of the wave. Once the code is "frozen" we add the Cargo.lock and do the release process on a separate branch. The mainline code development continues on the main branch.

I have a couple of questions that will open a can of worms:

1. How many versions of the product will we maintain? Apache Jena. takes the approach that we use "x.y.z" version numbers and we release "x.y.0" as a release, at which point "x.y+1.0" is the new main branch version. We rarely release x.y.1 (critical bug fixes only). But then Jena is a library (mostly).
2. How long will Instaclustr support a given version? The last place I worked that was 5 years.
3. Will we build custom versions for customers? I recommend "no" be the answer here.
4. Will we patch older versions. I suggest again the answer is "no". If we are going to do anything like this then we should maintain the "x.y.1", "x.y.2", "x.y.3", structure and tell customers that if they want the fix in "x.y.3" they have to accept "x.y.2" as well. We also should then look at merging those changes into "x.y+.n" versions. Again, these are only bug fixes, not enhancements. (see semantic versioning for clarification)

[rukai]

Committing the Cargo.lock is the standard approach for rust binaries, I dont see much value in deviating from that. By checking in the Cargo.lock we eliminate any "works on my machine" issues caused by version mismatch between developers.

A developer can at any time submit a cargo update PR.

If we do end up with multiple maintained branches we can run a cargo update just before splitting off that branch. Doing that will also give us a useful diff of the changes in the cargo.lock that occurred before the release.

[benbromhead]

Coming back around to this.

• Having Cargo.lock part of the commit 100% makes sense as it is good for reproduceability. I've been bitten by this as before we started caching deps for the CI system it would just whatever was latest and not every package maintainer is good at not breaking APIs on minor versions. (e.g. why does it work on my machine and not CI).
• To @Claudenw point, keeping our deps up to date is also super important (Apache C* is notorious for old old deps as it vendors the binaries in tree... yuck). There might even be a GH action that does this as part of each PR (make it part of the build infra as @Claudenw suggested).

If there is no easy way to include this, two options would be:

• We could probably have a cargo update check happen on a weekly / daily / whatever basis where no PR is raised if nothing change, automerge if all tests pass? (e.g. write our own GH action / PR bot).
• Or for the time being though, just running a cargo update before you submit a PR and then including the updated lock file is the way to go?

[benbromhead]

cargo update --locked might very well be the check we need and can simply add it as a task to our GH Actions folder. It wont fail on explicity locked versions (e.g. clap in this case). However for a normal dep it will throw an error if we can update to the latest minor.

This way the PR reviewer can see if deps are out of date and request they get updated as part of a PR. If we need to lock to an older version for a good reason (e.g. bug in a new one), we can pin a version in the cargo file explitly. We'll just need to keep on top of pinned versions if/when they happen

[rukai]

If we value keeping our dependencies up to date then we also need to ensure that we upgrading to the latest major releases (and minor releases of 0.x.x crates). This requires human intervention to handle breaking changes. Once we go open source we can stick a badge for https://deps.rs/repo/github/shotover/shotover-proxy in our readme (example page for another project https://deps.rs/repo/github/bevyengine/bevy) deps.rs will show us outdated major releases (and minor releases of 0.x.x crates) Then when someone notices an outdated dependency they can submit a PR for it. I tend to keep an eye on such things and the process usually requires manual intervention so I dont see much value in automating.

We can then bundle the cargo update into these manual dependency update PRs.

The bots that I have seen for automatically updating rust dependencies create an absurd amount of PR noise.

[rukai]

cargo update --locked might very well be the check we need and can simply add it as a task to our GH Actions folder. It wont fail on explicity locked versions (e.g. clap in this case). However for a normal dep it will throw an error if we can update to the latest minor.

This way the PR reviewer can see if deps are out of date and request they get updated as part of a PR. If we need to lock to an older version for a good reason (e.g. bug in a new one), we can pin a version in the cargo file explitly. We'll just need to keep on top of pinned versions if/when they happen

I'm opposed to this, dependency changes should not go in unrelated PRs:

• it adds noise to unrelated PRs
• confusing workflow for new contributors (once open sourced)
• easy for a bad update to sneak in and break something and then harder to track back to the PR/commit that caused it.

[benbromhead]

Good points - The deps.rs badge is a good approach in that we have a clear signal when things are out of date (plus dep security warnings as well), but decouples it from unrelated PRs