Do not auto-approve dependabot PRs, and add cooldown to dependabot - #1564
Do not auto-approve dependabot PRs, and add cooldown to dependabot#1564ludfjig wants to merge 1 commit into
Conversation
Signed-off-by: Ludvig Liljenberg <4257730+ludfjig@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR removes the repository automation that auto-approved/auto-merged Dependabot PRs (to require human review) and adds a Dependabot “cooldown” period to reduce update churn while still allowing Dependabot to operate normally.
Changes:
- Deleted the scheduled GitHub Actions workflow that auto-approved/auto-merged Dependabot PRs.
- Removed the
dev/auto-approve-dependabot.shscript used by that workflow. - Added a 7-day Dependabot cooldown across configured ecosystems/directories.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
dev/auto-approve-dependabot.sh |
Removed the Dependabot auto-approval/auto-merge script to require human review. |
.github/workflows/auto-merge-dependabot.yml |
Removed the scheduled workflow that used the script to auto-approve/auto-merge Dependabot PRs. |
.github/workflows/dep_update_guest_locks.yml |
Removed a comment referencing the deleted auto-merge workflow (no functional change). |
.github/dependabot.yml |
Added cooldown: default-days: 7 to reduce update noise and avoid immediate adoption of freshly-released versions. |
jsturtevant
left a comment
There was a problem hiding this comment.
It seems this is a wise choice in the current environment, but it will make a little bit more work for maintainers. I would leave it for others to look at too and give some comments or approve
|
I am in 2 minds about this, generally agree with the cooldown changes (although we should probably think about if we want different settings for major/minor/patch). I don't know about having manual review, besides the overhead, what is the expectation on a reviewer? How likely is it that one of us will find a supply chain attack in a dependency that has presumably evaded the maintainers of the dependency? If a dependency has been compromised then I guess it will be yanked (and dependabot will behave appropriately? |
|
I think this is generally a good direction to move in, although a bit of extra work. If we do want to actually vet crates and keep track of changes in case of malicious maintainers or attacks on maintainers, we might want to use something like Mozilla's cargo vet as well. If the idea is partly to avoid attacks on our own infrastructure, rather than on downstream users of hyperlight (since they control their own Cargo.locks anyway, and we can't do anything about semver-compatible minor version bumps of our dependencies auto-updating there), do we also need to avoid running CI on PRs (both dependant and manual) that include dependency or lock file upgrades, at least if they have not already appeared in trusted cargo-vet feeds or something like that? |
Automatically merging PRs is not a good idea security-wise, especially given the many supply chain attacks these days. Having a human review the diff of an updated crate is always good to make sure it's not been compromised. This PR removes auto-approving PRs by our bot and also adds a cooldown to dependabot which is good practice in general.
I also considered just adding a dependabot cooldown, and also only auto-merging patch/minor bumps, but ultimately settle on this which I think is the better approach.
Note: The bot is still used in
dep_update_guest_locks.yml