-
Notifications
You must be signed in to change notification settings - Fork 424
Group non-breaking dependabot PRs together to reduce review load #18402
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sandhose questioned in an internal meeting whether this would be compatible with what we do for towncrier changelog entries for dependabot PRs in the release script:
synapse/scripts-dev/release.py
Lines 929 to 961 in 740fc88
| def build_dependabot_changelog(repo: Repo, current_version: version.Version) -> str: | |
| """Summarise dependabot commits between `current_version` and `release_branch`. | |
| Returns an empty string if there have been no such commits; otherwise outputs a | |
| third-level markdown header followed by an unordered list.""" | |
| last_release_commit = repo.tag("v" + str(current_version)).commit | |
| rev_spec = f"{last_release_commit.hexsha}.." | |
| commits = list(git.objects.Commit.iter_items(repo, rev_spec)) | |
| messages = [] | |
| for commit in reversed(commits): | |
| if commit.author.name == "dependabot[bot]": | |
| message: Union[str, bytes] = commit.message | |
| if isinstance(message, bytes): | |
| message = message.decode("utf-8") | |
| messages.append(message.split("\n", maxsplit=1)[0]) | |
| if not messages: | |
| print(f"No dependabot commits in range {rev_spec}", file=sys.stderr) | |
| return "" | |
| messages.sort() | |
| def replacer(match: Match[str]) -> str: | |
| desc = match.group(1) | |
| number = match.group(2) | |
| return f"* {desc}. ([\\#{number}](https://github.com/element-hq/synapse/issues/{number}))" | |
| for i, message in enumerate(messages): | |
| messages[i] = re.sub(r"(.*) \(#(\d+)\)$", replacer, message) | |
| messages.insert(0, "### Updates to locked dependencies\n") | |
| # Add an extra blank line to the bottom of the section | |
| messages.append("") | |
| return "\n".join(messages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From checking internal repos that have grouping of dependabot PRs enabled, dependabot will create one commit with the title "Bump the patches group with 5 updates" and then a markdown table with the old/new versions:
| Package | From | To |
| --- | --- | --- |
| [tower-http](https://github.com/tower-rs/tower-http) | `0.6.4` | `0.6.5` |
| [hyper-util](https://github.com/hyperium/hyper-util) | `0.1.12` | `0.1.13` |
| [ruma](https://github.com/ruma/ruma) | `0.12.2` | `0.12.3` |
| [clap](https://github.com/clap-rs/clap) | `4.5.38` | `4.5.39` |
| [ruma-events](https://github.com/ruma/ruma) | `0.30.2` | `0.30.3` |as well as a yaml-based list:
updated-dependencies:
- dependency-name: tower-http
dependency-version: 0.6.5
dependency-type: direct:production
update-type: version-update:semver-patch
dependency-group: patches
- dependency-name: hyper-util
dependency-version: 0.1.13
dependency-type: direct:production
update-type: version-update:semver-patch
dependency-group: patches
- dependency-name: ruma
dependency-version: 0.12.3
dependency-type: direct:production
update-type: version-update:semver-patch
dependency-group: patches
- dependency-name: clap
dependency-version: 4.5.39
dependency-type: direct:production
update-type: version-update:semver-patch
dependency-group: patches
- dependency-name: ruma-events
dependency-version: 0.30.3
dependency-type: direct:production
update-type: version-update:semver-patch
dependency-group: patchesthe release script could parse one of these so that we still have a single line per update in the changelog. But I'm not sure how much folks actually care. I suppose this is one to ask the Synapse package maintainers matrix room...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Synapse package maintainers said they don't use this information, so let's drop it.
Should be unblocked by #19254, which removes said section from the release script/changelog.
When queried, the Synapse Package Maintainer group did not find this list useful. The dev team do not use it either, nor found anyone else that used it. This PR unblocks #18402, which itself will increase overall bandwidth for PR review from the team.
When queried, the Synapse Package Maintainer group did not find this list useful. The dev team do not use it either, nor found anyone else that used it. This PR unblocks #18402, which itself will increase overall bandwidth for PR review from the team.
…endabot_group_update_prs
devonh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on.
The grouping choices seem sane to me. I think it will be best just to try this out for a while and see how things go.
It looks easy to tweak these setting in the future if we find that a different grouping setup would be better.
|
If it's any reassurance, we currently make use of this on the (internal) ti-messenger-proxy repo. You can see an example of what a grouped PR looks like here: https://github.com/element-hq/ti-messenger-proxy/pull/769 |
While it's great that we have dependabot updating our PRs, the sheer amount of PRs it produces has a few side effects:
Thus it seems necessary to try and group the PRs that don't need very careful review together into a single PR. See the comments in the patch for the chosen rules and the justification.
Further reading: https://docs.github.com/en/code-security/dependabot/dependabot-version-updates/optimizing-pr-creation-version-updates#prioritizing-meaningful-updates
Originally suggested by the Server Products Team, and used successfully in the SBG/TI-Messenger repos.
Pull Request Checklist
EventStoretoEventWorkerStore.".code blocks.(run the linters)