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

ci: disable dependabot cargo updates #120

Merged
merged 1 commit into from
Aug 26, 2024
Merged

Conversation

cpu
Copy link
Member

@cpu cpu commented Aug 5, 2024

Until we have a better sense of how to handle the update PRs it produces let's turn this off for now. I think I was premature in enabling it. Inspired by discussion in this PR thread.

@cpu cpu self-assigned this Aug 5, 2024
@complexspaces
Copy link
Collaborator

Until we have a better sense of how to handle the update PRs it produces let's turn this off for now.

Did you want to discuss that in this PR or would it be better to open a followup issue where the topic can happen? I don't mind either way.

@cpu
Copy link
Member Author

cpu commented Aug 5, 2024

Did you want to discuss that in this PR

WFM!

I think there's two topics:

  1. whether it's worth maintaining Cargo.lock updates for compatible updates. We do it in the other Rustls repos, but I don't think I can articulate a very meaningful argument for why we should do it here.
  2. how they should be reviewed (edit: and how many reviews should there be before merge). In the other Rustls repos we don't (at least as I understand things) do more than a cursory review of the update. When we were using Dependabot I would review the updates in https://diff.rs and pay particular attention to things like build.rs updates, but for large deps like aws-lc-rs/aws-lc-sys I certainly don't review every changed line. I also haven't been keeping up with that practice since switching to Renovate. How are other folks handling these?

@djc
Copy link
Member

djc commented Aug 5, 2024

I generally scroll through the Cargo.lock changes looking for any new dependencies, but I don't look at the actual changes made in semver-compatible upstream versions.

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

Should we also disable this for github-actions?

@djc
Copy link
Member

djc commented Aug 5, 2024

A potential third topic for discussion: being more explicit about ownership. So far I think we've mostly deferred to @complexspaces on any half-way nuanced changes, but the downside is that @complexspaces has pretty limited bandwidth certainly compared to the other rustls org maintainers. Maybe we can get to a tweaked proposed governance model?

@cpu
Copy link
Member Author

cpu commented Aug 5, 2024

Should we also disable this for github-actions?

These seem infrequent enough that I was OK leaving it on, but happy to revisit.

@worldwise001
Copy link

worldwise001 commented Aug 11, 2024

Hey folks, @complexspaces directly reports to me currently (I'm director of seceng at 1Password); I don't feel particularly strongly one way or another as to what the governance model of this library/project should look like, but I am open to discussion as to how/where we can adjust or increase 1Password involvement/support. :)

Let's bring this discussion into another issue?

@djc
Copy link
Member

djc commented Aug 19, 2024

Hey folks, @complexspaces directly reports to me currently (I'm director of seceng at 1Password); I don't feel particularly strongly one way or another as to what the governance model of this library/project should look like, but I am open to discussion as to how/where we can adjust or increase 1Password involvement/support. :)

Let's bring this discussion into another issue?

Opened #125 for this.

Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

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

Should we also disable this for github-actions?

These seem infrequent enough that I was OK leaving it on, but happy to revisit.

I'm also in favor of leaving GHA updates enabled since they are less frequent and less work to review.

I generally scroll through the Cargo.lock changes looking for any new dependencies, but I don't look at the actual changes made in semver-compatible upstream versions.

This is what I do as well.

Overall, I'm in favor of what is currently being proposed in this PR's branch contents as it creates less noise and review burden over very small changesets.

Until we have a better sense of how to handle the update PRs it produces
let's turn this off for now. I think I was premature in enabling it.
@cpu cpu force-pushed the cpu-dependabot-less branch from 1855a77 to ce4abed Compare August 26, 2024 13:22
@cpu cpu merged commit b039717 into rustls:main Aug 26, 2024
18 checks passed
@cpu cpu deleted the cpu-dependabot-less branch August 26, 2024 13:31
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.

4 participants