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

Combined dependencies PR #4135

Merged
merged 6 commits into from
Jun 7, 2021
Merged

Combined dependencies PR #4135

merged 6 commits into from
Jun 7, 2021

Conversation

rnorth
Copy link
Member

@rnorth rnorth commented May 29, 2021

Combining multiple dependencies PRs into one:

dependabot bot and others added 6 commits May 19, 2021 12:46
Bumps tomcat-jdbc from 10.0.0 to 10.0.6.

Signed-off-by: dependabot[bot] <[email protected]>
…jupiter/org.junit.jupiter-junit-jupiter-engine-5.7.2' into combined-pr-branch
…rg.apache.tomcat-tomcat-jdbc-10.0.6' into combined-pr-branch
…se-commons/org.assertj-assertj-core-3.19.0' into combined-pr-branch
@rnorth rnorth added the dependencies Pull requests that update a dependency file label May 29, 2021
@rnorth rnorth marked this pull request as draft May 29, 2021 09:54
@rnorth
Copy link
Member Author

rnorth commented May 29, 2021

This PR was created using a simple bash script that iterates over open dependencies PRs. I'll share the script later.

I've made it add closes: entries to the body for PRs that are being combined. I wonder if it would be worth experimenting to determine if there's a better approach - I'm thinking about how release-drafter will react. Perhaps not closing the original PR, but letting GitHub detect that the original PR should be marked merged by virtue of its head commit being merged into master. I'll experiment with this aspect in a separate repo, as it's not something that can be easily reversed if it fails to work.

There seems to be something screwy with GitHub's PR search as well: I'm using gh pr list --search "label:dependencies status:success" which I believe should list all dependencies PRs that are passing checks. I'm only getting three results back right now, though, which is fewer than it should be. 🤔

@rnorth
Copy link
Member Author

rnorth commented Jun 7, 2021

Good news, having tested a few things over in https://github.com/rnorth/pr-closure-test:

  • GitHub detects closure of the original PR when its commits are merged in the combined PR. Therefore, it's not necessary to put closes #XYZ in the combined PR. We can put something like Includes #XYZ for reference.

  • Release Drafter also behaves as we'd want: the original PRs are included in the draft release. We actually see (a) each of the original PRs, and (b) the combined dependencies PR, as shown here: https://github.com/rnorth/pr-closure-test/releases/tag/0.0.1 (only using one original PR here)

@rnorth
Copy link
Member Author

rnorth commented Jun 7, 2021

Double-checking the above observations in this repo.

@rnorth rnorth marked this pull request as ready for review June 7, 2021 08:16
@rnorth rnorth merged commit b4bafc9 into master Jun 7, 2021
@rnorth rnorth deleted the combined-pr-branch branch June 7, 2021 08:16
@rnorth
Copy link
Member Author

rnorth commented Jun 7, 2021

Welp, it doesn't seem to have worked here. Looking into why.

@rnorth
Copy link
Member Author

rnorth commented Jun 7, 2021

Ah I know now: This approach does not work if 'squash and merge' is used, which is entirely reasonable. We don't currently have 'Allow merge commits' enabled in this repo.

I think the benefits will outweigh the costs of enabling this merge type. Will raise a separate issue to track this work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file type/housekeeping
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant