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

Setup Github Action to auto-approve/merge dependabot PRs. #1964

Merged
merged 3 commits into from
Sep 20, 2023

Conversation

branlwyd
Copy link
Contributor

No description provided.

@branlwyd branlwyd marked this pull request as ready for review September 19, 2023 22:47
@branlwyd branlwyd requested a review from a team as a code owner September 19, 2023 22:47
@branlwyd branlwyd force-pushed the bran/automerge-dependabot branch from 4b7d242 to f42f98b Compare September 19, 2023 23:36
We weren't actually using the output of this step; this was a
cargo-culting error.
Copy link
Contributor

@inahga inahga left a comment

Choose a reason for hiding this comment

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

I admit I've been ignoring the idea of automerging dependabot because I'm somewhat uncomfortable with the idea. It means that our vendors can more or less push code to our repository without any developer vetting them.

Let me think about it out loud: the threat model in my head is that a vendor releases malicious code.

I suppose that threat is better mitigated with the likes of cargo audit and cargo vet. We weren't really doing code review anyway and IMO it's unreasonable for us to. We also have lots of time for the community to discover such a thing before such code is actually released to production, assuming we deploy on wednesday or thursday. (note that malicious code can still exfiltrate CI secrets--but we already accept that risk since dependabot PRs run builds automatically).

My threat model also includes a vendor releasing buggy code. I do read the change notes and like to do code review for features that I know impact the code base. I think @jbr's idea of a changelog rollup is nice, because it lets the developer review the changes at release time, but I don't think we have to block this PR for that (I wish dependabot actually copied the release notes into the commit message, that'd pretty much solve it for me).

An entirely different approach we could take is just to freeze dependencies except for security updates and bug fixes that directly affect us. We would probably also want major version upgrades, to avoid being too far behind.

Overall I'm fine with this approach, just making some notes.

.github/workflows/dependabot-automerge.yml Outdated Show resolved Hide resolved
.github/workflows/dependabot-automerge.yml Show resolved Hide resolved
@branlwyd
Copy link
Contributor Author

Your thinking on the threat model is pretty much aligned with mine: since we are not actually reviewing the changes for safety/correctness (nor would it be feasible for us to do so), or doing any other vetting beyond our CI, the toil of manually approving each PR is not useful.

I think we can review changelogs just by looking to the merged PRs in our notifications (or in the git commit log, etc). I'll leave potentially switching tools to later work.

@branlwyd branlwyd merged commit fd0f77b into main Sep 20, 2023
8 checks passed
@branlwyd branlwyd deleted the bran/automerge-dependabot branch September 20, 2023 18:03
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.

2 participants