Add auto-owners-approved to OWNERS Add an `auto-owners-approved` setting to OWNERS files to control when owner approvals can be copied over to the next patchset. The goal is to avoid unnecessary re-approval when an owner updates only files they already own on their own change. In that case, loosing the existing vote adds little review value since they will need to re-approve it anyway. At the same time, this setting lets repositories turn that shortcut off in sensitive paths where every update should get a fresh owner review. A vote is now copied over only when the approver is also the change owner and patch set uploader, all modified files are owned by that user, and none of those files disable auto approval. Bug: Issue 489671151 Change-Id: Icd3d7a95234db6192932253153f178525f3ef268
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java index 54c624e..497d04c 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/ConfigurationParser.java
@@ -21,6 +21,7 @@ import com.fasterxml.jackson.databind.ObjectMapper; import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; import com.google.gerrit.entities.Account.Id; +import com.google.gerrit.extensions.client.InheritableBoolean; import java.io.IOException; import java.util.Optional; import java.util.Set; @@ -47,6 +48,14 @@ Optional.ofNullable(jsonNode.get("label")) .map(JsonNode::asText) .flatMap(LabelDefinition::parse)); + Optional<InheritableBoolean> autoOwnersApproved = + Optional.ofNullable(jsonNode.get("auto-owners-approved")) + .map(JsonNode::asText) + .map(String::toUpperCase) + .map(InheritableBoolean::valueOf); + + autoOwnersApproved.ifPresent(ret::setAutoOwnersApproved); + addClassicMatcher(jsonNode, ret); addMatchers(jsonNode, ret); return ret;
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java index 7e7e705..1fce900 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersConfig.java
@@ -16,8 +16,11 @@ package com.googlesource.gerrit.owners.common; +import static com.google.gerrit.extensions.client.InheritableBoolean.INHERIT; + import com.google.common.collect.Maps; import com.google.common.collect.Sets; +import com.google.gerrit.extensions.client.InheritableBoolean; import java.util.Map; import java.util.Optional; import java.util.Set; @@ -40,6 +43,9 @@ /** Label that is required for submit. CodeReview if nothing is specified. */ private Optional<LabelDefinition> label = Optional.empty(); + /** Ability to enable or disable the owners auto approval, when configured */ + private InheritableBoolean autoOwnersApproved = INHERIT; + @Override public String toString() { return "OwnersConfig [inherited=" @@ -50,6 +56,8 @@ + matchers + ", label=" + label + + ", autoOwnersApproved=" + + autoOwnersApproved + "]"; } @@ -92,4 +100,12 @@ public Optional<LabelDefinition> getLabel() { return label; } + + public InheritableBoolean getAutoOwnersApproved() { + return autoOwnersApproved; + } + + public void setAutoOwnersApproved(InheritableBoolean autoOwnersApproved) { + this.autoOwnersApproved = autoOwnersApproved; + } }
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java index 760b8a6..425e650 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/OwnersMap.java
@@ -31,6 +31,7 @@ private Map<String, Set<Account.Id>> fileOwners = Maps.newHashMap(); private Map<String, Set<Account.Id>> fileReviewers = Maps.newHashMap(); private Map<String, Set<String>> fileGroupOwners = Maps.newHashMap(); + private Set<String> fileOwnersBannedAutoApproval = Sets.newHashSet(); private Optional<LabelDefinition> label = Optional.empty(); @Override @@ -86,6 +87,10 @@ return fileGroupOwners; } + public Set<String> getFileOwnersBannedAutoApproval() { + return fileOwnersBannedAutoApproval; + } + public void addFileOwners(String file, Set<Id> owners) { if (owners.isEmpty()) { return; @@ -122,6 +127,10 @@ fileGroupOwners.computeIfAbsent(file, (f) -> Sets.newHashSet()).addAll(groupOwners); } + public void banFileFromOwnersAutoApproval(String file) { + fileOwnersBannedAutoApproval.add(file); + } + public Optional<LabelDefinition> getLabel() { return label; }
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java index 78d4967..161e4d9 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwners.java
@@ -90,6 +90,8 @@ private final Map<String, Set<String>> fileGroupOwners; + private final Set<String> fileOwnersBannedAutoApproval; + private final boolean expandGroups; private final Optional<LabelDefinition> label; @@ -175,6 +177,7 @@ matchers = map.getMatchers(); fileOwners = map.getFileOwners(); fileGroupOwners = map.getFileGroupOwners(); + fileOwnersBannedAutoApproval = map.getFileOwnersBannedAutoApproval(); label = globalLabel.or(map::getLabel); } @@ -208,6 +211,10 @@ return fileGroupOwners; } + public Set<String> getFileOwnersBannedAutoApproval() { + return fileOwnersBannedAutoApproval; + } + public boolean expandGroups() { return expandGroups; } @@ -257,6 +264,9 @@ ownersMap.addFileOwners(path, currentEntry.getOwners()); ownersMap.addFileReviewers(path, currentEntry.getReviewers()); ownersMap.addFileGroupOwners(path, currentEntry.getGroupOwners()); + if (!currentEntry.isAutoOwnersApproved()) { + ownersMap.banFileFromOwnersAutoApproval(path); + } // Only add the path to the OWNERS file to reduce the number of // entries in the result @@ -337,6 +347,7 @@ Optional.empty(), Collections.emptySet(), Collections.emptySet(), + Optional.empty(), Collections.emptySet(), Collections.emptySet()))); } @@ -448,6 +459,7 @@ label, owners, reviewers, + Optional.of(pathFallbackEntry.isAutoOwnersApproved()), inheritedMatchers, groupOwners); }) @@ -478,6 +490,9 @@ if (currentEntry.getLabel().isEmpty()) { currentEntry.setLabel(projectEntry.getLabel()); } + if (!currentEntry.hasExplicitAutoOwnersApproved()) { + currentEntry.setAutoOwnersApproved(projectEntry.isAutoOwnersApproved()); + } } /**
diff --git a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java index 8633461..7b42709 100644 --- a/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java +++ b/owners-common/src/main/java/com/googlesource/gerrit/owners/common/PathOwnersEntry.java
@@ -20,6 +20,7 @@ import com.google.common.collect.Maps; import com.google.common.collect.Sets; import com.google.gerrit.entities.Account; +import com.google.gerrit.extensions.client.InheritableBoolean; import java.util.Collection; import java.util.Map; import java.util.Optional; @@ -43,9 +44,11 @@ Optional<LabelDefinition> inheritedLabel, Set<Account.Id> inheritedOwners, Set<Account.Id> inheritedReviewers, + Optional<Boolean> autoOwnersApproved, Collection<Matcher> inheritedMatchers, Set<String> inheritedGroupOwners) { super(config.isInherited()); + this.explicitAutoOwnersApproved = config.getAutoOwnersApproved() != InheritableBoolean.INHERIT; this.ownersPath = path; this.owners = config.getOwners().stream() @@ -65,12 +68,19 @@ this.owners.addAll(inheritedOwners); this.groupOwners.addAll(inheritedGroupOwners); this.reviewers.addAll(inheritedReviewers); + if (config.getAutoOwnersApproved() == InheritableBoolean.INHERIT) { + autoOwnersApproved.ifPresent(this::setAutoOwnersApproved); + } else { + setAutoOwnersApproved(config.getAutoOwnersApproved() == InheritableBoolean.TRUE); + } for (Matcher matcher : inheritedMatchers) { addMatcher(matcher); } this.label = config.getLabel().or(() -> inheritedLabel); } else { this.label = config.getLabel(); + // Default to true unless the OWNERS file explicitly sets it to false. + this.setAutoOwnersApproved(config.getAutoOwnersApproved() != InheritableBoolean.FALSE); } } @@ -119,6 +129,8 @@ protected String ownersPath; protected Map<String, Matcher> matchers = Maps.newHashMap(); protected Set<String> groupOwners = Sets.newHashSet(); + protected boolean autoOwnersApproved = true; + protected boolean explicitAutoOwnersApproved; protected ReadOnlyPathOwnersEntry(boolean inherited) { this.inherited = inherited; @@ -153,6 +165,18 @@ return label; } + public boolean isAutoOwnersApproved() { + return autoOwnersApproved; + } + + public boolean hasExplicitAutoOwnersApproved() { + return explicitAutoOwnersApproved; + } + + public void setAutoOwnersApproved(boolean autoOwnersApproved) { + this.autoOwnersApproved = autoOwnersApproved; + } + public boolean hasMatcher(String path) { return this.matchers.containsKey(path); }
diff --git a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java index 4f11b86..ca09d57 100644 --- a/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java +++ b/owners-common/src/test/java/com/googlesource/gerrit/owners/common/PathOwnersTest.java
@@ -493,6 +493,138 @@ assertThat(cacheMock.hit).isEqualTo(expectedCacheCalls); } + @Test + public void testAutoOwnersMisconfigured() throws Exception { + expectConfig("OWNERS", "inherited: true\nauto-owners-approved: \"some wrong value\""); + + replayAll(); + + PathOwners owners = + new PathOwners( + accounts, + repositoryManager, + repository, + emptyList(), + branch, + Set.of("file.txt"), + EXPAND_GROUPS, + "foo", + CACHE_MOCK, + Optional.empty()); + + assertThat(owners.getFileOwnersBannedAutoApproval()).isEmpty(); + assertThat(owners.getFileOwners()).isEmpty(); + } + + @Test + public void testAutoOwnersApprovedInheritedFromRoot() throws Exception { + expectConfig( + "OWNERS", + "inherited: true\nauto-owners-approved: false\nowners:\n- " + USER_A_EMAIL_COM + "\n"); + expectConfig("dir/OWNERS", "inherited: true\nowners:\n- " + USER_B_EMAIL_COM + "\n"); + + replayAll(); + + PathOwners owners = + new PathOwners( + accounts, + repositoryManager, + repository, + emptyList(), + branch, + Set.of("dir/file.txt"), + EXPAND_GROUPS, + "foo", + CACHE_MOCK, + Optional.empty()); + + assertThat(owners.getFileOwnersBannedAutoApproval()).contains("dir/file.txt"); + } + + @Test + public void testAutoOwnersApprovedDefaultsWhenInheritanceStopped() throws Exception { + expectConfig( + "OWNERS", + "inherited: true\nauto-owners-approved: false\nowners:\n- " + USER_A_EMAIL_COM + "\n"); + expectConfig("dir/OWNERS", "inherited: false\nowners:\n- " + USER_B_EMAIL_COM + "\n"); + + replayAll(); + + PathOwners owners = + new PathOwners( + accounts, + repositoryManager, + repository, + emptyList(), + branch, + Set.of("dir/file.txt"), + EXPAND_GROUPS, + "foo", + CACHE_MOCK, + Optional.empty()); + + assertThat(owners.getFileOwnersBannedAutoApproval()).isEmpty(); + } + + @Test + public void testAutoOwnersApprovedInheritedFromParentProjectOwners() throws Exception { + expectConfig("OWNERS", "master", createConfig(true, owners())); + expectConfig("OWNERS", RefNames.REFS_CONFIG, repository, createConfig(true, owners())); + expectConfig( + "OWNERS", + RefNames.REFS_CONFIG, + parentRepository1, + "inherited: true\nauto-owners-approved: false\nowners:\n- " + USER_A_EMAIL_COM + "\n"); + + mockParentRepository(parentRepository1NameKey, parentRepository1); + replayAll(); + + PathOwners owners = + new PathOwners( + accounts, + repositoryManager, + repository, + Arrays.asList(parentRepository1NameKey), + branch, + Set.of("file.txt"), + EXPAND_GROUPS, + "foo", + CACHE_MOCK, + Optional.empty()); + + assertThat(owners.getFileOwnersBannedAutoApproval()).contains("file.txt"); + } + + @Test + public void testExplicitAutoOwnersApprovedInRootOverridesProjectOwners() throws Exception { + expectConfig( + "OWNERS", + "master", + "inherited: true\nauto-owners-approved: false\nowners:\n- " + USER_A_EMAIL_COM + "\n"); + expectConfig( + "OWNERS", + RefNames.REFS_CONFIG, + repository, + "inherited: true\nauto-owners-approved: true\nowners:\n- " + USER_B_EMAIL_COM + "\n"); + + replayAll(); + + PathOwners owners = + new PathOwners( + accounts, + repositoryManager, + repository, + emptyList(), + branch, + Set.of("file.txt"), + EXPAND_GROUPS, + "foo", + CACHE_MOCK, + Optional.empty()); + + assertThat(owners.getFileOwnersBannedAutoApproval()).contains("file.txt"); + } + private void mockOwners(String... owners) throws IOException { expectNoConfig("OWNERS"); expectConfig(CLASSIC_OWNERS, createConfig(false, owners(owners)));
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java b/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java index 04d0da8..e74e402 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/AlreadyApprovedByPredicate.java
@@ -76,6 +76,8 @@ Project.NameKey project = ctx.changeData().project(); PatchSet targetPatchSet = ctx.targetPatchSet(); PatchSet sourcePatchSet = ctx.changeNotes().getPatchSets().get(ctx.sourcePatchSetId()); + Account.Id changeOwner = ctx.changeNotes().getChange().getOwner(); + Account.Id uploader = targetPatchSet.uploader(); checkState( predicateField == UserInPredicate.Field.APPROVER, @@ -113,12 +115,20 @@ .map(Optional::get) .collect(Collectors.toSet()); + String branch = ctx.changeData().branchOrThrow().branch(); Set<String> filesOwnedByApprover = - getFilesOwners.filterFilesOwnedBy( - currentApprover, - allFilePathsInDiff, - project, - ctx.changeData().branchOrThrow().branch()); + getFilesOwners.filterFilesOwnedBy(currentApprover, allFilePathsInDiff, project, branch); + + if (isApproverAlsoOwnerAndUploader(currentApprover, changeOwner, uploader) + && allTouchedFilesAreOwned(filesOwnedByApprover, allFilePathsInDiff) + && getFilesOwners.noOwnedFileIsBannedFromAutoApproval( + filesOwnedByApprover, project, branch)) { + logger.atFinest().log( + "Approver '%s' is change owner and uploader. only owned files have been modified and" + + " none of them has auto-owners-approved=false. Label WILL be copied.", + currentApprover); + return true; + } if (!filesOwnedByApprover.isEmpty()) { logger.atFinest().log( @@ -209,6 +219,17 @@ return !d.oldPath().equals(d.newPath()); } + private static boolean isApproverAlsoOwnerAndUploader( + Account.Id currentApprover, Account.Id changeOwner, Account.Id uploader) { + return currentApprover.equals(changeOwner) && currentApprover.equals(uploader); + } + + private static boolean allTouchedFilesAreOwned( + Set<String> filesOwnedByApprover, Set<String> allFilePathsInDiff) { + return !filesOwnedByApprover.isEmpty() + && filesOwnedByApprover.size() == allFilePathsInDiff.size(); + } + private int getParentNum(ObjectId objectId, RevWalk revWalk) { try { RevCommit commit = revWalk.parseCommit(objectId);
diff --git a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java index c395179..af62387 100644 --- a/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java +++ b/owners/src/main/java/com/googlesource/gerrit/owners/restapi/GetFilesOwners.java
@@ -108,6 +108,15 @@ .collect(Collectors.toSet()); } + public boolean noOwnedFileIsBannedFromAutoApproval( + Set<String> ownedPaths, Project.NameKey project, String branch) + throws IOException, InvalidOwnersFileException { + PathOwners owners = getPathOwners(project, branch, ownedPaths); + Set<String> filesBannedFromAutoOwnersApproval = owners.getFileOwnersBannedAutoApproval(); + + return ownedPaths.stream().noneMatch(filesBannedFromAutoOwnersApproval::contains); + } + @Override public Response<FilesOwnersResponse> apply(RevisionResource revision) throws AuthException, BadRequestException, ResourceConflictException, Exception {
diff --git a/owners/src/main/resources/Documentation/config.md b/owners/src/main/resources/Documentation/config.md index 4b94ac3..a536116 100644 --- a/owners/src/main/resources/Documentation/config.md +++ b/owners/src/main/resources/Documentation/config.md
@@ -18,8 +18,9 @@ ``` owners.expandGroups -: Expand owners and groups into account ids. If set to `false` all owners are left untouched, apart from e-mail - addresses which have the domain dropped. Defaults to `true`. +: Expand owners and groups into account ids. If set to `false` all owners are left untouched, +apart from e-mail +addresses which have the domain dropped. Defaults to `true`. Example: @@ -30,8 +31,8 @@ owners.label : Global override for the label and score, separated by a comma, to use by - the owners of changes for approving them. When defined, it overrides any - other label definition set by the OWNERS at any level in any project. +the owners of changes for approving them. When defined, it overrides any +other label definition set by the OWNERS at any level in any project. > **NOTE:** Compulsory when the selected label's function is NoBlock/NoOp. @@ -44,10 +45,10 @@ <a name="owners.enableSubmitRequirement">owners.enableSubmitRequirement</a> : If set to `true` the approvals are evaluated through the owners plugin - default submit requirement, named "Code-Review-from-Owners", without a need of - prolog predicate being added to a project or submit requirement configured - in the `project.config` as it is automatically applied to all projects. - Defaults to `false`. +default submit requirement, named "Code-Review-from-Owners", without a need of +prolog predicate being added to a project or submit requirement configured +in the `project.config` as it is automatically applied to all projects. +Defaults to `false`. Example: @@ -80,19 +81,18 @@ > submittableIf = has:approval_owners > ``` - cache."owners.path_owners_entries".memoryLimit : The cache is used to hold the parsed version of `OWNERS` files in the - repository so that when submit rules are calculated (either through prolog - or through submit requirements) it is not read over and over again. The - cache entry gets invalidated when `OWNERS` file branch is updated. - By default it follows default Gerrit's cache memory limit but it makes - sense to adjust it as a function of number of project that use the `owners` - plugin multiplied by average number of active branches (plus 1 for the - refs/meta/config) and average number of directories (as directory hierarchy - back to root is checked for the `OWNERS` file existence). - _Note that in opposite to the previous settings the modification needs to be - performed in the `$GERRIT_SITE/etc/gerrit.config` file._ +repository so that when submit rules are calculated (either through prolog +or through submit requirements) it is not read over and over again. The +cache entry gets invalidated when `OWNERS` file branch is updated. +By default it follows default Gerrit's cache memory limit but it makes +sense to adjust it as a function of number of project that use the `owners` +plugin multiplied by average number of active branches (plus 1 for the +refs/meta/config) and average number of directories (as directory hierarchy +back to root is checked for the `OWNERS` file existence). +_Note that in opposite to the previous settings the modification needs to be +performed in the `$GERRIT_SITE/etc/gerrit.config` file._ Example @@ -112,20 +112,20 @@ inherited: true label: Code-Review, 1 owners: -- some.email@example.com -- User Name -- group/Group of Users + - some.email@example.com + - User Name + - group/Group of Users matchers: -- suffix: .java - owners: - [...] -- regex: .*/README.* - owners: - [...] -- partial_regex: example - owners: - [...] -- exact: path/to/file.txt + - suffix: .java + owners: + [ ... ] + - regex: .*/README.* + owners: + [ ... ] + - partial_regex: example + owners: + [ ... ] + - exact: path/to/file.txt [...] ``` @@ -190,9 +190,9 @@ ```yaml matchers: -- suffix: .config - owners: - - Configuration Managers + - suffix: .config + owners: + - Configuration Managers ``` Global refs/meta/config OWNERS configuration is inherited only when the OWNERS file @@ -204,6 +204,43 @@ If the global project OWNERS has the 'inherited: true', it will check for a global project OWNERS in all parent projects up to All-Projects. +## auto-owners-approved + +The `auto-owners-approved` field controls a specific exception to the default +`approverin:already-approved-by_owners` behavior. It applies when a new patch-set updates only files +that are owned by the change owner or patch-set committer, in a situation where the normal +`approverin:already-approved-by_owners` logic would otherwise drop that owner's previous vote. + +The rationale is simple: if an owner already approved a change that stays entirely within code they +own, and the next patch set is uploaded by that same owner, forcing that same person to re-apply +the same vote adds little review value. + +See [copy-conditions.md](copy-conditions.md) for predicate evaluation details. + +This field can be configured only at `OWNERS` file level. +If the field is not set, it defaults to `true`. + +If `auto-owners-approved` is `false` for any touched file, the predicate does not use that +self-update shortcut for the patch set. The usual `approverin:already-approved-by_owners` logic +still applies. + +### Inheritance + +The usual `OWNERS` [inheritance](#global-project-owners) logic applies to `auto-owners-approved` as +well. This includes directory `OWNERS` lookup, project `refs/meta/config` `OWNERS`, and +parent project `OWNERS` when inheritance continues up the project hierarchy. + +### auto-owners-approved example + +Disable at `OWNERS` level: + + inherited: true + auto-owners-approved: false + +With this setting, the predicate will not copy an owner's vote just because the owner is updating +only files they own on their own change. Paths under that `OWNERS` file still participate in the +normal copy-condition behavior. + ## Example 1 - OWNERS file without matchers Given an OWNERS configuration of: @@ -211,8 +248,8 @@ ```yaml inherited: true owners: -- John Doe -- Doug Smith + - John Doe + - Doug Smith ``` In this case the owners plugin will assume the default label configuration,`Code-Review @@ -222,6 +259,7 @@ change, you can then either enable `owners.enableSubmitRequirement = true` in your `gerrit.config` or define a submit requirement in your `project.config` that uses the `has:approval_owners` in the `submittableIf` section, like so: + ``` [submit-requirement "Owner-Approval"] description = Files needs to be approved by owners @@ -263,8 +301,8 @@ inherited: true label: Owner-Approved, 1 owners: -- John Doe -- Doug Smith + - John Doe + - Doug Smith ``` This will mean that, a change cannot be submitted until 'John Doe' or 'Doug @@ -279,6 +317,7 @@ If you no longer wish to require a `Code-Review +2` and would rather only use the custom submit requirement, you have two options: + - change the definition of the `Code-Review` label in `All-Projects`'s `project.config` so that `function = NoOp`. - set an `overrideIf` clause in your custom submit requirement definition @@ -302,13 +341,13 @@ ```yaml inherited: true matchers: -- suffix: .sql - owners: - - Mister Dba -- regex: .*Test.* - owners: - - John Bug - - Matt Free + - suffix: .sql + owners: + - Mister Dba + - regex: .*Test.* + owners: + - John Bug + - Matt Free ``` You can then either enable `owners.enableSubmitRequirement = true` in your
diff --git a/owners/src/main/resources/Documentation/copy-conditions.md b/owners/src/main/resources/Documentation/copy-conditions.md index b7f617b..3b93755 100644 --- a/owners/src/main/resources/Documentation/copy-conditions.md +++ b/owners/src/main/resources/Documentation/copy-conditions.md
@@ -118,3 +118,11 @@ value = +2 Looks good to me, approved copyCondition = approverin:already-approved-by_owners ``` + +## Customizing the copy condition behaviour with the `auto-owners-approved` in `OWNERS` + +The `approverin:already-approved-by_owners` can be fine-grained enabled/disabled using the +`auto-owners-approved` configuration in the `OWNERS` file. + +Details on the `auto-owners-approved` behaviour can be +found [here](./config.md#auto-owners-approved).
diff --git a/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java b/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java index 166bf18..74cacc5 100644 --- a/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java +++ b/owners/src/test/java/com/googlesource/gerrit/owners/AlreadyApprovedByCopyConditionIT.java
@@ -62,6 +62,7 @@ private TestAccount FRONTEND_FILES_OWNER; private TestAccount BACKEND_FILES_OWNER; + private TestAccount NON_OWNER; private static final String FRONTEND_OWNED_FILE = "foo.js"; private static final String BACKEND_OWNED_FILE = "foo.java"; @@ -87,6 +88,7 @@ FRONTEND_FILES_OWNER = accountCreator.create("user-frontend"); BACKEND_FILES_OWNER = accountCreator.create("user-backend"); + NON_OWNER = accountCreator.create("user-non-owner"); addOwnerFileWithMatchersToRoot( Map.of( @@ -422,6 +424,155 @@ assertVotes(c, BACKEND_FILES_OWNER, 2); } + @Test + public void shouldCopyApprovalWhenAllModifiedFilesAreOwnedAndAutoOwnersApprovedIsDefault() + throws Exception { + assertOwnedOnlySelfUpdateCopiesApproval( + String.format("inherited: true\nowners:\n- %s\n", BACKEND_FILES_OWNER.username())); + } + + @Test + public void shouldNotCopyApprovalWhenAllModifiedFilesAreOwnedButApproverIsNotChangeOwner() + throws Exception { + Change.Id changeId = + changeOperations + .newChange() + .project(project) + .owner(NON_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("java content") + .create(); + + vote(BACKEND_FILES_OWNER, changeId.toString(), 2); + + changeOperations + .change(changeId) + .newPatchset() + .uploader(BACKEND_FILES_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("updated java content") + .create(); + + ChangeInfo c = detailedChange(changeId.toString()); + assertVotes(c, BACKEND_FILES_OWNER, 0); + } + + @Test + public void shouldNotCopyApprovalWhenAllModifiedFilesAreOwnedButUploaderNotOwner() + throws Exception { + Change.Id changeId = + changeOperations + .newChange() + .project(project) + .owner(BACKEND_FILES_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("java content") + .create(); + + vote(BACKEND_FILES_OWNER, changeId.toString(), 2); + + changeOperations + .change(changeId) + .newPatchset() + .uploader(NON_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("updated java content") + .create(); + + ChangeInfo c = detailedChange(changeId.toString()); + assertVotes(c, BACKEND_FILES_OWNER, 0); + } + + @Test + public void shouldNotCopyApprovalWhenAllModifiedFilesAreOwnedButAutoOwnersApprovedIsFalse() + throws Exception { + pushOwnersToMaster( + String.format( + "inherited: true\nauto-owners-approved: false\nowners:\n- %s\n", + BACKEND_FILES_OWNER.username())); + + Change.Id changeId = + changeOperations + .newChange() + .project(project) + .owner(BACKEND_FILES_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("java content") + .create(); + + vote(BACKEND_FILES_OWNER, changeId.toString(), 2); + + changeOperations + .change(changeId) + .newPatchset() + .uploader(BACKEND_FILES_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("updated java content") + .create(); + + ChangeInfo c = detailedChange(changeId.toString()); + assertVotes(c, BACKEND_FILES_OWNER, 0); + } + + @Test + public void shouldCopyApprovalWhenAllModifiedFilesAreOwnedAndAutoOwnersApprovedIsTrue() + throws Exception { + assertOwnedOnlySelfUpdateCopiesApproval( + String.format( + "inherited: true\nauto-owners-approved: true\nowners:\n- %s\n", + BACKEND_FILES_OWNER.username())); + } + + @Test + public void shouldCopyApprovalWhenAutoOwnersApprovedIsFalseButOwnedEditsAreRebaseOnly() + throws Exception { + pushOwnersToMaster( + String.format( + "inherited: true\nauto-owners-approved: false\nowners:\n- %s\n", + BACKEND_FILES_OWNER.username())); + + ObjectId initialCommitId = createInitialContentFor(BACKEND_OWNED_FILE); + PushOneCommit.Result amendL3 = + createChangeWithReplacedContent(BACKEND_OWNED_FILE, "Line 3\n", "Line three\n"); + vote(BACKEND_FILES_OWNER, amendL3.getChangeId(), 2); + + testRepo.reset(initialCommitId); + PushOneCommit.Result amendL7 = + createChangeWithReplacedContent(BACKEND_OWNED_FILE, "Line 7\n", "Line seven\n"); + + rebaseChangeOn(amendL3.getChangeId(), amendL7.getCommit().getId()); + + ChangeInfo c = detailedChange(amendL3.getChangeId()); + assertVotes(c, BACKEND_FILES_OWNER, 2); + } + + @Test + public void shouldNotCopyApprovalWhenChangedFilesAreNotOwnedByUploader() throws Exception { + Change.Id changeId = + changeOperations + .newChange() + .project(project) + .owner(BACKEND_FILES_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("java content") + .create(); + + vote(BACKEND_FILES_OWNER, changeId.toString(), 2); + + changeOperations + .change(changeId) + .newPatchset() + .uploader(BACKEND_FILES_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("updated java content") + .file(FILE_WITH_NO_OWNERS) + .content("updated text") + .create(); + + ChangeInfo c = detailedChange(changeId.toString()); + assertVotes(c, BACKEND_FILES_OWNER, 0); + } + private PushOneCommit.Result createChangeWithReplacedContent( String file, String oldLine, String replacement) throws Exception { PushOneCommit.Result r = @@ -472,6 +623,32 @@ assertThat(vote).isEqualTo(expectedVote); } + private void assertOwnedOnlySelfUpdateCopiesApproval(String owners) throws Exception { + pushOwnersToMaster(owners); + + Change.Id changeId = + changeOperations + .newChange() + .project(project) + .owner(BACKEND_FILES_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("java content") + .create(); + + vote(BACKEND_FILES_OWNER, changeId.toString(), 2); + + changeOperations + .change(changeId) + .newPatchset() + .uploader(BACKEND_FILES_OWNER.id()) + .file(BACKEND_OWNED_FILE) + .content("updated java content") + .create(); + + ChangeInfo c = detailedChange(changeId.toString()); + assertVotes(c, BACKEND_FILES_OWNER, 2); + } + private void vote(TestAccount user, String changeId, int vote) throws Exception { requestScopeOperations.setApiUser(user.id()); gApi.changes() @@ -519,6 +696,28 @@ pushOwnersToMaster(String.format("inherited: %s\nmatchers:\n%s", true, matchersYaml)); } + private void addOwnerFileWithMatchersToRoot( + Map<String, List<TestAccount>> ownersBySuffix, boolean autoOwnersApproved) throws Exception { + String matchersYaml = + ownersBySuffix.entrySet().stream() + .map( + entry -> { + String suffix = entry.getKey(); + List<TestAccount> users = entry.getValue(); + String ownersYaml = + users.stream() + .map(user -> String.format(" - %s\n", user.username())) + .collect(joining()); + return String.format("- suffix: %s\n owners:\n%s", suffix, ownersYaml); + }) + .collect(joining()); + + pushOwnersToMaster( + String.format( + "inherited: %s\nauto-owners-approved: %s\nmatchers:\n%s", + true, autoOwnersApproved, matchersYaml)); + } + private void pushOwnersToMaster(String owners) throws Exception { pushFactory .create(admin.newIdent(), testRepo, "Add OWNER file", "OWNERS", owners)