Merge "Revert "Upstream +2 should not override merge conflict -2.""
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 fc918a4..2ecb88b 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/automerger/ConfigLoader.java
@@ -248,11 +248,6 @@
return getConfig().getBoolean("global", "disableMinAutomergeVote", false);
}
- public boolean contextUserIdIsSet() throws ConfigInvalidException {
- int contextUserId = getConfig().getInt("global", "contextUserId", -1);
- return contextUserId > 0;
- }
-
public Account.Id getContextUserId() throws ConfigInvalidException {
int contextUserId = getConfig().getInt("global", "contextUserId", -1);
if (contextUserId > 0) {
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 0a1287f..fae8344 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreator.java
@@ -82,7 +82,6 @@
protected GerritApi gApi;
protected ConfigLoader config;
protected CurrentUser user;
- protected MergeValidator mergeValidator;
private final OneOffRequestContext oneOffRequestContext;
@@ -92,7 +91,6 @@
this.gApi = gApi;
this.config = config;
this.oneOffRequestContext = oneOffRequestContext;
- mergeValidator = new MergeValidator(gApi, config);
}
/**
@@ -217,34 +215,21 @@
for (Integer changeNumber : existingDownstream) {
ChangeInfo downstreamChange =
gApi.changes().id(changeNumber).get(EnumSet.of(ListChangesOption.CURRENT_REVISION));
- if (shouldReceivePropagatedVote(downstreamChange)) {
- for (Map.Entry<String, LabelInfo> labelEntry : labels.entrySet()) {
- if (labelEntry.getValue().all.size() > 0) {
- OptionalInt maxVote =
- labelEntry
- .getValue()
- .all
- .stream()
- .filter(o -> o.value != null)
- .mapToInt(i -> i.value)
- .max();
+ for (Map.Entry<String, LabelInfo> labelEntry : labels.entrySet()) {
+ if (labelEntry.getValue().all.size() > 0) {
+ OptionalInt maxVote =
+ labelEntry
+ .getValue()
+ .all
+ .stream()
+ .filter(o -> o.value != null)
+ .mapToInt(i -> i.value)
+ .max();
- if (maxVote.isPresent()) {
- updateVote(downstreamChange, labelEntry.getKey(), (short) maxVote.getAsInt());
- }
+ if (maxVote.isPresent()) {
+ updateVote(downstreamChange, labelEntry.getKey(), (short) maxVote.getAsInt());
}
}
- } else {
- try {
- ReviewInput reviewInput = new ReviewInput();
- reviewInput.message(
- "Downstream change is missing. Vote will not be propagated to this change "
- + "until all merge conflicts are resolved and added to the topic.");
- reviewInput.notify = NotifyHandling.NONE;
- gApi.changes().id(downstreamChange._number).revision(CURRENT).review(reviewInput);
- } catch (RestApiException e) {
- log.error("Failed to comment on change for automerger plugin.", e);
- }
}
}
} catch (RestApiException | InvalidQueryParameterException e) {
@@ -348,10 +333,16 @@
reviewInput.labels = labels;
// if this fails, i.e. -2 is restricted, catch it and still post message without a vote.
try {
- gApi.changes().id(mdsMergeInput.changeNumber).revision(CURRENT).review(reviewInput);
+ gApi.changes()
+ .id(mdsMergeInput.changeNumber)
+ .revision(CURRENT)
+ .review(reviewInput);
} catch (AuthException e) {
reviewInput.labels = null;
- gApi.changes().id(mdsMergeInput.changeNumber).revision(CURRENT).review(reviewInput);
+ gApi.changes()
+ .id(mdsMergeInput.changeNumber)
+ .revision(CURRENT)
+ .review(reviewInput);
}
}
}
@@ -583,21 +574,6 @@
return null;
}
- private boolean shouldReceivePropagatedVote(ChangeInfo change)
- throws RestApiException, IOException, ConfigInvalidException, InvalidQueryParameterException {
- boolean shouldPropagate = true;
- // Check change is created by contextUser if contextUser is set
- if (config.contextUserIdIsSet()) {
- Integer accountId = change.revisions.get(change.currentRevision).uploader._accountId;
-
- shouldPropagate = accountId == config.getContextUserId().get();
- }
- // Check that the change does not have any downstream conflicts
- shouldPropagate =
- shouldPropagate && mergeValidator.getMissingDownstreamMerges(change).isEmpty();
- return shouldPropagate;
- }
-
private void automergeChanges(ChangeInfo change, RevisionInfo revisionInfo)
throws RestApiException, IOException, ConfigInvalidException, InvalidQueryParameterException,
OrmException {
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 ed17d57..d692e95 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/automerger/ConfigLoaderIT.java
@@ -227,18 +227,6 @@
assertThat(configLoader.getContextUserId()).isEqualTo(user.get().getAccountId());
}
- @Test
- public void contextUserIdIsSetTest() throws Exception {
- defaultSetup("context_user.config");
- assertThat(configLoader.contextUserIdIsSet()).isEqualTo(true);
- }
-
- @Test
- public void contextUserIdIsSetTest_noContextUser() throws Exception {
- defaultSetup("automerger.config");
- assertThat(configLoader.contextUserIdIsSet()).isEqualTo(false);
- }
-
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 90be9ab..cd36f2d 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/automerger/DownstreamCreatorIT.java
@@ -917,81 +917,6 @@
assertThat(getVote(dsTwoChange, "Code-Review").value).isEqualTo(1);
}
- @Test
- public void testContextUser_conflict() throws Exception {
- Project.NameKey manifestNameKey = defaultSetup();
- // Create initial change
- PushOneCommit.Result initialResult = createChange("subject", "filename", "echo Hello");
- // Project name is scoped by test, so we need to get it from our initial change
- Project.NameKey projectNameKey = initialResult.getChange().project();
- String projectName = projectNameKey.get();
- createBranch(new Branch.NameKey(projectName, "ds_one"));
- createBranch(new Branch.NameKey(projectName, "ds_two"));
- initialResult.assertOkStatus();
- merge(initialResult);
- // Reset to create a sibling
- ObjectId initial = repo().exactRef("HEAD").getLeaf().getObjectId();
- testRepo.reset(initial);
- // Set up a merge conflict between master and ds_one
- PushOneCommit.Result ds2Result =
- createChange(
- testRepo, "ds_two", "subject", "filename", "echo \"Hello asdfsd World\"", "randtopic");
- ds2Result.assertOkStatus();
- merge(ds2Result);
- // Reset to allow our merge conflict to come
- testRepo.reset(initial);
-
- // Create normalUserGroup, containing current user, and contextUserGroup, containing contextUser
- String normalUserGroup = createGroup("normalUserGroup");
- gApi.groups().id(normalUserGroup).addMembers(user.get().getAccountId().toString());
- AccountApi contextUserApi = gApi.accounts().create("aContextUser");
- String contextUserGroup = createGroup("contextUserGroup");
- gApi.groups().id(contextUserGroup).addMembers(contextUserApi.get().name);
-
- // Grant +2 to context user, since it doesn't have it by default
- grantLabel(
- "Code-Review",
- -2,
- 2,
- projectNameKey,
- "refs/heads/*",
- false,
- AccountGroup.UUID.parse(gApi.groups().id(contextUserGroup).get().id),
- false);
- pushContextUserConfig(manifestNameKey.get(), projectName, contextUserApi.get()._accountId);
-
- // After we upload our config, we upload a new patchset to create the downstreams
- PushOneCommit.Result result =
- createChange("subject", "filename", "echo 'Hello World!'", "sometopic");
- result.assertOkStatus();
- // Check that there are the correct number of changes in the topic
- List<ChangeInfo> changesInTopic =
- gApi.changes()
- .query("topic:" + gApi.changes().id(result.getChangeId()).topic())
- .withOptions(ListChangesOption.CURRENT_REVISION, ListChangesOption.CURRENT_COMMIT)
- .get();
- assertThat(changesInTopic).hasSize(2);
-
- List<ChangeInfo> sortedChanges = sortedChanges(changesInTopic);
-
- ChangeInfo dsOneChangeInfo = sortedChanges.get(0);
- assertThat(dsOneChangeInfo.branch).isEqualTo("ds_one");
- ChangeApi dsOneChange = gApi.changes().id(dsOneChangeInfo._number);
- // This is -2 because there is a merge conflict downstream
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(-2);
- assertThat(getVote(dsOneChange, "Code-Review").tag).isEqualTo("autogenerated:MergeConflict");
-
- ChangeInfo masterChangeInfo = sortedChanges.get(1);
- ChangeApi masterChange = gApi.changes().id(masterChangeInfo._number);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(0);
- assertThat(masterChangeInfo.branch).isEqualTo("master");
-
- // Test that +2 on master won't propagate to ds_one, as conflict is not resolved.
- approve(masterChangeInfo.id);
- assertThat(getVote(masterChange, "Code-Review").value).isEqualTo(2);
- assertThat(getVote(dsOneChange, "Code-Review").value).isEqualTo(-2);
- }
-
private Project.NameKey defaultSetup() throws Exception {
Project.NameKey manifestNameKey = createProject("platform/manifest");
setupTestRepo("default.xml", manifestNameKey, "master", "default.xml");
