Allow configurable vote values.
Since the label is configurable, the vote values should be
as well. Users of the plugin may want to be able to vote
Code-Review -2 on a merge conflict, or Verified +1 on a
successful merge.
We also now default to Code-Review instead of Verified
for voting, since Verified may not be installed by
default.
Change-Id: I8aed1d313641c47f2e15f7acad083b2fab7e1c76
diff --git a/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java b/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
index e4e11da..97ce063 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
@@ -126,7 +126,7 @@
*/
public String getAutomergeLabel() throws ConfigInvalidException {
String automergeLabel = getConfig().getString("global", null, "automergeLabel");
- return automergeLabel != null ? automergeLabel : "Verified";
+ return automergeLabel != null ? automergeLabel : "Code-Review";
}
/**
@@ -213,6 +213,14 @@
return downstreamBranches;
}
+ public short getMaxAutomergeVote() throws ConfigInvalidException {
+ return (short) getConfig().getInt("global", "maxAutomergeVote", 1);
+ }
+
+ public short getMinAutomergeVote() throws ConfigInvalidException {
+ return (short) getConfig().getInt("global", "minAutomergeVote", 1);
+ }
+
// Returns overriden manifest config if specified, default if not
private String getManifestFile() throws ConfigInvalidException {
String manifestFile = getConfig().getString("global", null, "manifestFile");
diff --git a/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java b/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
index f953898..634924f 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
@@ -167,7 +167,7 @@
ChangeInfo downstreamChange =
gApi.changes().id(changeNumber).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
for (Map.Entry<String, ApprovalInfo> label : approvals.entrySet()) {
- updateVote(downstreamChange, label.getKey(), label.getValue().value.shortValue());
+ propagateVote(downstreamChange, label.getKey(), label.getValue().value.shortValue());
}
}
} catch (RestApiException | ConfigInvalidException e) {
@@ -222,7 +222,7 @@
}
/**
- * Creates merges downstream, and votes -1 on the automerge label if we have a failed merge.
+ * Creates merges downstream, and votes on the automerge label if we have a failed merge.
*
* @param mdsMergeInput Input containing the downstream branch map and source change ID.
* @throws RestApiException Throws if we fail a REST API call.
@@ -244,9 +244,9 @@
} catch (FailedMergeException e) {
reviewInput.message = e.getDisplayString();
reviewInput.notify = NotifyHandling.ALL;
- vote = -1;
+ // Vote minAutomergeVote if we hit a conflict.
+ vote = config.getMinAutomergeVote();
}
- // Zero out automerge label if success, -1 vote if fail.
labels.put(config.getAutomergeLabel(), vote);
reviewInput.labels = labels;
gApi.changes()
@@ -367,9 +367,10 @@
*
* @param sdsMergeInput Input containing metadata for the merge.
* @throws RestApiException
+ * @throws ConfigInvalidException
*/
public void createSingleDownstreamMerge(SingleDownstreamMergeInput sdsMergeInput)
- throws RestApiException {
+ throws RestApiException, ConfigInvalidException {
String currentTopic = setTopic(sdsMergeInput.sourceId, sdsMergeInput.topic);
@@ -396,7 +397,10 @@
sdsMergeInput.downstreamBranch);
}
- gApi.changes().create(downstreamChangeInput);
+ ChangeApi downstreamChange = gApi.changes().create(downstreamChangeInput);
+
+ // Vote maxAutomergeVote on the change so we know it was successful.
+ updateVote(downstreamChange.get(), config.getAutomergeLabel(), config.getMaxAutomergeVote());
}
private void automergeChanges(ChangeInfo change, RevisionInfo revisionInfo)
@@ -461,12 +465,16 @@
}
}
- private void updateVote(ChangeInfo change, String label, short vote)
+ private void propagateVote(ChangeInfo change, String label, short vote)
throws RestApiException, ConfigInvalidException {
if (label.equals(config.getAutomergeLabel())) {
log.debug("Not updating automerge label, as it blocks when there is a merge conflict.");
return;
}
+ updateVote(change, label, vote);
+ }
+
+ private void updateVote(ChangeInfo change, String label, short vote) throws RestApiException {
log.debug("Giving {} for label {} to {}", vote, label, change.id);
// Vote on all downstream branches unless merge conflict.
ReviewInput reviewInput = new ReviewInput();
@@ -478,7 +486,7 @@
private void updateDownstreamMerge(
String newParentRevision, String upstreamSubject, Integer sourceNum, boolean doMerge)
- throws RestApiException {
+ throws RestApiException, ConfigInvalidException {
MergeInput mergeInput = new MergeInput();
mergeInput.source = newParentRevision;
@@ -500,7 +508,8 @@
originalChange.restore(restoreInput);
}
- originalChange.createMergePatchSet(mergePatchSetInput);
+ ChangeInfo downstreamChange = originalChange.createMergePatchSet(mergePatchSetInput);
+ updateVote(downstreamChange, config.getAutomergeLabel(), config.getMaxAutomergeVote());
}
private String getPreviousRevision(ChangeApi change, int currentPatchSetNumber)
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md
index 8f3dfe8..ada9bb5 100644
--- a/src/main/resources/Documentation/config.md
+++ b/src/main/resources/Documentation/config.md
@@ -25,10 +25,24 @@
```
global.automergeLabel
-: Label to vote -1 on when there is a merge conflict.
+: Label to vote minAutomergeVote on when there is a merge conflict.
When the automerger detects a merge conflict from one branch to another, it
- will vote -1 on this label.
+ will vote minAutomergeVote on this label.
+
+global.maxAutomergeVote
+: Value to vote on a successful automerge.
+
+ When the automerger succeeds in merging downstream, it will vote
+ maxAutomergeVote on the downstream change. The original change uploaded by
+ the user will have a vote of 0, so that it can be easily programatically
+ distinguished from the otheres.
+
+global.minAutomergeVote
+: Value to vote on a failed automerge.
+
+ When the automerger detects a merge conflict from one branch to another, it
+ will vote minAutomergeVote for the configured automergeLabel.
global.hostName
: Hostname to use in a custom conflict message.
diff --git a/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java b/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java
index c2f9637..b5afb95 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java
@@ -188,6 +188,18 @@
.isEqualTo("line1\n" + "line2\n" + "line3 ${branch}\n" + "line4");
}
+ @Test
+ public void getMaxAutomergeVoteTest() throws Exception {
+ defaultSetup("alternate.config");
+ assertThat(configLoader.getMaxAutomergeVote()).isEqualTo(5);
+ }
+
+ @Test
+ public void getMinAutomergeVoteTest() throws Exception {
+ defaultSetup("alternate.config");
+ assertThat(configLoader.getMinAutomergeVote()).isEqualTo(-3);
+ }
+
private void setupTestRepo(
String resourceName, Project.NameKey projectNameKey, String branchName, String filename)
throws Exception {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
index 4a7fc24..a9863ca 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
@@ -23,13 +23,16 @@
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.TestPlugin;
import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.restapi.BinaryResult;
+import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import java.io.InputStream;
import java.io.InputStreamReader;
+import java.util.EnumSet;
import java.util.List;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.junit.TestRepository;
@@ -87,15 +90,18 @@
ChangeApi downstreamChange = gApi.changes().id(c._number);
// It should skip ds_one, since this is a DO NOT MERGE
if (c.branch.equals("ds_one")) {
+ assertThat(getVote(downstreamChange, "Code-Review")).isEqualTo(1);
assertThat(downstreamChange.get().subject).contains("skipped:");
assertThat(downstreamChange.current().files().keySet()).contains("filename");
assertThat(downstreamChange.current().files().get("filename").linesDeleted).isEqualTo(1);
} else if (c.branch.equals("ds_two")) {
+ assertThat(getVote(downstreamChange, "Code-Review")).isEqualTo(1);
// It should not skip ds_two, since it is marked with mergeAll: true
assertThat(downstreamChange.get().subject).doesNotContain("skipped:");
BinaryResult downstreamContent = downstreamChange.current().file("filename").content();
assertThat(downstreamContent.asString()).isEqualTo(content.asString());
} else {
+ assertThat(getVote(downstreamChange, "Code-Review")).isEqualTo(0);
assertThat(c.branch).isEqualTo("master");
}
}
@@ -212,4 +218,14 @@
push.to(RefNames.REFS_CONFIG).assertOkStatus();
}
}
+
+ private int getVote(ChangeApi change, String label) throws RestApiException {
+ return change
+ .get(EnumSet.of(ListChangesOption.DETAILED_LABELS))
+ .labels
+ .get(label)
+ .all
+ .get(0)
+ .value;
+ }
}
diff --git a/src/test/resources/com/googlesource/gerrit/plugins/automerger/alternate.config b/src/test/resources/com/googlesource/gerrit/plugins/automerger/alternate.config
index 001a1cf..1016e89 100644
--- a/src/test/resources/com/googlesource/gerrit/plugins/automerger/alternate.config
+++ b/src/test/resources/com/googlesource/gerrit/plugins/automerger/alternate.config
@@ -12,6 +12,8 @@
blankMerge = .*DO NOT MERGE.*
manifestFile = default.xml
manifestProject = platform/manifest
+ maxAutomergeVote = 5
+ minAutomergeVote = -3
ignoreProjects = platform/ignore/me
conflictMessage = line1\n\
line2\n\
