Minor: Document the rationale for the lack of Cargo.lock #14071
Conversation
| @@ -146,3 +146,27 @@ stable API, we also improve the API over time. As a result, we typically | |||
| deprecate methods before removing them, according to the [deprecation guidelines]. | |||
|
|
|||
| [deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html | |||
|
|
|||
| ## Dependencies and a `Cargo.lock` | |||
There was a problem hiding this comment.
@mbrobbel has a good point here #14069 (comment)
I think one concern originally was how we would keep a Cargo.lock file up to date with the latest version of the dependencies
I can't remember if depndabot was available at that point -- maybe dependabot is good enough that we could check in Cargo.lock and have Dependabot update it
There was a problem hiding this comment.
From my perspective it is very important that CI covers testing with the latest versions of all dependencies (as that is what many/most downstream crates will use as well)
There was a problem hiding this comment.
I'd like to point to https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html for considerations and suggestions.
There was a problem hiding this comment.
TLDR it sounds like the rust team now suggests always committing Cargo.lock and letting dependabot handle updates. That seems like a good idea to me
There was a problem hiding this comment.
Just my two cents, but I have found Renovate to be much more configurable. Here's an example of a lock file maintenance PR: spiraldb/vortex#1818
There was a problem hiding this comment.
One thing we have to be aware of in DataFusion is that as part of the Apache security posture, only certain third party actions are allowed -- we would have to double check Rennovate
I think the next step is probably to file an issue to explicitly discuss checking in a Cargo.lock file. I'll try and find time over the next few days if no one beats me to it
There was a problem hiding this comment.
I filed a ticket to discuss next steps:
Which issue does this PR close?
The question of why
Cargo.lockis not checked in comes up from time to time, most recently inctorto0.2.9#14069 (review)from @mbrobbel
Rationale for this change
I think documenting the rationale as I understand it will help:
What changes are included in this PR?
Document my recollection of why Cargo.lock is not checked in
Are these changes tested?
By CI
Are there any user-facing changes?
Update the readme