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