Merge "Improve assertions in test.rb"
diff --git a/README.md b/README.md
index 34b2d05..372bba1 100644
--- a/README.md
+++ b/README.md
@@ -12,3 +12,14 @@
when all approvals for all reviews are present, and all reviews are mergeable.
Requires Gerrit 2.14 or later.
+
+Configuration
+-------------
+
+```
+[automerge]
+ botEmail=admin@example.com
+[commentlink "change"]
+ match = "#/c/(\\d+)"
+ html = "<a href=\"/#/c/$1/\">$1</a>"
+```
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 e9e548d..98d67b3 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AtomicityHelper.java
@@ -1,77 +1,66 @@
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.common.ChangeInfo;
+import com.google.gerrit.extensions.client.ChangeStatus;
+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.Change;
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.data.ChangeAttribute;
-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.SubmitRuleEvaluator;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
-
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
import java.io.IOException;
import java.util.List;
import java.util.Set;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
public class AtomicityHelper {
- private final static Logger log = LoggerFactory.getLogger(AtomicityHelper.class);
+ private static final Logger log = LoggerFactory.getLogger(AtomicityHelper.class);
- @Inject
- private AccountByEmailCache byEmailCache;
+ @Inject ChangeData.Factory changeDataFactory;
- @Inject
- ChangeData.Factory changeDataFactory;
+ @Inject private ChangesCollection collection;
- @Inject
- private ChangeControl.GenericFactory changeFactory;
+ @Inject AutomergeConfig config;
- @Inject
- private ChangesCollection collection;
+ @Inject Provider<ReviewDb> db;
- @Inject
- AutomergeConfig config;
+ @Inject private IdentifiedUser.GenericFactory factory;
- @Inject
- Provider<ReviewDb> db;
+ @Inject GetRelated getRelated;
- @Inject
- private IdentifiedUser.GenericFactory factory;
+ @Inject Submit submitter;
- @Inject
- GetRelated getRelated;
+ @Inject Emails emails;
- @Inject
- MergeUtil.Factory mergeUtilFactory;
+ @Inject ChangeNotes.Factory changeNotesFactory;
- @Inject
- Provider<PostReview> reviewer;
+ @Inject PermissionBackend permissionBackend;
- @Inject
- Submit submitter;
+ @Inject SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
/**
- * Check if the current patchset of the specified change has dependent
- * unmerged changes.
+ * Check if the current patchset of the specified change has dependent unmerged changes.
*
* @param project
* @param number
@@ -80,23 +69,52 @@
* @throws NoSuchChangeException
* @throws OrmException
*/
- public boolean hasDependentReview(String project, int number) throws IOException, NoSuchChangeException, OrmException {
- RevisionResource r = getRevisionResource(project, number);
+ public boolean hasDependentReview(String project, int number)
+ throws IOException, NoSuchChangeException, OrmException {
+ RevisionResource r = getRevisionResource(project, number);
RelatedInfo related = getRelated.apply(r);
log.debug(String.format("Checking for related changes on review %d", number));
- return related.changes.size() > 0;
+
+ String checkedCommitSha1 = r.getPatchSet().getRevision().get();
+ int firstParentIndex = 0;
+ int i = 0;
+ for (ChangeAndCommit c : related.changes) {
+ if (checkedCommitSha1.equals(c.commit.commit)) {
+ firstParentIndex = i + 1;
+ log.debug(
+ String.format(
+ "First parent index on review %d is %d on commit %s",
+ number, firstParentIndex, c.commit.commit));
+ break;
+ }
+ i++;
+ }
+
+ boolean hasNonMergedParent = false;
+ for (ChangeAndCommit c : related.changes.subList(firstParentIndex, related.changes.size())) {
+ if (!ChangeStatus.MERGED.toString().equals(c.status)) {
+ log.info(
+ String.format(
+ "Found non merged parent commit on review %d: %s", number, c.commit.commit));
+ hasNonMergedParent = true;
+ break;
+ }
+ }
+
+ return hasNonMergedParent;
}
/**
- * Check if a change is an atomic change or not. A change is atomic if it has
- * the atomic topic prefix.
+ * 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 ChangeAttribute change) {
+ public boolean isAtomicReview(final Change change) {
final boolean atomic = change.topic != null && change.topic.startsWith(config.getTopicPrefix());
- log.debug(String.format("Checking if change %s is an atomic change: %b", change.number, atomic));
+ log.debug(
+ String.format("Checking if change %s is an atomic change: %b", change.number, atomic));
return atomic;
}
@@ -109,9 +127,17 @@
* @throws OrmException
*/
public boolean isSubmittable(String project, int change) throws OrmException {
- ChangeData changeData = changeDataFactory.create(db.get(), new Project.NameKey(project), new Change.Id(change));
+ ChangeData changeData =
+ changeDataFactory.create(
+ db.get(),
+ new Project.NameKey(project),
+ 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();
+ final List<SubmitRecord> cansubmit =
+ submitRuleEvaluatorFactory
+ .create(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) {
@@ -123,29 +149,37 @@
return true;
}
- /**
- * Merge a review.
- *
- * @param info
- */
- public void mergeReview(ChangeInfo info) throws Exception {
- submitter.apply(getRevisionResource(info.project, info._number),
- 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 Change.Id(changeNumber), getBotUser());
- ChangeData changeData = changeDataFactory.create(db.get(), new Project.NameKey(project), new Change.Id(changeNumber));
- RevisionResource r = new RevisionResource(collection.parse(ctl), changeData.currentPatchSet());
- return r;
+ public RevisionResource getRevisionResource(String project, int changeNumber)
+ 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 e83f47b..864de7d 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomaticMerger.java
@@ -18,8 +18,8 @@
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.common.ChangeInfo;
import com.google.gerrit.extensions.client.ListChangesOption;
+import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.events.LifecycleListener;
import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.reviewdb.server.ReviewDb;
@@ -27,88 +27,72 @@
import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.data.AccountAttribute;
-import com.google.gerrit.server.data.ApprovalAttribute;
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.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Provider;
-
+import java.util.EnumSet;
+import java.util.List;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-import java.util.EnumSet;
-import java.util.List;
-
-/**
- * Starts at the same time as the gerrit server, and sets up our change hook
- * listener.
- */
+/** Starts at the same time as the gerrit server, and sets up our change hook listener. */
public class AutomaticMerger implements EventListener, LifecycleListener {
- private final static Logger log = LoggerFactory.getLogger(AutomaticMerger.class);
+ private static final Logger log = LoggerFactory.getLogger(AutomaticMerger.class);
- @Inject
- private GerritApi api;
+ @Inject private GerritApi api;
- @Inject
- private AtomicityHelper atomicityHelper;
+ @Inject private AtomicityHelper atomicityHelper;
- @Inject
- ChangeData.Factory changeDataFactory;
+ @Inject ChangeData.Factory changeDataFactory;
- @Inject
- private AutomergeConfig config;
+ @Inject private AutomergeConfig config;
- @Inject
- Provider<ReviewDb> db;
+ @Inject Provider<ReviewDb> db;
- @Inject
- GetRelated getRelated;
+ @Inject GetRelated getRelated;
- @Inject
- MergeUtil.Factory mergeUtilFactory;
+ @Inject MergeUtil.Factory mergeUtilFactory;
- @Inject
- Provider<PostReview> reviewer;
+ @Inject Provider<PostReview> reviewer;
- @Inject
- private ReviewUpdater reviewUpdater;
+ @Inject private ReviewUpdater reviewUpdater;
- @Inject
- Submit submitter;
+ @Inject Submit submitter;
@Override
- synchronized public void onEvent(final Event event) {
- if (event instanceof TopicChangedEvent) {
- onTopicChanged((TopicChangedEvent)event);
+ 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) {
+ Change change = Change.from(((ChangeEvent) event).change.get());
+ onNewOrChangedPatchSet(change);
+ } else if (event instanceof CommentAddedEvent) {
+ onCommentAdded((CommentAddedEvent) event);
}
- else if (event instanceof PatchSetCreatedEvent) {
- onPatchSetCreated((PatchSetCreatedEvent)event);
- }
- else if (event instanceof CommentAddedEvent) {
- onCommentAdded((CommentAddedEvent)event);
+ // it is not an else since the previous automatic submit(s) can potentially
+ // trigger others on the whole project/branch
+ if (event instanceof RefUpdatedEvent) {
+ onRefUpdatedEvent((RefUpdatedEvent) event);
}
}
- private void onTopicChanged(final TopicChangedEvent event) {
- ChangeAttribute change = event.change.get();
- if (!atomicityHelper.isAtomicReview(change)) {
- return;
- }
- processNewAtomicPatchSet(change);
- }
-
- private void onPatchSetCreated(final PatchSetCreatedEvent event) {
- ChangeAttribute change = event.change.get();
+ private void onNewOrChangedPatchSet(Change change) {
if (atomicityHelper.isAtomicReview(change)) {
processNewAtomicPatchSet(change);
}
-
try {
autoSubmitIfMergeable(change);
} catch (Exception e) {
@@ -124,23 +108,63 @@
ChangeAttribute change = newComment.change.get();
try {
checkReviewExists(change.number);
- autoSubmitIfMergeable(change);
+ autoSubmitIfMergeable(Change.from(change));
} catch (Exception e) {
log.error("An exception occured while trying to atomic merge a change.", e);
throw new RuntimeException(e);
}
}
- private void autoSubmitIfMergeable(ChangeAttribute change) throws Exception {
+ private void onRefUpdatedEvent(final RefUpdatedEvent event) {
+ String refName = event.getRefName();
+ String projectName = event.getProjectNameKey().get();
+ try {
+ api.changes()
+ .query("branch:" + refName + " project:" + projectName + " is:submittable")
+ .get()
+ .forEach(
+ submittable -> {
+ try {
+ log.info(
+ "Found another submittable change #"
+ + submittable._number
+ + " on project "
+ + projectName
+ + " during update of ref "
+ + refName
+ + ": Submitting ...");
+ autoSubmitIfMergeable(Change.from(submittable));
+ } catch (Exception e) {
+ log.error(
+ "Cannot autosubmit change "
+ + submittable._number
+ + " on project "
+ + projectName
+ + " to ref "
+ + refName,
+ e);
+ }
+ });
+ } catch (RestApiException e) {
+ log.error(
+ "Cannot query submittable changes on project " + projectName + " for ref " + refName);
+ }
+ }
+
+ private void autoSubmitIfMergeable(Change change) throws Exception {
if (atomicityHelper.isSubmittable(change.project, change.number)) {
- log.info(String.format("Change %d is submittable. Will try to merge all related changes.", change.number));
- attemptToMerge(change);
+ if (atomicityHelper.isAtomicReview(change)) {
+ attemptToMergeAtomic(change);
+ } else {
+ log.info("Submitting non-atomic change {}...", change.number);
+ atomicityHelper.mergeReview(change.project, change.number);
+ }
}
}
/**
- * Returns true if the plugin must handle this comment, i.e. if we are sure it does not come
- * from this plugin (to avoid infinite loop).
+ * Returns true if the plugin must handle this comment, i.e. if we are sure it does not come from
+ * this plugin (to avoid infinite loop).
*
* @param comment
* @return a boolean
@@ -150,60 +174,71 @@
if (!config.getBotEmail().equals(account.email)) {
return true;
}
- ApprovalAttribute[] approvals = comment.approvals.get();
- if (approvals != null) {
- for (ApprovalAttribute approval : approvals) {
- // See ReviewUpdate#setMinusOne
- if (!("Code-Review".equals(approval.type) && "-1".equals(approval.value))) {
- return true;
- }
- }
+ if (!comment.comment.contains(ReviewUpdater.commentsPrefix)) {
+ return true;
}
return false;
}
- private void attemptToMerge(ChangeAttribute change) throws Exception {
+ private void attemptToMergeAtomic(Change change) throws Exception {
final List<ChangeInfo> related = Lists.newArrayList();
if (atomicityHelper.isAtomicReview(change)) {
- related.addAll(api.changes().query("status: open AND topic: " + change.topic)
- .withOption(ListChangesOption.CURRENT_REVISION).get());
+ 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)));
}
- boolean submittable = true;
- boolean mergeable = true;
+
for (final ChangeInfo info : related) {
- if (!info.mergeable) {
- mergeable = false;
- }
if (!atomicityHelper.isSubmittable(info.project, info._number)) {
- submittable = false;
+ log.info(
+ "Change {} is not submittable because same topic change {} has not all approvals.",
+ change.number,
+ info._number);
+ return;
}
}
- if (submittable) {
- if (mergeable) {
- log.debug(String.format("Change %d is mergeable", change.number));
- for (final ChangeInfo info : related) {
- atomicityHelper.mergeReview(info);
- }
- } else {
- reviewUpdater.commentOnReview(change.project, change.number, AutomergeConfig.CANT_MERGE_COMMENT_FILE);
+ for (final ChangeInfo info : related) {
+ boolean dependsOnNonMergedCommit =
+ atomicityHelper.hasDependentReview(info.project, info._number);
+ if (!info.mergeable || dependsOnNonMergedCommit) {
+ log.info(
+ "Change {} is not mergeable because same topic change {} {}",
+ change.number,
+ info._number,
+ !info.mergeable ? "is non mergeable" : "depends on a non merged commit.");
+ PluginComment comment =
+ !info.mergeable ? config.cantMergeGitConflict : config.cantMergeDependsOnNonMerged;
+ reviewUpdater.commentOnReview(
+ change.project, change.number, String.format(comment.getContent(), info._number));
+ return;
}
}
+
+ log.info("Submitting atomic change {}...", change.number);
+ for (final ChangeInfo info : related) {
+ atomicityHelper.mergeReview(info.project, info._number);
+ }
}
- private void processNewAtomicPatchSet(ChangeAttribute change) {
+ private void processNewAtomicPatchSet(Change change) {
try {
checkReviewExists(change.number);
+ log.info(String.format("Detected atomic review on change %d.", change.number));
+ reviewUpdater.commentOnReview(
+ change.project, change.number, config.atomicReviewDetected.getContent());
if (atomicityHelper.hasDependentReview(change.project, change.number)) {
- log.info(String.format("Warn the user by setting -1 on change %d, as other atomic changes exists on the same repository.",
- change.number));
- reviewUpdater.setMinusOne(change.project, change.number, AutomergeConfig.ATOMIC_REVIEWS_SAME_REPO_FILE);
- } else {
- log.info(String.format("Detected atomic review on change %d.", change.number));
- reviewUpdater.commentOnReview(change.project, change.number, AutomergeConfig.ATOMIC_REVIEW_DETECTED_FILE);
+ log.info(
+ String.format(
+ "Warn the user on change %d, as other atomic changes exists on the same repository.",
+ change.number));
+ reviewUpdater.commentOnReview(
+ change.project, change.number, config.atomicReviewsSameRepo.getContent());
}
} catch (Exception e) {
throw new RuntimeException(e);
@@ -220,6 +255,5 @@
}
@Override
- public void stop() {
- }
+ public void stop() {}
}
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java
index bd1bbca..8eb1010 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeConfig.java
@@ -3,22 +3,21 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePaths;
import com.google.inject.Inject;
-
+import java.io.File;
import org.eclipse.jgit.lib.Config;
-import java.io.File;
-
public class AutomergeConfig {
+ public static final String AUTOMERGE_SECTION = "automerge";
+ public static final String BOT_EMAIL_KEY = "botEmail";
- public final static String ATOMIC_REVIEW_DETECTED_FILE = "atomic_review_detected.txt";
- public final static String ATOMIC_REVIEWS_SAME_REPO_FILE = "atomic_review_same_repo.txt";
- public final static String AUTOMERGE_SECTION = "automerge";
- public final static String BOT_EMAIL_KEY = "botEmail";
- public final static String CANT_MERGE_COMMENT_FILE = "cantmerge.txt";
+ public final PluginComment atomicReviewDetected;
+ public final PluginComment atomicReviewsSameRepo;
+ public final PluginComment cantMergeGitConflict;
+ public final PluginComment cantMergeDependsOnNonMerged;
- private final static String defaultBotEmail = "qabot@criteo.com";
- private final static String defaultTopicPrefix = "crossrepo/";
- public final static String TOPIC_PREFIX_KEY = "topicPrefix";
+ private static final String defaultBotEmail = "qabot@criteo.com";
+ private static final String defaultTopicPrefix = "crossrepo/";
+ public static final String TOPIC_PREFIX_KEY = "topicPrefix";
public static final String getDefaultBotEmail() {
return defaultBotEmail;
@@ -44,19 +43,32 @@
}
templatesPath = paths.etc_dir.toFile();
+
+ atomicReviewDetected =
+ new PluginComment(
+ getCommentPath("atomic_review_detected.txt"),
+ "This review is part of a cross-repository change.\n"
+ + "It will be submitted once all related reviews are submittable.");
+ atomicReviewsSameRepo =
+ new PluginComment(
+ getCommentPath("atomic_review_same_repo.txt"),
+ "This cross-repo review depends on a not merged commit that must be merged first.");
+ cantMergeGitConflict =
+ new PluginComment(
+ getCommentPath("cantmerge_git_conflict.txt"),
+ "This cross-repo review is blocked by a git conflict on change #/c/%d.");
+ cantMergeDependsOnNonMerged =
+ new PluginComment(
+ getCommentPath("cantmerge_depends_on_non_merged.txt"),
+ "This cross-repo review is blocked by a non merged commit below #/c/%d.");
}
public final String getBotEmail() {
return botEmail;
}
- /**
- * Return a comment from a file located in the gerrit etc_dir
- *
- * @return a string containing a comment.
- */
- public final String getTemplatesPath() {
- return templatesPath.getPath();
+ public final File getCommentPath(String fileName) {
+ return new File(templatesPath.getPath(), fileName);
}
public final String getTopicPrefix() {
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeModule.java b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeModule.java
index 744bb2d..dda0aa5 100644
--- a/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeModule.java
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/AutomergeModule.java
@@ -21,7 +21,7 @@
/**
* Main automerge Guice module.
*
- * Configures how all classes in the plugin are instantiated via Guice.
+ * <p>Configures how all classes in the plugin are instantiated via Guice.
*/
public class AutomergeModule extends AbstractModule {
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/Change.java b/src/main/java/com/criteo/gerrit/plugins/automerge/Change.java
new file mode 100644
index 0000000..8728bed
--- /dev/null
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/Change.java
@@ -0,0 +1,40 @@
+package com.criteo.gerrit.plugins.automerge;
+
+import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.server.data.ChangeAttribute;
+
+/**
+ * A change represented only with the fields that this plugin requires.
+ *
+ * <p>As a change can be a {@link ChangeAttribute} or a {@link ChangeInfo}, this intermediate class
+ * is needed.
+ */
+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 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.id,
+ changeAttribute.topic,
+ changeAttribute.branch);
+ }
+
+ public static Change from(ChangeInfo changeInfo) {
+ return new Change(
+ changeInfo.project, changeInfo._number, changeInfo.id, changeInfo.topic, changeInfo.branch);
+ }
+}
diff --git a/src/main/java/com/criteo/gerrit/plugins/automerge/PluginComment.java b/src/main/java/com/criteo/gerrit/plugins/automerge/PluginComment.java
new file mode 100644
index 0000000..4cacdd6
--- /dev/null
+++ b/src/main/java/com/criteo/gerrit/plugins/automerge/PluginComment.java
@@ -0,0 +1,38 @@
+package com.criteo.gerrit.plugins.automerge;
+
+import com.google.common.base.Charsets;
+import com.google.common.io.Files;
+import java.io.File;
+import java.io.IOException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** A comment pushed by the plugin to a Gerrit patchset. */
+public class PluginComment {
+
+ private static final Logger log = LoggerFactory.getLogger(PluginComment.class);
+
+ private final File templatePath;
+ private final String defaultMessage;
+
+ PluginComment(File templatePath, String defaultMessage) {
+ this.templatePath = templatePath;
+ this.defaultMessage = defaultMessage;
+ }
+
+ /**
+ * Returns the comment message, possibly with interpolation placeholders.
+ *
+ * @return a string
+ */
+ String getContent() {
+ if (templatePath.exists()) {
+ try {
+ return Files.toString(templatePath, Charsets.UTF_8);
+ } catch (final IOException exc) {
+ log.error("Not able to read " + templatePath, exc);
+ }
+ }
+ return defaultMessage;
+ }
+}
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 9c1285f..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,64 +1,32 @@
package com.criteo.gerrit.plugins.automerge;
-import com.google.common.base.Charsets;
-import com.google.common.io.Files;
import com.google.gerrit.extensions.api.changes.ReviewInput;
-import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.change.PostReview;
import com.google.gerrit.server.change.RevisionResource;
-import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Provider;
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-import java.io.File;
-import java.io.IOException;
-
public class ReviewUpdater {
- private final static Logger log = LoggerFactory.getLogger(AutomaticMerger.class);
+ /**
+ * Prefix used in front of messages pushed to Gerrit by this plugin. This prefix is used to
+ * discriminate the messages emitted by the plugin from the other messages and avoid infinite
+ * loops.
+ */
+ public static final String commentsPrefix = "[Autosubmitter] ";
- @Inject
- ChangeData.Factory changeDataFactory;
+ @Inject Provider<PostReview> reviewer;
- @Inject
- AutomergeConfig config;
+ @Inject private AtomicityHelper atomicityHelper;
- @Inject
- Provider<ReviewDb> db;
-
- @Inject
- Provider<PostReview> reviewer;
-
- @Inject
- private AtomicityHelper atomicityHelper;
-
- public void commentOnReview(String project, int number, String commentTemplate) throws Exception {
- ReviewInput comment = createComment(commentTemplate);
- applyComment(project, number, comment);
+ public void commentOnReview(String project, int number, String comment) throws Exception {
+ ReviewInput reviewInput = createComment(comment);
+ applyComment(project, number, reviewInput);
}
- public void setMinusOne(String project, int number, String commentTemplate) throws Exception {
- ReviewInput message = createComment(commentTemplate).label("Code-Review", -1);
- applyComment(project, number, message);
+ private ReviewInput createComment(String comment) {
+ return new ReviewInput().message(commentsPrefix + comment);
}
- private ReviewInput createComment(final String commentTemplate) {
- return new ReviewInput().message(getCommentFromFile(commentTemplate));
- }
-
- private String getCommentFromFile(final String filename) {
- try {
- return Files.toString(new File(config.getTemplatesPath(), filename), Charsets.UTF_8);
- } catch (final IOException exc) {
- final String errmsg =
- String.format("Cannot find %s file in gerrit etc dir. Please check your gerrit configuration", filename);
- log.error(errmsg);
- return errmsg;
- }
- }
-
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/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java b/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java
index 49535db..69e4ef9 100644
--- a/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java
+++ b/src/test/java/com/criteo/gerrit/plugins/automerge/AutomergeConfigTest.java
@@ -4,13 +4,11 @@
import static org.junit.Assert.fail;
import com.google.gerrit.server.config.SitePaths;
-
-import org.eclipse.jgit.lib.Config;
-import org.junit.Test;
-
import java.io.FileNotFoundException;
import java.io.IOException;
import java.nio.file.Paths;
+import org.eclipse.jgit.lib.Config;
+import org.junit.Test;
public class AutomergeConfigTest {
@@ -35,8 +33,13 @@
try {
final SitePaths paths = new SitePaths(Paths.get("."));
- conf.setString(AutomergeConfig.AUTOMERGE_SECTION, null, AutomergeConfig.BOT_EMAIL_KEY, "Foo@bar.com");
- conf.setString(AutomergeConfig.AUTOMERGE_SECTION, null, AutomergeConfig.TOPIC_PREFIX_KEY, "fake_topic_prefix");
+ conf.setString(
+ AutomergeConfig.AUTOMERGE_SECTION, null, AutomergeConfig.BOT_EMAIL_KEY, "Foo@bar.com");
+ conf.setString(
+ AutomergeConfig.AUTOMERGE_SECTION,
+ null,
+ AutomergeConfig.TOPIC_PREFIX_KEY,
+ "fake_topic_prefix");
final AutomergeConfig amconf = new AutomergeConfig(conf, paths);
assertEquals(amconf.getBotEmail(), "Foo@bar.com");
diff --git a/test.rb b/test.rb
index dd3523f..92ccfab 100755
--- a/test.rb
+++ b/test.rb
@@ -14,11 +14,12 @@
PROJECTS_DIR="~/"
PROJECT1="project1"
PROJECT2="project2"
+USER="admin"
class TestAutomerge < MiniTest::Test
HOST = "0.0.0.0"
PORT = 29418
- GERRIT_SSH = "ssh -p #{PORT} #{HOST}"
+ GERRIT_SSH = "ssh -l #{USER} -p #{PORT} #{HOST}"
def setup
clean_local_repo(PROJECT1)
@@ -48,8 +49,8 @@
def test_crossrepo_topic_1_repo_over_not_merged_commit
commit0 = create_review(PROJECT1, "review0 on #{PROJECT1}")
commit0b = create_review(PROJECT1, "review0b on #{PROJECT1}", "crossrepo/topic1")
- check_label(commit0b, "Code-Review", "-1")
- check_last_message_contains(commit0b, "atomic_review_same_repo.txt")
+
+ check_last_message_contains(commit0b, "This cross-repo review depends on a not merged commit")
end
def test_normal_topic_2_repos
@@ -68,11 +69,90 @@
approve_review(commit1)
check_status(commit1, 'NEW')
check_status(commit2, 'NEW')
+
approve_review(commit2)
+
check_status(commit1, 'MERGED')
check_status(commit2, 'MERGED')
end
+ def test_crossrepo_topic_2_repos_above_non_mergeable_commit
+ commit1a = create_review(PROJECT1, "review1a on #{PROJECT1}")
+ commit1b = create_review(PROJECT1, "review1b on #{PROJECT1}", "crossrepo/topic2")
+ commit2 = create_review(PROJECT2, "review2 on #{PROJECT2}", "crossrepo/topic2")
+ approve_review(commit1b)
+ check_status(commit1a, 'NEW')
+ check_status(commit1b, 'NEW')
+ check_status(commit2, 'NEW')
+
+ approve_review(commit2)
+
+ check_last_message_contains(commit2, "blocked by a non merged commit below")
+ check_status(commit1a, 'NEW')
+ check_status(commit1b, 'NEW')
+ check_status(commit2, 'NEW')
+ end
+
+ def test_crossrepo_topic_2_repos_below_not_merged_commit
+ commit1 = create_review(PROJECT1, "review1 on #{PROJECT1}", "crossrepo/topic2")
+ commit1b = create_review(PROJECT1, "review1b on #{PROJECT1}")
+ commit2 = create_review(PROJECT2, "review2 on #{PROJECT2}", "crossrepo/topic2")
+ approve_review(commit1)
+ check_status(commit1, 'NEW')
+ check_status(commit2, 'NEW')
+
+ approve_review(commit2)
+
+ check_status(commit1, 'MERGED')
+ check_status(commit2, 'MERGED')
+ end
+
+ def test_refupdatedevent_merge_upper_commit
+ commit1a = create_review(PROJECT1, "review1a on #{PROJECT1}")
+ commit1b = create_review(PROJECT1, "review1b on #{PROJECT1}")
+ approve_review(commit1b)
+ check_status(commit1a, 'NEW')
+ check_status(commit1b, 'NEW')
+
+ approve_review(commit1a)
+
+ check_status(commit1a, 'MERGED')
+ check_status(commit1b, 'MERGED')
+ end
+
+ def test_refupdatedevent_merge_upper_crossrepo
+ commit1a = create_review(PROJECT1, "review1a on #{PROJECT1}")
+ commit1b = create_review(PROJECT1, "review1b on #{PROJECT1}", "crossrepo/topic2")
+ commit2 = create_review(PROJECT2, "review2 on #{PROJECT2}", "crossrepo/topic2")
+ approve_review(commit1b)
+ approve_review(commit2)
+ check_status(commit1a, 'NEW')
+ check_status(commit1b, 'NEW')
+ check_status(commit2, 'NEW')
+
+ approve_review(commit1a)
+
+ check_status(commit1a, 'MERGED')
+ check_status(commit1b, 'MERGED')
+ check_status(commit2, 'MERGED')
+ end
+
+ def test_refupdatedevent_does_not_merge_non_mergeable_upper_crossrepo
+ commit1a = create_review(PROJECT1, "review1a on #{PROJECT1}")
+ commit1b = create_review(PROJECT1, "review1b on #{PROJECT1}", "crossrepo/topic2")
+ commit2 = create_review(PROJECT2, "review2 on #{PROJECT2}", "crossrepo/topic2")
+ approve_review(commit1b)
+ check_status(commit1a, 'NEW')
+ check_status(commit1b, 'NEW')
+ check_status(commit2, 'NEW')
+
+ approve_review(commit1a)
+
+ check_status(commit1a, 'MERGED')
+ check_status(commit1b, 'NEW')
+ check_status(commit2, 'NEW')
+ end
+
def test_two_reviews_with_same_changed_id
commit1 = create_review(PROJECT1, "review1 on #{PROJECT1}")
change_id = read_change_id(PROJECT1)
@@ -85,6 +165,16 @@
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)
@@ -101,7 +191,11 @@
reviews = gerrit_query(query)
reviews.each do |review|
review_number = review['number']
- execute("#{GERRIT_SSH} gerrit review --abandon #{review_number},1")
+ if review['status'] == 'DRAFT'
+ execute("#{GERRIT_SSH} gerrit review --delete #{review_number},1")
+ else
+ execute("#{GERRIT_SSH} gerrit review --abandon #{review_number},1")
+ end
end
end
@@ -113,14 +207,14 @@
change_id
end
- def create_review(project_name, message, topic = nil, change_id = nil)
+ def create_review(project_name, message, topic = nil, change_id = nil, draft: false)
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/for/master#{topic_suffix}"
+ "git push origin HEAD:refs/#{draft ? 'drafts' : 'for'}/master#{topic_suffix}"
].join(" && "))
commit_id = execute("cd #{project_dir(project_name)} && git rev-parse HEAD")
refute(commit_id.empty?, "missing commit-id")
@@ -131,6 +225,10 @@
execute("#{GERRIT_SSH} gerrit review --strict-labels --verified 1 --code-review 2 #{commit_id}")
end
+ def publish_draft(commit_id)
+ execute("#{GERRIT_SSH} gerrit review --publish #{commit_id}")
+ end
+
def abandon_review(commit_id)
execute("#{GERRIT_SSH} gerrit review --abandon #{commit_id}")
end