Merge branch 'stable-2.14' into stable-2.15
* stable-2.14:
Bazel: Include eclipse-out directory in .bazelignore
Change-Id: I3d3d6bbb187527d4579e9d514fac35fc6c44df61
diff --git a/README.md b/README.md
index a96c3c5..372bba1 100644
--- a/README.md
+++ b/README.md
@@ -1,7 +1,7 @@
Autosubmitter
=============
-[](https://gerrit-ci.gerritforge.com/view/Plugins-stable-2.14/job/plugin-automerge-plugin-gh-bazel-stable-2.14/)
+[](https://gerrit-ci.gerritforge.com/job/plugin-autosubmitter-bazel-master/)
A [gerrit](https://www.gerritcodereview.com) plugin that takes care of
automatically merging reviews when all approvals are present.
diff --git a/WORKSPACE b/WORKSPACE
index e98960f..809778d 100644
--- a/WORKSPACE
+++ b/WORKSPACE
@@ -3,7 +3,7 @@
load("//:bazlets.bzl", "load_bazlets")
load_bazlets(
- commit = "714a32382ebd02919007d3514513af4395768d80",
+ commit = "f1d3eefb78029298afe119b0d8b2a43de2b510f6",
#local_path = "/home/<user>/projects/bazlets",
)
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java b/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
index db37a37..ee8801f 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
@@ -1,24 +1,28 @@
package com.criteo.gerrit.plugins.automerge;
+import static com.google.gerrit.server.permissions.ChangePermission.READ;
+
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.extensions.api.changes.SubmitInput;
import com.google.gerrit.extensions.client.ChangeStatus;
-import com.google.gerrit.extensions.restapi.RestApiException;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
-import com.google.gerrit.server.account.AccountByEmailCache;
+import com.google.gerrit.server.account.Emails;
import com.google.gerrit.server.change.ChangesCollection;
import com.google.gerrit.server.change.GetRelated;
import com.google.gerrit.server.change.GetRelated.ChangeAndCommit;
import com.google.gerrit.server.change.GetRelated.RelatedInfo;
-import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.change.Submit;
-import com.google.gerrit.server.git.MergeUtil;
-import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.project.NoSuchChangeException;
+import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.SubmitRuleEvaluator;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
@@ -34,12 +38,8 @@
private static final Logger log = LoggerFactory.getLogger(AtomicityHelper.class);
- @Inject private AccountByEmailCache byEmailCache;
-
@Inject ChangeData.Factory changeDataFactory;
- @Inject private ChangeControl.GenericFactory changeFactory;
-
@Inject private ChangesCollection collection;
@Inject AutomergeConfig config;
@@ -50,12 +50,16 @@
@Inject GetRelated getRelated;
- @Inject MergeUtil.Factory mergeUtilFactory;
-
- @Inject Provider<PostReview> reviewer;
-
@Inject Submit submitter;
+ @Inject Emails emails;
+
+ @Inject ChangeNotes.Factory changeNotesFactory;
+
+ @Inject PermissionBackend permissionBackend;
+
+ @Inject SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
+
/**
* Check if the current patchset of the specified change has dependent unmerged changes.
*
@@ -64,10 +68,13 @@
* @return true or false
* @throws IOException
* @throws NoSuchChangeException
+ * @throws NoSuchProjectException
* @throws OrmException
+ * @throws PermissionBackendException
*/
public boolean hasDependentReview(String project, int number)
- throws IOException, NoSuchChangeException, OrmException {
+ throws IOException, NoSuchChangeException, NoSuchProjectException, OrmException,
+ PermissionBackendException {
RevisionResource r = getRevisionResource(project, number);
RelatedInfo related = getRelated.apply(r);
log.debug(String.format("Checking for related changes on review %d", number));
@@ -105,7 +112,7 @@
* Check if a change is an atomic change or not. A change is atomic if it has the atomic topic
* prefix.
*
- * @param change a ChangeAttribute instance
+ * @param change a Change instance
* @return true or false
*/
public boolean isAtomicReview(final Change change) {
@@ -131,7 +138,10 @@
new com.google.gerrit.reviewdb.client.Change.Id(change));
// For draft reviews, the patchSet must be set to avoid an NPE.
final List<SubmitRecord> cansubmit =
- new SubmitRuleEvaluator(changeData).setPatchSet(changeData.currentPatchSet()).evaluate();
+ submitRuleEvaluatorFactory
+ .create(getBotUser(), changeData)
+ .setPatchSet(changeData.currentPatchSet())
+ .evaluate();
log.debug(String.format("Checking if change %d is submitable.", change));
for (SubmitRecord submit : cansubmit) {
if (submit.status != SubmitRecord.Status.OK) {
@@ -143,40 +153,37 @@
return true;
}
- /**
- * Merge a review.
- *
- * @param info
- * @throws RestApiException
- * @throws NoSuchChangeException
- * @throws OrmException
- * @throws IOException
- */
- public void mergeReview(String project, int changeNumber)
- throws RestApiException, NoSuchChangeException, OrmException, IOException {
- submitter.apply(getRevisionResource(project, changeNumber), new SubmitInput());
+ /** Merge a review. */
+ public void mergeReview(String project, int number) throws Exception {
+ submitter.apply(getRevisionResource(project, number), new SubmitInput());
}
public RevisionResource getRevisionResource(String project, int changeNumber)
- throws NoSuchChangeException, OrmException {
- ChangeControl ctl =
- changeFactory.validateFor(
- db.get(), new com.google.gerrit.reviewdb.client.Change.Id(changeNumber), getBotUser());
- ChangeData changeData =
- changeDataFactory.create(
- db.get(),
- new Project.NameKey(project),
- new com.google.gerrit.reviewdb.client.Change.Id(changeNumber));
- RevisionResource r = new RevisionResource(collection.parse(ctl), changeData.currentPatchSet());
- return r;
+ throws OrmException {
+ com.google.gerrit.reviewdb.client.Change.Id changeId =
+ new com.google.gerrit.reviewdb.client.Change.Id(changeNumber);
+ ChangeNotes notes = changeNotesFactory.createChecked(changeId);
+ try {
+ permissionBackend.user(getBotUser()).change(notes).database(db).check(READ);
+ ChangeData changeData =
+ changeDataFactory.create(db.get(), new Project.NameKey(project), changeId);
+ RevisionResource r =
+ new RevisionResource(collection.parse(changeId), changeData.currentPatchSet());
+ return r;
+ } catch (ResourceNotFoundException | AuthException | PermissionBackendException e) {
+ throw new NoSuchChangeException(changeId);
+ }
}
private IdentifiedUser getBotUser() {
- final Set<Account.Id> ids = byEmailCache.get(config.getBotEmail());
- if (ids.isEmpty()) {
- throw new RuntimeException("No user found with email: " + config.getBotEmail());
+ try {
+ Set<Account.Id> ids = emails.getAccountFor(config.getBotEmail());
+ if (ids.isEmpty()) {
+ throw new RuntimeException("No user found with email: " + config.getBotEmail());
+ }
+ return factory.create(ids.iterator().next());
+ } catch (IOException | OrmException e) {
+ throw new RuntimeException("Unable to get account with email: " + config.getBotEmail(), e);
}
- final IdentifiedUser bot = factory.create(ids.iterator().next());
- return bot;
}
}
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
index cc5347b..8145d46 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
@@ -17,6 +17,7 @@
import com.google.common.collect.Lists;
import com.google.gerrit.common.EventListener;
import com.google.gerrit.extensions.api.GerritApi;
+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.events.LifecycleListener;
@@ -29,20 +30,15 @@
import com.google.gerrit.server.data.ChangeAttribute;
import com.google.gerrit.server.events.ChangeEvent;
import com.google.gerrit.server.events.CommentAddedEvent;
-import com.google.gerrit.server.events.DraftPublishedEvent;
import com.google.gerrit.server.events.Event;
import com.google.gerrit.server.events.PatchSetCreatedEvent;
import com.google.gerrit.server.events.RefUpdatedEvent;
import com.google.gerrit.server.events.ReviewerDeletedEvent;
import com.google.gerrit.server.events.TopicChangedEvent;
import com.google.gerrit.server.git.MergeUtil;
-import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.query.change.ChangeData;
-import com.google.gerrit.server.update.UpdateException;
-import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
-import java.io.IOException;
import java.util.EnumSet;
import java.util.List;
import org.slf4j.Logger;
@@ -76,7 +72,6 @@
@Override
public synchronized void onEvent(final Event event) {
if (event instanceof TopicChangedEvent
- || event instanceof DraftPublishedEvent
|| event instanceof ReviewerDeletedEvent
|| // A blocking score might be removed when a reviewer is deleted.
event instanceof PatchSetCreatedEvent) {
@@ -98,7 +93,7 @@
}
try {
autoSubmitIfMergeable(change);
- } catch (OrmException | RestApiException | IOException | UpdateException e) {
+ } catch (Exception e) {
log.error("An exception occured while trying to merge change #" + change.number, e);
}
}
@@ -112,7 +107,7 @@
try {
checkReviewExists(change.number);
autoSubmitIfMergeable(Change.from(change));
- } catch (RestApiException | OrmException | UpdateException | IOException e) {
+ } catch (Exception e) {
log.error("An exception occured while trying to atomic merge a change.", e);
throw new RuntimeException(e);
}
@@ -137,7 +132,7 @@
+ refName
+ ": Submitting ...");
autoSubmitIfMergeable(Change.from(submittable));
- } catch (UpdateException | RestApiException | OrmException | IOException e) {
+ } catch (Exception e) {
log.error(
"Cannot autosubmit change "
+ submittable._number
@@ -154,8 +149,7 @@
}
}
- private void autoSubmitIfMergeable(Change change)
- throws OrmException, RestApiException, NoSuchChangeException, IOException, UpdateException {
+ private void autoSubmitIfMergeable(Change change) throws Exception {
if (atomicityHelper.isSubmittable(change.project, change.number)) {
if (atomicityHelper.isAtomicReview(change)) {
attemptToMergeAtomic(change);
@@ -183,14 +177,19 @@
return false;
}
- private void attemptToMergeAtomic(Change change)
- throws RestApiException, OrmException, NoSuchChangeException, IOException, UpdateException {
+ private void attemptToMergeAtomic(Change change) throws Exception {
final List<ChangeInfo> related = Lists.newArrayList();
- related.addAll(
- api.changes()
- .query("status: open AND topic: " + change.topic)
- .withOption(ListChangesOption.CURRENT_REVISION)
- .get());
+ if (atomicityHelper.isAtomicReview(change)) {
+ related.addAll(
+ api.changes()
+ .query("status: open AND topic: " + change.topic)
+ .withOption(ListChangesOption.CURRENT_REVISION)
+ .get());
+ } else {
+ ChangeApi changeApi = api.changes().id(change.project, change.branch, change.id);
+ related.add(changeApi.get(EnumSet.of(ListChangesOption.CURRENT_REVISION)));
+ }
+
for (final ChangeInfo info : related) {
if (!atomicityHelper.isSubmittable(info.project, info._number)) {
log.info(
@@ -224,8 +223,7 @@
}
}
- private void attemptToMergeNonAtomic(Change change)
- throws IOException, OrmException, RestApiException {
+ private void attemptToMergeNonAtomic(Change change) throws Exception {
// There may be a parent commit that it not merged while having all approvals
// because it is part of a cross-repo. We take care to not let Gerrit merge it
// by merging only the commits whose parents are already merged.
@@ -255,7 +253,7 @@
reviewUpdater.commentOnReview(
change.project, change.number, config.atomicReviewsSameRepo.getContent());
}
- } catch (RestApiException | IOException | OrmException | UpdateException e) {
+ } catch (Exception e) {
throw new RuntimeException(e);
}
}
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/Change.java b/src/main/java/com/criteo/gerrit/plugins/automerge/Change.java
index 3216cca..8728bed 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/Change.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/Change.java
@@ -12,19 +12,29 @@
public class Change {
public final String project;
public final int number;
+ public final String id;
public final String topic;
+ public final String branch;
- private Change(String project, int number, String topic) {
+ private Change(String project, int number, String id, String topic, String branch) {
this.project = project;
this.number = number;
+ this.id = id;
this.topic = topic;
+ this.branch = branch;
}
public static Change from(ChangeAttribute changeAttribute) {
- return new Change(changeAttribute.project, changeAttribute.number, changeAttribute.topic);
+ return new Change(
+ changeAttribute.project,
+ changeAttribute.number,
+ changeAttribute.id,
+ changeAttribute.topic,
+ changeAttribute.branch);
}
public static Change from(ChangeInfo changeInfo) {
- return new Change(changeInfo.project, changeInfo._number, changeInfo.topic);
+ return new Change(
+ changeInfo.project, changeInfo._number, changeInfo.id, changeInfo.topic, changeInfo.branch);
}
}
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java b/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java
index d5f982f..690ebc0 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/ReviewUpdater.java
@@ -1,14 +1,10 @@
package com.criteo.gerrit.plugins.automerge;
import com.google.gerrit.extensions.api.changes.ReviewInput;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.update.UpdateException;
-import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
-import java.io.IOException;
public class ReviewUpdater {
/**
@@ -22,8 +18,7 @@
@Inject private AtomicityHelper atomicityHelper;
- public void commentOnReview(String project, int number, String comment)
- throws RestApiException, OrmException, IOException, UpdateException {
+ public void commentOnReview(String project, int number, String comment) throws Exception {
ReviewInput reviewInput = createComment(comment);
applyComment(project, number, reviewInput);
}
@@ -32,8 +27,7 @@
return new ReviewInput().message(commentsPrefix + comment);
}
- private void applyComment(String project, int number, ReviewInput comment)
- throws RestApiException, OrmException, IOException, UpdateException {
+ private void applyComment(String project, int number, ReviewInput comment) throws Exception {
RevisionResource r = atomicityHelper.getRevisionResource(project, number);
reviewer.get().apply(r, comment);
}
diff --git a/test.rb b/test.rb
index 995cbb9..859d375 100755
--- a/test.rb
+++ b/test.rb
@@ -179,16 +179,6 @@
check_status(commit2, 'MERGED')
end
- def test_published_draft_is_autosubmitted
- commit_id = create_review(PROJECT1, "review0 on #{PROJECT1}", draft: true)
- approve_review(commit_id)
- check_status(commit_id, 'DRAFT')
-
- publish_draft(commit_id)
-
- check_status(commit_id, 'MERGED')
- end
-
private
def project_dir(project_name)
@@ -205,11 +195,7 @@
reviews = gerrit_query(query)
reviews.each do |review|
review_number = review['number']
- if review['status'] == 'DRAFT'
- execute("#{GERRIT_SSH} gerrit review --delete #{review_number},1")
- else
- execute("#{GERRIT_SSH} gerrit review --abandon #{review_number},1")
- end
+ execute("#{GERRIT_SSH} gerrit review --abandon #{review_number},1")
end
end
@@ -221,14 +207,14 @@
change_id
end
- def create_review(project_name, message, topic = nil, change_id = nil, draft: false)
+ def create_review(project_name, message, topic = nil, change_id = nil)
topic_suffix = "/#{topic}" if topic
message = "#{message}\n\nChange-Id: #{change_id}" if change_id
execute(["cd #{project_dir(project_name)}",
"echo 0 >> a",
"git add .",
%Q(git commit -m "#{message}"),
- "git push origin HEAD:refs/#{draft ? 'drafts' : 'for'}/master#{topic_suffix}"
+ "git push origin HEAD:refs/for/master#{topic_suffix}"
].join(" && "))
commit_id = execute("cd #{project_dir(project_name)} && git rev-parse HEAD")
refute(commit_id.empty?, "missing commit-id")
@@ -236,11 +222,7 @@
end
def approve_review(commit_id)
- execute("#{GERRIT_SSH} gerrit review --verified 1 --code-review 2 #{commit_id}")
- end
-
- def publish_draft(commit_id)
- execute("#{GERRIT_SSH} gerrit review --publish #{commit_id}")
+ execute("#{GERRIT_SSH} gerrit review --strict-labels --verified 1 --code-review 2 #{commit_id}")
end
def abandon_review(commit_id)
@@ -251,15 +233,17 @@
reviews = gerrit_query("commit:#{commit_id}")
assert_equal(1, reviews.size, "missing review with commit #{commit_id}")
review = reviews[0]
- assert_equal(expected_status, review['status'], "wrong status on review: #{review['number']}")
+ assert_equal(expected_status, review['status'], "wrong status on review #{review['number']} '#{review['subject']}'")
end
def check_label(commit_id, label_name, expected_label_value)
reviews = gerrit_query("commit:#{commit_id}", "--all-approvals")
assert_equal(1, reviews.size, "missing review with commit #{commit_id}")
review = reviews[0]
- code_review_approvals = review['patchSets'][0]['approvals'].select {|ap| ap['description'] == "Code-Review"}
- refute(code_review_approvals.empty?)
+ approvals = review['patchSets'][0]['approvals']
+ refute(approvals.nil?, "No approval on #{commit_id}")
+ code_review_approvals = approvals.select {|ap| ap['description'] == "Code-Review"}
+ refute(code_review_approvals.empty?, "No code-review score on #{commit_id}")
assert_equal(expected_label_value, code_review_approvals[0]['value'], "wrong label on review: #{review['number']}")
end
