Skip to content

Conditionally auto-approve dependabot PRs#1117

Merged
thomasiles merged 13 commits intomainfrom
auto-merge-patches
Jul 28, 2025
Merged

Conditionally auto-approve dependabot PRs#1117
thomasiles merged 13 commits intomainfrom
auto-merge-patches

Conversation

@cadmiumcat
Copy link
Copy Markdown
Contributor

@cadmiumcat cadmiumcat commented Dec 31, 2024

What problem does this pull request solve?

We want to set up a flow to to auto-approve some dependabot PRs. PRs will only
be approved if:

  • they are not npm updates, and
  • the update is only a version patch

Trello card

Things to consider when reviewing

It would be great to sense check the workflow against what we're trying to achieve.

The workflow is configured to approve and merge a PR only if:

  • The PR was raised by Dependabot
  • The PR is a patch version and not part of an NPM package
  • All checks have passed

Otherwise, those steps will be skipped and the PR goes through the regular review process. You can see this in action in this PR, where all there a bunch of skipped checks relating to the auto-approving and auto-merging.

General things to consider

  • Is there enough debug logging in the workflow?
  • Are there any name changes you'd suggest?
  • Can the workflow be optimised in any way?
  • Ensure you consider the wider context.
  • Is it clear what the code is doing?
  • Do the commit messages explain why the changes were made?
  • Has all relevant documentation been updated?

Local testing

The only way to test if this really works will be to merge it into the main branch unfortunately. Below are the two testing strategies Cat and I took.

Cat

I wanted to try this workflow locally using act but it keep getting an error at the step where we ask it to get the dependabot metadata.

[Dependabot auto-approve/dependabot]   💬  ::debug::Verifying the job is for an authentic Dependabot Pull Request
[Dependabot auto-approve/dependabot]   ❗  ::error::Api Error: (404) Not Found
[Dependabot auto-approve/dependabot]   ❌  Failure - Main dependabot/fetch-metadata@v2
[Dependabot auto-approve/dependabot] exitcode '1': failure
[Dependabot auto-approve/dependabot] 🏁  Job failed
<!-- If this section isn't relevant for your PR feel free to edit or remove it -->

Sarah

