opendev/gerrit
Code Issues Proposed changes

Change the Presubmit handling with multiple SubmitRecords

Browse Source 
With the future SubmitRule interface, several plugins will be able to
provide custom SubmitRecords, this means we have to deal with multiple
different SubmitRecords at once: Prolog may generate one, an other
plugin may generate an other.
Before this change, one valid SubmitRecord was "enough" to enable the
submit option. This leads to several issues if we allow plugins to hook
into the system.

Let's consider the situation where two plugins providing a presubmit
hook are enabled. The first one checks that a reviewer (determined by
the OWNERS file) has validated the changes, the second one checks that
the build was successful.
With the former system, the change could be submitted: at least one
plugin acccepted it.

The proposed change is to require at least one OK status, and reject
when we have one or more "Not ready" statuses.
If we consider our example again, the behavior seems fair:
[   Plugin A ,  Plugin B  ,   Result  ]
[    OK      ,    OK      ,     OK    ]
[    OK      , NOT_READY  , NOT_READY ]
[ NOT_READY  ,    OK      , NOT_READY ]

We also handle the special case where no plugins are enabled: in this
situation, no plugin can "validate" the changes. We allow submission
anyway, so it is less confusing for the user.
When this occurs, the permission system is the mechanism preventing
users from submitting unwanted changes.

Before this commit, Prolog used to return multiple `SubmitRecord`s.
The behavior was quite counter-intuitive: as soon as an OK record
is found, return the current list - which might contain some not_ready
records but not necessarily all of them.
In order to be compatible and retro-compatible, we set all the
not_ready records statuses to OK. This way, we simulate the old behavior
by keeping the same SubmitRecords and Labels.

This is an edge case and a non retro-compatible could simplify this
behavior without breaking too many configurations.

Change-Id: I11ba85aaa10e60050c02fd3a6051e04ce81d7be7
This commit is contained in:
Maxime Guerreiro
2018-02-28 14:37:25 +01:00
parent ead26b44df
commit 8f62fca3e1
6 changed files with 39 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
javatests/com/google/gerrit/common/data/SubmitRecordTest.java
View File

@@ -34,19 +34,26 @@ public class SubmitRecordTest {
}
@Test
public void findOkRecord() {
public void okIfAllOkay() {
Collection<SubmitRecord> submitRecords = new ArrayList<>();
submitRecords.add(NOT_READY_RECORD);
submitRecords.add(OK_RECORD);
assertThat(SubmitRecord.findOkRecord(submitRecords).isPresent()).isTrue();
assertThat(SubmitRecord.allRecordsOK(submitRecords)).isTrue();
}
@Test
public void okWhenEmpty() {
Collection<SubmitRecord> submitRecords = new ArrayList<>();
assertThat(SubmitRecord.allRecordsOK(submitRecords)).isTrue();
}
@Test
public void emptyResultIfInvalid() {
Collection<SubmitRecord> submitRecords = new ArrayList<>();
submitRecords.add(NOT_READY_RECORD);
submitRecords.add(OK_RECORD);
assertThat(SubmitRecord.findOkRecord(submitRecords).isPresent()).isFalse();
assertThat(SubmitRecord.allRecordsOK(submitRecords)).isFalse();
}
}