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

941: Warn about required merge after dependent PR integration #1134

Closed
wants to merge 3 commits into from
Closed
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -76,8 +76,26 @@ public void onNewPullRequest(PullRequest pr, Path scratchPath) {
@Override
public void onStateChange(PullRequest pr, Path scratchPath, Issue.State oldState) {
if (pr.state() == Issue.State.CLOSED) {
PreIntegrations.retargetDependencies(pr);
var retargetedDependencies = PreIntegrations.retargetDependencies(pr);
deleteBranch(pr);
for (var retargeted : retargetedDependencies) {
retargeted.addComment("""
The dependent pull request has now been integrated, and the target branch of this pull request \
has been updated. This means that changes from the dependent pull request can start to show up \
as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, \
simply merge the latest changes from the target branch into this pull request by running commands \
rwestberg marked this conversation as resolved.
Show resolved Hide resolved
similar to these in the local repository for your personal fork:

```bash
git checkout %s
git fetch %s %s
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge %s"
git push
```
""".formatted(pr.sourceRef(), pr.repository().webUrl(), pr.targetRef(), pr.targetRef()));
}
} else {
pushBranch(pr);
}
@@ -232,6 +232,12 @@ void retarget(TestInfo testInfo) throws IOException {
// The follow-up PR should have been retargeted
followUpPr = repo.pullRequest(followUpPr.id());
assertEquals("master", followUpPr.targetRef());

// Instructions on how to adapt to the newly integrated changes should have been posted
var lastComment = followUpPr.comments().get(followUpPr.comments().size() - 1);
assertTrue(lastComment.body().contains("The dependent pull request has now"), lastComment.body());
assertTrue(lastComment.body().contains("git checkout source"), lastComment.body());
assertTrue(lastComment.body().contains("git commit -m \"Merge master\""), lastComment.body());
}
}
}
@@ -22,7 +22,7 @@
*/
package org.openjdk.skara.forge;

import java.util.Optional;
import java.util.*;

public class PreIntegrations {
public static Optional<String> dependentPullRequestId(PullRequest pr) {
@@ -42,15 +42,18 @@ public static String preIntegrateBranch(PullRequest pr) {
return "pr/" + pr.id();
}

public static void retargetDependencies(PullRequest pr) {
public static Collection<PullRequest> retargetDependencies(PullRequest pr) {
var ret = new ArrayList<PullRequest>();
var dependentRef = preIntegrateBranch(pr);

var candidates = pr.repository().pullRequests();
for (var candidate : candidates) {
if (candidate.targetRef().equals(dependentRef)) {
candidate.setTargetRef(pr.targetRef());
ret.add(candidate);
}
}
return ret;
}

public static boolean isPreintegrationBranch(String name) {