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

Add a lockfile #1337

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add a lockfile #1337

wants to merge 2 commits into from

Conversation

clux
Copy link
Member

@clux clux commented Nov 8, 2023

From a blank build today. I personally think this makes sense.

Reasons;

  • basically https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html
  • frequently we get cargo deny issues pulled in from under us which we have to explain away
  • contributor confusion; why does this not build? we don't want them to be the first to notice a breaking build, that's CIs job
  • we can setup dependabot to auto-merge non-breaking changes that pass CI

Downsides:

  • we still have to manually fix up most cargo deny issues
  • we will get a lot more dependency prs, but if most are auto-merged then it should be ok? they don't show up in the github releases because of our changelog configuration

AFAIU non-breaking builds do not need to get pins updated from Cargo.toml so this should mostly be a sanity thing for CI and contributors (rather than forcing everyone to bump the dependencies of us).

Have setup automatic dependency merging in all other kube repos, but those are binaries so it's less scary / controversial. My experience with these have been very positive however, so feel we should probably do this here also.

From a blank build today.

Reasons;

- basically https://blog.rust-lang.org/2023/08/29/committing-lockfiles.html
- frequently we get cargo deny issues pulled in from under us which we have to explain away
- contributor confusion; why does this not build? we don't want them to be the first to notice a breaking build, that's CIs job
- we can setup dependabot to auto-merge non-breaking changes that pass CI

Downsides:

- we still have to manually fix up most cargo deny issues
- we might get a lot more PRs, but if most are auto-merged that's ok

AFAIU non-breaking builds do not need to get pins updated from `Cargo.toml` so this should mostly be a sanity thing for CI and contributors (rather than forcing everyone to bump the dependencies of us).

Signed-off-by: clux <[email protected]>
@clux clux added the changelog-exclude changelog excluded prs label Nov 8, 2023
@clux clux added this to the 0.88.0 milestone Nov 8, 2023
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

Merging #1337 (01dec7f) into main (5813ad0) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1337   +/-   ##
=====================================
  Coverage   72.1%   72.1%           
=====================================
  Files         75      75           
  Lines       6377    6377           
=====================================
  Hits        4597    4597           
  Misses      1780    1780           

@Dav1dde
Copy link
Member

Dav1dde commented Nov 8, 2023

Re: dependabot, I feel like there should be just a weekly lockfile maintenance which bumps the lockfile to the max dependency versions and not for each dependency individually

@clux
Copy link
Member Author

clux commented Nov 8, 2023

We can do that I believe; weekly interval on dependabot with a grouping on "*"

@clux clux removed this from the 0.88.0 milestone Nov 8, 2023
@clux clux added the question Direction unclear; possibly a bug, possibly could be improved. label Nov 8, 2023
@nightkr
Copy link
Member

nightkr commented Dec 1, 2023

Very 👍 on this from me.

@clux
Copy link
Member Author

clux commented Dec 4, 2023

I think this is nice personally also, but we could also get a decent approximation of safety with a daily build.

so a couple of things that would be good to get feelings on;

  • lockfile safety vs. cron'd check of latest (cargo update + build)
  • bump frequency / dependabot grouping setup / automerging (if using lockfile)
  • MSRV implications (if we tail latest with a lockfile, then maybe we change msrv test to use -Zminimal-versions builds? - maybe we should do that anyway)

Feel free to leave comments here, but have also added it as an agenda item for tomorrow's meeting :-)

@nightkr
Copy link
Member

nightkr commented Dec 4, 2023

Why not both?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-exclude changelog excluded prs question Direction unclear; possibly a bug, possibly could be improved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants