Optionally skip submit rules evaluation on closed changes
Adds a `change.skipCurrentRulesEvaluationOnClosedChanges` configuration
option to control submit rule evaluation on closed (merged and abandoned)
changes.
The default value for this option is `true` to keep the current
behaviour of Gerrit. This means that each time closed change is viewed,
submit rules will be revaluated. This may result in labels being added
or removed depending on the most recent project configuration. This also
means that labels on the closed changes may not represent the stale of
them when it was submitted.
When `change.skipCurrentRulesEvaluationOnClosedChanges` is set to `true`
Gerrit will show the exact state of review labels the change was merged
with.
Bug: Issue 298084094
Release-Notes: Allow disable rules evaluation on closed changes
Change-Id: I2499271b84edceba0c34c3347dea2f8508eca00f
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 4b89c6b..524b171 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -1662,6 +1662,21 @@
+
Default is 5 minutes.
+[[change.skipCurrentRulesEvaluationOnClosedChanges]]
++
+If false, Gerrit will always take latest project configuration to
+compute submit labels. This means that, closed changes (either merged
+or abandoned) will be evaluated against the latest configuration which
+may produce different results. Especially for merged changes, they may
+look like they didn't meet the submit requirements.
++
+When true, evaluation will be skipped and Gerrit will show the
+exact status of submit labels when change was submitted. Post-review
+votes will only be allowed on labels that were configured when change
+was closed.
++
+Default it false.
+
[[changeCleanup]]
=== Section changeCleanup
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index 78a162b..fe5966c 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -60,6 +60,7 @@
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.GitReceivePackGroups;
import com.google.gerrit.server.config.GitUploadPackGroups;
+import com.google.gerrit.server.config.SkipCurrentRulesEvaluationOnClosedChangesModule;
import com.google.gerrit.server.config.SysExecutorModule;
import com.google.gerrit.server.extensions.events.EventUtil;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
@@ -209,6 +210,7 @@
modules.add(new PrologModule(getConfig()));
modules.add(new DefaultSubmitRuleModule());
modules.add(new IgnoreSelfApprovalRuleModule());
+ modules.add(new SkipCurrentRulesEvaluationOnClosedChangesModule());
// Global submit requirements
DynamicSet.setOf(binder(), SubmitRequirement.class);
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 6449155..d6baa7f 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -290,6 +290,7 @@
install(ThreadLocalRequestContext.module());
install(new ApprovalModule());
install(new MailSoySauceModule());
+ install(new SkipCurrentRulesEvaluationOnClosedChangesModule());
factory(CapabilityCollection.Factory.class);
factory(ChangeData.AssistedFactory.class);
diff --git a/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChanges.java b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChanges.java
new file mode 100644
index 0000000..b812710
--- /dev/null
+++ b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChanges.java
@@ -0,0 +1,25 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.config;
+
+import static java.lang.annotation.RetentionPolicy.RUNTIME;
+
+import com.google.inject.BindingAnnotation;
+import java.lang.annotation.Retention;
+
+/** Marker on a {@link Boolean} holding the evaluation of current Prolog rules on closed changes. */
+@Retention(RUNTIME)
+@BindingAnnotation
+public @interface SkipCurrentRulesEvaluationOnClosedChanges {}
diff --git a/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesModule.java b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesModule.java
new file mode 100644
index 0000000..9eaae02
--- /dev/null
+++ b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesModule.java
@@ -0,0 +1,27 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.config;
+
+import com.google.inject.AbstractModule;
+
+public class SkipCurrentRulesEvaluationOnClosedChangesModule extends AbstractModule {
+
+ @Override
+ protected void configure() {
+ bind(Boolean.class)
+ .annotatedWith(SkipCurrentRulesEvaluationOnClosedChanges.class)
+ .toProvider(SkipCurrentRulesEvaluationOnClosedChangesProvider.class);
+ }
+}
diff --git a/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesProvider.java b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesProvider.java
new file mode 100644
index 0000000..bc25a33
--- /dev/null
+++ b/java/com/google/gerrit/server/config/SkipCurrentRulesEvaluationOnClosedChangesProvider.java
@@ -0,0 +1,35 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server.config;
+
+import com.google.inject.Inject;
+import com.google.inject.Provider;
+import com.google.inject.Singleton;
+import org.eclipse.jgit.lib.Config;
+
+@Singleton
+public class SkipCurrentRulesEvaluationOnClosedChangesProvider implements Provider<Boolean> {
+ private final Boolean value;
+
+ @Inject
+ SkipCurrentRulesEvaluationOnClosedChangesProvider(@GerritServerConfig Config config) {
+ value = config.getBoolean("change", null, "skipCurrentRulesEvaluationOnClosedChanges", false);
+ }
+
+ @Override
+ public Boolean get() {
+ return value;
+ }
+}
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index cc08254..3f5f63b 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -75,6 +75,7 @@
import com.google.gerrit.server.change.MergeabilityCache;
import com.google.gerrit.server.change.PureRevert;
import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.config.SkipCurrentRulesEvaluationOnClosedChanges;
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
@@ -284,7 +285,7 @@
ChangeData cd =
new ChangeData(
null, null, null, null, null, null, null, null, null, null, null, null, null, null,
- null, null, null, project, id, null, null);
+ null, null, null, false, project, id, null, null);
cd.currentPatchSet =
PatchSet.builder()
.id(PatchSet.id(id, currentPatchSetId))
@@ -313,6 +314,7 @@
private final SubmitRequirementsEvaluator submitRequirementsEvaluator;
private final SubmitRequirementsUtil submitRequirementsUtil;
private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory;
+ private final boolean skipCurrentRulesEvaluationOnClosedChanges;
// Required assisted injected fields.
private final Project.NameKey project;
@@ -400,6 +402,7 @@
SubmitRequirementsEvaluator submitRequirementsEvaluator,
SubmitRequirementsUtil submitRequirementsUtil,
SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory,
+ @SkipCurrentRulesEvaluationOnClosedChanges Boolean skipCurrentRulesEvaluationOnClosedChange,
@Assisted Project.NameKey project,
@Assisted Change.Id id,
@Assisted @Nullable Change change,
@@ -421,6 +424,7 @@
this.submitRequirementsEvaluator = submitRequirementsEvaluator;
this.submitRequirementsUtil = submitRequirementsUtil;
this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory;
+ this.skipCurrentRulesEvaluationOnClosedChanges = skipCurrentRulesEvaluationOnClosedChange;
this.project = project;
this.legacyId = id;
@@ -1083,6 +1087,9 @@
project(), getId().get());
return Collections.emptyList();
}
+ if (skipCurrentRulesEvaluationOnClosedChanges && change().isClosed()) {
+ return notes().getSubmitRecords();
+ }
records = submitRuleEvaluatorFactory.create(options).evaluate(this);
submitRecords.put(options, records);
if (!change().isClosed() && submitRecords.size() == 1) {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index c5f0d23..86691c6 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -31,6 +31,7 @@
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.labelPermissionKey;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.permissionKey;
import static com.google.gerrit.entities.RefNames.changeMetaRef;
+import static com.google.gerrit.extensions.client.ChangeStatus.ABANDONED;
import static com.google.gerrit.extensions.client.ChangeStatus.MERGED;
import static com.google.gerrit.extensions.client.ListChangesOption.ALL_REVISIONS;
import static com.google.gerrit.extensions.client.ListChangesOption.CHANGE_ACTIONS;
@@ -3806,6 +3807,74 @@
}
@Test
+ @GerritConfig(name = "change.skipCurrentRulesEvaluationOnClosedChanges", value = "true")
+ public void checkLabelsNotUpdatedForMergedChange() throws Exception {
+ PushOneCommit.Result r = createChange();
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).review(ReviewInput.approve());
+ gApi.changes().id(r.getChangeId()).revision(r.getCommit().name()).submit();
+
+ ChangeInfo change = gApi.changes().id(r.getChangeId()).get();
+ assertThat(change.status).isEqualTo(MERGED);
+ assertThat(change.submissionId).isNotNull();
+ assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
+ assertThat(change.permittedLabels.keySet()).containsExactly(LabelId.CODE_REVIEW);
+ assertPermitted(change, LabelId.CODE_REVIEW, 2);
+
+ LabelType verified = TestLabels.verified();
+ AccountGroup.UUID registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
+ String heads = RefNames.REFS_HEADS + "*";
+
+ // add new label and assert that it's returned for existing changes
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig().upsertLabelType(verified);
+ u.save();
+ }
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1))
+ .update();
+
+ change = gApi.changes().id(r.getChangeId()).get();
+ assertThat(change.labels.keySet()).containsExactly(LabelId.CODE_REVIEW);
+ assertThat(change.permittedLabels.keySet())
+ .containsExactly(LabelId.CODE_REVIEW, LabelId.VERIFIED);
+ assertPermitted(change, LabelId.CODE_REVIEW, 2);
+ }
+
+ @Test
+ @GerritConfig(name = "change.skipCurrentRulesEvaluationOnClosedChanges", value = "true")
+ public void checkLabelsNotUpdatedForAbandonedChange() throws Exception {
+ PushOneCommit.Result r = createChange();
+ gApi.changes().id(r.getChangeId()).abandon();
+
+ ChangeInfo change = gApi.changes().id(r.getChangeId()).get();
+ assertThat(change.status).isEqualTo(ABANDONED);
+ assertThat(change.labels.keySet()).isEmpty();
+ assertThat(change.submitRecords).isEmpty();
+
+ LabelType verified = TestLabels.verified();
+ AccountGroup.UUID registeredUsers = systemGroupBackend.getGroup(REGISTERED_USERS).getUUID();
+ String heads = RefNames.REFS_HEADS + "*";
+
+ // add new label and assert that it's returned for existing changes
+ try (ProjectConfigUpdate u = updateProject(project)) {
+ u.getConfig().upsertLabelType(verified);
+ u.save();
+ }
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel(verified.getName()).ref(heads).group(registeredUsers).range(-1, 1))
+ .update();
+
+ change = gApi.changes().id(r.getChangeId()).get();
+ assertThat(change.labels.keySet()).isEmpty();
+ assertThat(change.permittedLabels.keySet()).isEmpty();
+ assertThat(change.submitRecords).isEmpty();
+ }
+
+ @Test
public void notifyConfigForDirectoryTriggersEmail() throws Exception {
// Configure notifications on project level.
RevCommit oldHead = projectOperations.project(project).getHead("master");