I ended up forking the forms-runner repo so I could test changes on a safe main branch (see fork here: https://github.com/sarahseewhy/forms-runner). I created a new file review_apps_on_pr_change.yml in the forked repo and copied the contents from this branch.

This was a useful approach because it allowed me to close and reopen Dependabot PRs to test the workflow and learn quite a bit.

Here's an example of the Actions at work on the forked repo: https://github.com/sarahseewhy/forms-runner/actions/runs/15110638823/job/42469149737?pr=39

However, forked repos don't have the Github permission to actually auto-merge so I'm returning to this branch and pull request.

You can take a similar approach by:

  1. Forking the repo
  2. Creating a new file review_apps_on_pr_change.yml
  3. Copy and pasting the contents of review_apps_on_pr_change.yml from this branch into the forked repo
  4. Configure the fork to have the same Dependabot and branch settings as this repo
  5. Find a Dependabot PR which matches our criteria
  6. Close the dependabot PR and then reopen it by typing @dependabot reopen in the comment section

@cadmiumcat cadmiumcat marked this pull request as ready for review December 31, 2024 13:38
@cadmiumcat cadmiumcat changed the title TMP: conditionally auto-approve dependabot PRs Prepare to conditionally auto-approve dependabot PRs Dec 31, 2024
@cadmiumcat cadmiumcat marked this pull request as draft December 31, 2024 14:31
@cadmiumcat cadmiumcat force-pushed the auto-merge-patches branch 2 times, most recently from 31a8097 to 52296b9 Compare December 31, 2024 14:58
@cadmiumcat cadmiumcat changed the title Prepare to conditionally auto-approve dependabot PRs Conditionally auto-approve dependabot PRs Dec 31, 2024
@cadmiumcat cadmiumcat marked this pull request as ready for review December 31, 2024 15:06
@cadmiumcat cadmiumcat marked this pull request as draft December 31, 2024 15:42
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Jan 9, 2025

@sarahseewhy sarahseewhy force-pushed the auto-merge-patches branch 3 times, most recently from 8ac6588 to eb2be71 Compare May 20, 2025 10:49
@sarahseewhy sarahseewhy marked this pull request as ready for review May 20, 2025 10:56
@sarahseewhy sarahseewhy force-pushed the auto-merge-patches branch 5 times, most recently from d7b8460 to 868463d Compare May 20, 2025 12:43
chao-xian
chao-xian previously approved these changes May 20, 2025
Copy link
Copy Markdown
Contributor

@chao-xian chao-xian left a comment

Choose a reason for hiding this comment

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

🚀

@sarahseewhy
Copy link
Copy Markdown
Contributor

I'm going to merge this pull request in an hour (after a meeting). Then either update a relevant Dependabot PR or close/reopen a Dependabot PR (which ever works to test). I'll also check the other Dependabot PRs, especially ones which we don't want merged (like npm or minor bumps).

If anything goes wrong I'll revert the merge and we can figure out next steps.

@lfdebrux
Copy link
Copy Markdown
Contributor

lfdebrux commented Jun 7, 2025

Another thought (sorry)... We should probably add branch protections to all dependabot/** branches preventing pushes... Otherwise I think an attacker could add malicious commits to the PR and get them auto-approved and merged (if they are quick).

I got the idea from https://boostsecurity.io/blog/weaponizing-dependabot-pwn-request-at-its-finest#:~:text=To%20prevent%20branch%20protection%20bypasses.

@lfdebrux lfdebrux closed this Jun 7, 2025
@lfdebrux lfdebrux reopened this Jun 7, 2025
@cadmiumcat
Copy link
Copy Markdown
Contributor Author

Another thought (sorry)... We should probably add branch protections to all dependabot/** branches preventing pushes... Otherwise I think an attacker could add malicious commits to the PR and get them auto-approved and merged (if they are quick).

I got the idea from https://boostsecurity.io/blog/weaponizing-dependabot-pwn-request-at-its-finest#:~:text=To%20prevent%20branch%20protection%20bypasses.

It's a good point. My concern with branch protections is that we'd then have to open a PR to commit changes to dependabot PRs, which is not a great flow for us.
I was wondering if we could check the contributors to the PR: if any of them is not dependabot, then we don't autoapprove/merge. But I can only find references to the author of the PR

@cadmiumcat cadmiumcat force-pushed the auto-merge-patches branch from ec5ae32 to 4d534c8 Compare June 26, 2025 07:56
@cadmiumcat cadmiumcat dismissed AP-Hunt’s stale review June 26, 2025 08:01

AP-Hunt is gone :(

@cadmiumcat cadmiumcat requested a review from lfdebrux June 26, 2025 08:27
@cadmiumcat cadmiumcat force-pushed the auto-merge-patches branch from 5d98805 to 94b46b4 Compare June 30, 2025 06:36
cadmiumcat and others added 13 commits July 28, 2025 10:55
We want to set up a flow to to auto-approve some dependabot PRs. PRs will only
be approved if:
- they are not `npm` updates, and
- the update is only a version patch
* The `auto_approve_and_merge` if-conditional was skipping because there was no value in `outputs.is_allowed_dependency`
* Adding an outputs field on the `validate_this_is_an_allowed_dependency` job solved this issue
* This section is almost impossible test on a branch or even a fork, it must be tested by merging into the main branch
* Uncommenting the section in order raise a PR and pair with someone on merging it
* Add step with a script which iterates through all the checks for a commit and waits until there are no pending and no failing checks
* I tried waiting for a success but this didn't work because it's possible to have successful checks and pending/failing checks simultaneously, it was more reliable to wait for the absence of pending/failing checks then the presence of successful checks
* Exclude the `wait_for_checks` check or else it loops until the max retries are reached (I learned this the hard way)
* Add `wait_for_checks` to the auto_approve_and_merge step's need attribute so it will only run once the `wait_for_checks` has succeeded
* Add curly braces to the PR_URL variable since this is more conventional and safer bash
* Add line separation to the if/else block to make the test command and statements more distinct
* Add linting workflow which runs `actionlint` on all the repo's workflows (see documentation: https://github.com/rhysd/actionlint).
* I've introduced a fair amount of bash in the dependabot-auto-approve.yml file and it behooves us to ensure the bash is linted (in addition to the workflow logic)
Only the job that merges the PR needs write access.
And we want to only auto-merge the dependencies we agree to automerge
We unintentionally broke this flow by agreeing to actionlint's suggestions
Instead of using actionlint's suggested script for installation
(https://github.com/rhysd/actionlint/blob/main/scripts/download-actionlint.bash),
dowload directly from releases and execute ourselves
We unintentionally broke this flow by agreeing to actionlint's suggestions.
Since it's mostly a stylistic issue, we can ignore it
@thomasiles thomasiles force-pushed the auto-merge-patches branch from 94b46b4 to 9d1d759 Compare July 28, 2025 09:55
@sonarqubecloud
Copy link
Copy Markdown

@github-actions
Copy link
Copy Markdown
Contributor

🎉 A review copy of this PR has been deployed! It is made of up two components

  1. A review copy of forms-runner
  2. A production copy of forms-admin

Important

Not all of the functionality of forms-runner is present in review apps.
Functionality such as sending emails, file upload, and S3 submission types are
deliberately disabled for the sake of simplifying review apps.

You should use the full dev environment to test the functionality which is disabled here.

It may take 5 minutes or so for the application to be fully deployed and working. If it still isn't ready
after 5 minutes, there may be something wrong with the ECS task. You will need to go to the integration AWS account
to debug, or otherwise ask an infrastructure person.

For the sign in details and more information, see the review apps wiki page.

@thomasiles thomasiles merged commit 52892c4 into main Jul 28, 2025
11 checks passed
@thomasiles thomasiles deleted the auto-merge-patches branch July 28, 2025 10: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.

6 participants