Skip to content

Commit

Permalink
Show that the overrideIf of a configured Code-Owners SR has no effect
Browse files Browse the repository at this point in the history
Whether a change is code-owner approved is checked by the (hard-coded)
code owners submit rule. This submit rule respects overrides that comply
with the label that is configured as override in the code-owners plugin
configuration.

If a Code-Owners submit requirement is manually configured in
project.config it conflicts with the (hard-coded) code owners submit
rule. If submit requirements conflict the change only gets submittable
when both submit requirements pass.

This means if the Code-Owners submit requirement has a different
overrideIf condition than what's configured in the code-owners plugin
configuration it has no effect:

1. If the overrideIf is satisfied it doesn't count as an override for
   the (hard-coded) code owners submit rule so this submit rule is still
   not passing and hence the change doesn't get submittable.

2. If the override that is configured in the code-owners plugin
   configuration is satisfied the (hard-coded) code owners submit rule
   is passing. If the Code-Owners submit requirement uses
   "has:approval_code-owners" as submittableIf condition it is passing
   now too since "has:approval_code-owners" just means to check if the
   (hard-coded) code owners submit rule passes.

This means the overrideIf condition of a configured Code-Owners submit
requirement can neither replace nor add to the override condition that
is configured in the code-owners plugin configuration.

This also means defining a Code-Owners submit requirement that has an
overrideIf condition that is different from the override condition that
is configured in the code-owners plugin configuration has no effect and
is only confusing as the overrideIf condition from the Code-Owners
submit requirement is shown in the UI but doesn't work.

Change-Id: I5bfe4a45ad4e2ec67fae5ec3681012537e7b6a63
Signed-off-by: Edwin Kempin <ekempin@google.com>
Reviewed-on: https://gerrit-review.googlesource.com/c/plugins/code-owners/+/412560
Reviewed-by: Patrick Hiesel <hiesel@google.com>
Tested-by: Zuul <zuul-63@gerritcodereview-ci.iam.gserviceaccount.com>
  • Loading branch information
EdwinKempin authored and phiesel committed Mar 11, 2024
1 parent 08a2164 commit 004aceb
Showing 1 changed file with 49 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,55 @@ public void submitRuleIsInvokedTwiceWhenACodeOwnersSubmitRequirementIsConfigured
.isEqualTo(2);
}

@Test
@GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
public void cannotDefineOverrideWithCodeOwnersSubmitRequirement() throws Exception {
// Create the Owners-Override label that is configured as override for the Code-Owners submit
// requirement.
createOwnersOverrideLabel();

// Configure a Code-Owners submit requirement with Other-Override+1 as override (note this is a
// different label than the one that is configured as override in the code-owners plugin
// configuration).
createOwnersOverrideLabel("Other-Override");
SubmitRequirementInput input = new SubmitRequirementInput();
input.name = "Code-Owners";
input.submittabilityExpression = "has:approval_code-owners";
input.overrideExpression = "label:Other-Override+1";
gApi.projects().name(project.get()).submitRequirement("Code-Owners").create(input);

// Create a change and approve the Code-Review submit requirement.
PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
String changeId = r.getChangeId();
approve(changeId);

// Check that the change is not submittable since the Code-Owners submit rule is not satisfied
// yet.
assertThat(gApi.changes().id(changeId).get().submittable).isFalse();

// Apply the override that is configured in the Code-Owners submit requirement and see that it
// has no effect (since the hard-coded code-owners submit rule is not overridden by it).
gApi.changes().id(changeId).current().review(new ReviewInput().label("Other-Override", 1));
assertThat(gApi.changes().id(changeId).get().submittable).isFalse();

// Apply the override that is configured in the code-owners plugin configuration and see that it
// makes the change submittable.
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
assertThat(gApi.changes().id(changeId).get().submittable).isTrue();

// Remove the override that is configured in the Code-Owners submit requirement and see that it
// has no effect and the change is still submittable (this is because the code-owners submit
// rule is fulfilled now and hence the submittableIf condition of the Code-Owners submit
// requirement passes).
gApi.changes().id(changeId).current().review(new ReviewInput().label("Other-Override", 0));
assertThat(gApi.changes().id(changeId).get().submittable).isTrue();

// Removing the override that is configured in the code-owners plugin configuration makes the
// change again not submittable.
gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 0));
assertThat(gApi.changes().id(changeId).get().submittable).isFalse();
}

@Test
public void submitRuleIsNotInvokedWhenQueryingChange() throws Exception {
PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
Expand Down

0 comments on commit 004aceb

Please # to comment.