Merge changes I8c557145,Iee7c9372,I853673d6,I273fb5d2,I83430861, ... * changes: OnCodeOwnerApproval: Detect if approval is ignored due to self approval OnCodeOwnerOverride: Detect if override is ignored due to self approval Add debug logs when override approval is ignored due to self approval Remove unused methods from Subject classes OnCodeOwnerApprovalIT: Reformat class javadoc Add test for TestPathExpressions TestPathExpressions: Match file types in all subfolders
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java index e571724..4dc69b0 100644 --- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java +++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnerProjectConfigInput.java
@@ -15,6 +15,8 @@ package com.google.gerrit.plugins.codeowners.api; import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; +import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy; +import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy; import java.util.List; /** @@ -65,4 +67,43 @@ /** Policy that controls who should own paths that have no code owners defined. */ public FallbackCodeOwners fallbackCodeOwners; + + /** Emails of users that should be code owners globally across all branches. */ + public List<String> globalCodeOwners; + + /** Strategy that defines for merge commits which files require code owner approvals. */ + public MergeCommitStrategy mergeCommitStrategy; + + /** Whether an implicit code owner approval from the last uploader is assumed. */ + public Boolean implicitApprovals; + + /** + * URL for a page that provides project/host-specific information about how to request a code + * owner override. + */ + public String overrideInfoUrl; + + /** Whether code owner config files are read-only. */ + public Boolean readOnly; + + /** Policy for validating code owner config files when a commit is received. */ + public CodeOwnerConfigValidationPolicy enableValidationOnCommitReceived; + + /** Policy for validating code owner config files when a change is submitted. */ + public CodeOwnerConfigValidationPolicy enableValidationOnSubmit; + + /** + * Whether modifications of code owner config files that newly add non-resolvable code owners + * should be rejected on commit received and submit. + */ + public Boolean rejectNonResolvableCodeOwners; + + /** + * Whether modifications of code owner config files that newly add non-resolvable imports should + * be rejected on commit received an submit. + */ + public Boolean rejectNonResolvableImports; + + /** The maximum number of paths that are included in change messages. */ + public Integer maxPathsInChangeMessages; }
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/BUILD b/java/com/google/gerrit/plugins/codeowners/backend/BUILD index 5aefae5..83671a0 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/BUILD +++ b/java/com/google/gerrit/plugins/codeowners/backend/BUILD
@@ -7,6 +7,7 @@ visibility = ["//visibility:public"], deps = PLUGIN_DEPS_NEVERLINK + [ "//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/common", + "//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/metrics", "//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/util", "//plugins/code-owners/proto:owners_metadata_java_proto", ],
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java index 78956a2..1e76b05 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/ChangedFiles.java
@@ -23,9 +23,11 @@ import com.google.gerrit.entities.Patch; import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; +import com.google.gerrit.metrics.Timer0; import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration; import com.google.gerrit.plugins.codeowners.common.ChangedFile; import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy; +import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics; import com.google.gerrit.server.change.RevisionResource; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.patch.PatchListCache; @@ -60,15 +62,18 @@ private final GitRepositoryManager repoManager; private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration; private final PatchListCache patchListCache; + private final CodeOwnerMetrics codeOwnerMetrics; @Inject public ChangedFiles( GitRepositoryManager repoManager, CodeOwnersPluginConfiguration codeOwnersPluginConfiguration, - PatchListCache patchListCache) { + PatchListCache patchListCache, + CodeOwnerMetrics codeOwnerMetrics) { this.repoManager = repoManager; this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration; this.patchListCache = patchListCache; + this.codeOwnerMetrics = codeOwnerMetrics; } /** @@ -139,12 +144,14 @@ logger.atFine().log( "computing changed files for revision %s in project %s", revCommit.name(), project); - if (revCommit.getParentCount() > 1 - && MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) { - return computeByComparingAgainstAutoMerge(project, revCommit); - } + try (Timer0.Context ctx = codeOwnerMetrics.computeChangedFiles.start()) { + if (revCommit.getParentCount() > 1 + && MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION.equals(mergeCommitStrategy)) { + return computeByComparingAgainstAutoMerge(project, revCommit); + } - return computeByComparingAgainstFirstParent(repoConfig, revWalk, revCommit); + return computeByComparingAgainstFirstParent(repoConfig, revWalk, revCommit); + } } /**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java index 6a41a91..33fd2e4 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerApprovalCheck.java
@@ -33,15 +33,14 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.restapi.ResourceConflictException; +import com.google.gerrit.metrics.Timer0; import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration; import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval; import com.google.gerrit.plugins.codeowners.common.ChangedFile; import com.google.gerrit.plugins.codeowners.common.CodeOwnerStatus; +import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics; import com.google.gerrit.server.ApprovalsUtil; import com.google.gerrit.server.git.GitRepositoryManager; -import com.google.gerrit.server.logging.Metadata; -import com.google.gerrit.server.logging.TraceContext; -import com.google.gerrit.server.logging.TraceContext.TraceTimer; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ReviewerStateInternal; import com.google.gerrit.server.patch.PatchListNotAvailableException; @@ -92,6 +91,7 @@ private final CodeOwnerConfigHierarchy codeOwnerConfigHierarchy; private final Provider<CodeOwnerResolver> codeOwnerResolver; private final ApprovalsUtil approvalsUtil; + private final CodeOwnerMetrics codeOwnerMetrics; @Inject CodeOwnerApprovalCheck( @@ -102,7 +102,8 @@ CodeOwnerConfigScanner.Factory codeOwnerConfigScannerFactory, CodeOwnerConfigHierarchy codeOwnerConfigHierarchy, Provider<CodeOwnerResolver> codeOwnerResolver, - ApprovalsUtil approvalsUtil) { + ApprovalsUtil approvalsUtil, + CodeOwnerMetrics codeOwnerMetrics) { this.permissionBackend = permissionBackend; this.repoManager = repoManager; this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration; @@ -111,6 +112,7 @@ this.codeOwnerConfigHierarchy = codeOwnerConfigHierarchy; this.codeOwnerResolver = codeOwnerResolver; this.approvalsUtil = approvalsUtil; + this.codeOwnerMetrics = codeOwnerMetrics; } /** @@ -201,13 +203,11 @@ public Stream<FileCodeOwnerStatus> getFileStatuses(ChangeNotes changeNotes) throws ResourceConflictException, IOException, PatchListNotAvailableException { requireNonNull(changeNotes, "changeNotes"); - try (TraceTimer traceTimer = - TraceContext.newTimer( - "Compute file statuses", - Metadata.builder() - .projectName(changeNotes.getProjectName().get()) - .changeId(changeNotes.getChangeId().get()) - .build())) { + try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatuses.start()) { + logger.atFine().log( + "compute file statuses (project = %s, change = %d)", + changeNotes.getProjectName(), changeNotes.getChangeId().get()); + boolean enableImplicitApprovalFromUploader = codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(changeNotes.getProjectName()); Account.Id patchSetUploader = changeNotes.getCurrentPatchSet().uploader(); @@ -296,14 +296,14 @@ requireNonNull(changeNotes, "changeNotes"); requireNonNull(patchSet, "patchSet"); requireNonNull(accountId, "accountId"); - try (TraceTimer traceTimer = - TraceContext.newTimer( - "Compute file statuses for account", - Metadata.builder() - .projectName(changeNotes.getProjectName().get()) - .changeId(changeNotes.getChangeId().get()) - .patchSetId(patchSet.id().get()) - .build())) { + try (Timer0.Context ctx = codeOwnerMetrics.computeFileStatusesForAccount.start()) { + logger.atFine().log( + "compute file statuses for account %d (project = %s, change = %d, patch set = %d)", + accountId.get(), + changeNotes.getProjectName(), + changeNotes.getChangeId().get(), + patchSet.id().get()); + RequiredApproval requiredApproval = codeOwnersPluginConfiguration.getRequiredApproval(changeNotes.getProjectName()); logger.atFine().log("requiredApproval = %s", requiredApproval);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java index 2fb7607..31dda71 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerResolver.java
@@ -25,7 +25,9 @@ import com.google.gerrit.entities.Account; import com.google.gerrit.entities.Project; import com.google.gerrit.exceptions.StorageException; +import com.google.gerrit.metrics.Timer0; import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration; +import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.AccountCache; @@ -33,9 +35,6 @@ import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.account.externalids.ExternalIds; -import com.google.gerrit.server.logging.Metadata; -import com.google.gerrit.server.logging.TraceContext; -import com.google.gerrit.server.logging.TraceContext.TraceTimer; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -62,6 +61,7 @@ private final AccountCache accountCache; private final AccountControl.Factory accountControlFactory; private final PathCodeOwners.Factory pathCodeOwnersFactory; + private final CodeOwnerMetrics codeOwnerMetrics; // Enforce visibility by default. private boolean enforceVisibility = true; @@ -79,7 +79,8 @@ ExternalIds externalIds, AccountCache accountCache, AccountControl.Factory accountControlFactory, - PathCodeOwners.Factory pathCodeOwnersFactory) { + PathCodeOwners.Factory pathCodeOwnersFactory, + CodeOwnerMetrics codeOwnerMetrics) { this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration; this.permissionBackend = permissionBackend; this.currentUser = currentUser; @@ -87,6 +88,7 @@ this.accountCache = accountCache; this.accountControlFactory = accountControlFactory; this.pathCodeOwnersFactory = pathCodeOwnersFactory; + this.codeOwnerMetrics = codeOwnerMetrics; } /** @@ -147,15 +149,10 @@ requireNonNull(absolutePath, "absolutePath"); checkState(absolutePath.isAbsolute(), "path %s must be absolute", absolutePath); - try (TraceTimer traceTimer = - TraceContext.newTimer( - "Resolve path code owners", - Metadata.builder() - .projectName(codeOwnerConfig.key().project().get()) - .branchName(codeOwnerConfig.key().ref()) - .filePath(codeOwnerConfig.key().fileName().orElse("<default>")) - .build())) { - logger.atFine().log("resolving path code owners for path %s", absolutePath); + try (Timer0.Context ctx = codeOwnerMetrics.resolvePathCodeOwners.start()) { + logger.atFine().log( + "resolve path code owners (code owner config = %s, path = %s)", + codeOwnerConfig.key(), absolutePath); PathCodeOwnersResult pathCodeOwnersResult = pathCodeOwnersFactory.create(codeOwnerConfig, absolutePath).resolveCodeOwnerConfig(); return resolve(
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java index 6cf8ef3..4c23676 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -23,10 +23,9 @@ import com.google.gerrit.extensions.annotations.Exports; import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.metrics.Timer0; import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration; -import com.google.gerrit.server.logging.Metadata; -import com.google.gerrit.server.logging.TraceContext; -import com.google.gerrit.server.logging.TraceContext.TraceTimer; +import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.query.change.ChangeData; @@ -56,13 +55,16 @@ private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration; private final CodeOwnerApprovalCheck codeOwnerApprovalCheck; + private final CodeOwnerMetrics codeOwnerMetrics; @Inject CodeOwnerSubmitRule( CodeOwnersPluginConfiguration codeOwnersPluginConfiguration, - CodeOwnerApprovalCheck codeOwnerApprovalCheck) { + CodeOwnerApprovalCheck codeOwnerApprovalCheck, + CodeOwnerMetrics codeOwnerMetrics) { this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration; this.codeOwnerApprovalCheck = codeOwnerApprovalCheck; + this.codeOwnerMetrics = codeOwnerMetrics; } @Override @@ -70,13 +72,11 @@ try { requireNonNull(changeData, "changeData"); - try (TraceTimer traceTimer = - TraceContext.newTimer( - "Run code owner submit rule", - Metadata.builder() - .projectName(changeData.project().get()) - .changeId(changeData.getId().get()) - .build())) { + try (Timer0.Context ctx = codeOwnerMetrics.runCodeOwnerSubmitRule.start()) { + logger.atFine().log( + "run code owner submit rule (project = %s, change = %d)", + changeData.project().get(), changeData.getId().get()); + if (codeOwnersPluginConfiguration.isDisabled(changeData.change().getDest())) { logger.atFine().log( "code owners functionality is disabled for branch %s", changeData.change().getDest());
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java index f787dc8..4f67254 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -24,10 +24,9 @@ import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.BranchNameKey; import com.google.gerrit.entities.Project; +import com.google.gerrit.metrics.Timer0; import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration; -import com.google.gerrit.server.logging.Metadata; -import com.google.gerrit.server.logging.TraceContext; -import com.google.gerrit.server.logging.TraceContext.TraceTimer; +import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; import com.google.inject.Inject; @@ -54,15 +53,18 @@ @Singleton public static class Factory { + private final CodeOwnerMetrics codeOwnerMetrics; private final ProjectCache projectCache; private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration; private final CodeOwners codeOwners; @Inject Factory( + CodeOwnerMetrics codeOwnerMetrics, ProjectCache projectCache, CodeOwnersPluginConfiguration codeOwnersPluginConfiguration, CodeOwners codeOwners) { + this.codeOwnerMetrics = codeOwnerMetrics; this.projectCache = projectCache; this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration; this.codeOwners = codeOwners; @@ -71,6 +73,7 @@ public PathCodeOwners create(CodeOwnerConfig codeOwnerConfig, Path absolutePath) { requireNonNull(codeOwnerConfig, "codeOwnerConfig"); return new PathCodeOwners( + codeOwnerMetrics, projectCache, codeOwners, codeOwnerConfig, @@ -85,6 +88,7 @@ .map( codeOwnerConfig -> new PathCodeOwners( + codeOwnerMetrics, projectCache, codeOwners, codeOwnerConfig, @@ -118,6 +122,7 @@ } } + private final CodeOwnerMetrics codeOwnerMetrics; private final ProjectCache projectCache; private final CodeOwners codeOwners; private final CodeOwnerConfig codeOwnerConfig; @@ -127,11 +132,13 @@ private PathCodeOwnersResult pathCodeOwnersResult; private PathCodeOwners( + CodeOwnerMetrics codeOwnerMetrics, ProjectCache projectCache, CodeOwners codeOwners, CodeOwnerConfig codeOwnerConfig, Path path, PathExpressionMatcher pathExpressionMatcher) { + this.codeOwnerMetrics = requireNonNull(codeOwnerMetrics, "codeOwnerMetrics"); this.projectCache = requireNonNull(projectCache, "projectCache"); this.codeOwners = requireNonNull(codeOwners, "codeOwners"); this.codeOwnerConfig = requireNonNull(codeOwnerConfig, "codeOwnerConfig"); @@ -187,13 +194,7 @@ return this.pathCodeOwnersResult; } - try (TraceTimer traceTimer = - TraceContext.newTimer( - "Resolve code owner config", - Metadata.builder() - .projectName(codeOwnerConfig.key().project().get()) - .filePath(path.toString()) - .build())) { + try (Timer0.Context ctx = codeOwnerMetrics.resolveCodeOwnerConfig.start()) { logger.atFine().log( "resolve code owners for %s from code owner config %s", path, codeOwnerConfig.key()); @@ -249,14 +250,9 @@ CodeOwnerConfig importingCodeOwnerConfig, CodeOwnerConfig.Builder resolvedCodeOwnerConfigBuilder) { ImmutableList.Builder<UnresolvedImport> unresolvedImports = ImmutableList.builder(); - try (TraceTimer traceTimer = - TraceContext.newTimer( - "Resolve code owner config imports", - Metadata.builder() - .projectName(codeOwnerConfig.key().project().get()) - .branchName(codeOwnerConfig.key().ref()) - .filePath(codeOwnerConfig.key().filePath("<default>").toString()) - .build())) { + try (Timer0.Context ctx = codeOwnerMetrics.resolveCodeOwnerConfigImports.start()) { + logger.atFine().log("resolve imports of codeOwnerConfig %s", importingCodeOwnerConfig.key()); + // To detect cyclic dependencies we keep track of all seen code owner configs. Set<CodeOwnerConfig.Key> seenCodeOwnerConfigs = new HashSet<>(); seenCodeOwnerConfigs.add(codeOwnerConfig.key()); @@ -278,17 +274,10 @@ CodeOwnerConfig.Key keyOfImportedCodeOwnerConfig = createKeyForImportedCodeOwnerConfig( importingCodeOwnerConfig.key(), codeOwnerConfigReference); - try (TraceTimer traceTimer2 = - TraceContext.newTimer( - "Resolve code owner config import", - Metadata.builder() - .projectName(keyOfImportedCodeOwnerConfig.project().get()) - .branchName(keyOfImportedCodeOwnerConfig.ref()) - .filePath( - keyOfImportedCodeOwnerConfig - .filePath(codeOwnerConfigReference.fileName()) - .toString()) - .build())) { + try (Timer0.Context ctx2 = codeOwnerMetrics.resolveCodeOwnerConfigImport.start()) { + logger.atFine().log( + "resolve import of code owner config %s", keyOfImportedCodeOwnerConfig); + Optional<ProjectState> projectState = projectCache.get(keyOfImportedCodeOwnerConfig.project()); if (!projectState.isPresent()) {
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java index 8348893..3e5538c 100644 --- a/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java +++ b/java/com/google/gerrit/plugins/codeowners/backend/config/GeneralConfig.java
@@ -79,7 +79,7 @@ @VisibleForTesting public static final String KEY_OVERRIDE_INFO_URL = "overrideInfoUrl"; - @VisibleForTesting static final int DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES = 100; + @VisibleForTesting public static final int DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES = 100; @VisibleForTesting public static final String KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS =
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/BUILD b/java/com/google/gerrit/plugins/codeowners/metrics/BUILD new file mode 100644 index 0000000..1f1ad14 --- /dev/null +++ b/java/com/google/gerrit/plugins/codeowners/metrics/BUILD
@@ -0,0 +1,9 @@ +load("@rules_java//java:defs.bzl", "java_library") +load("//tools/bzl:plugin.bzl", "PLUGIN_DEPS_NEVERLINK") + +java_library( + name = "metrics", + srcs = glob(["*.java"]), + visibility = ["//visibility:public"], + deps = PLUGIN_DEPS_NEVERLINK, +)
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java new file mode 100644 index 0000000..e1ce3bf --- /dev/null +++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -0,0 +1,74 @@ +// Copyright (C) 2021 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.plugins.codeowners.metrics; + +import com.google.gerrit.metrics.Description; +import com.google.gerrit.metrics.Description.Units; +import com.google.gerrit.metrics.MetricMaker; +import com.google.gerrit.metrics.Timer0; +import com.google.inject.Inject; +import com.google.inject.Singleton; + +/** Metrics of the code-owners plugin. */ +@Singleton +public class CodeOwnerMetrics { + public final Timer0 computeChangedFiles; + public final Timer0 computeFileStatuses; + public final Timer0 computeFileStatusesForAccount; + public final Timer0 resolveCodeOwnerConfig; + public final Timer0 resolveCodeOwnerConfigImport; + public final Timer0 resolveCodeOwnerConfigImports; + public final Timer0 resolvePathCodeOwners; + public final Timer0 runCodeOwnerSubmitRule; + + private final MetricMaker metricMaker; + + @Inject + CodeOwnerMetrics(MetricMaker metricMaker) { + this.metricMaker = metricMaker; + + this.computeChangedFiles = + createLatencyTimer("compute_changed_files", "Latency for computing changed files"); + this.computeFileStatuses = + createLatencyTimer("compute_file_statuses", "Latency for computing file statuses"); + this.computeFileStatusesForAccount = + createLatencyTimer( + "compute_file_statuses_for_account", + "Latency for computing file statuses for an account"); + this.resolveCodeOwnerConfig = + createLatencyTimer( + "resolve_code_owner_config", "Latency for resolving a code owner config file"); + this.resolveCodeOwnerConfigImport = + createLatencyTimer( + "resolve_code_owner_config_import", + "Latency for resolving an import of a code owner config file"); + this.resolveCodeOwnerConfigImports = + createLatencyTimer( + "resolve_code_owner_config_imports", + "Latency for resolving all imports of a code owner config file"); + this.resolvePathCodeOwners = + createLatencyTimer( + "resolve_path_code_owners", "Latency for resolving the code owners of a path"); + this.runCodeOwnerSubmitRule = + createLatencyTimer( + "run_code_owner_submit_rule", "Latency for running the code owner submit rule"); + } + + private Timer0 createLatencyTimer(String name, String description) { + return metricMaker.newTimer( + "code_owners/" + name, + new Description(description).setCumulative().setUnit(Units.MILLISECONDS)); + } +}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java b/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java index 69cf3db..597b694 100644 --- a/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java +++ b/java/com/google/gerrit/plugins/codeowners/restapi/PutCodeOwnerProjectConfig.java
@@ -15,8 +15,18 @@ package com.google.gerrit.plugins.codeowners.restapi; import static com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_SUBMIT; import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_FALLBACK_CODE_OWNERS; import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_FILE_EXTENSION; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_GLOBAL_CODE_OWNER; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_MAX_PATHS_IN_CHANGE_MESSAGES; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_MERGE_COMMIT_STRATEGY; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_OVERRIDE_INFO_URL; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_READ_ONLY; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS; +import static com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig.KEY_REJECT_NON_RESOLVABLE_IMPORTS; import static com.google.gerrit.plugins.codeowners.backend.config.OverrideApprovalConfig.KEY_OVERRIDE_APPROVAL; import static com.google.gerrit.plugins.codeowners.backend.config.RequiredApprovalConfig.KEY_REQUIRED_APPROVAL; import static com.google.gerrit.plugins.codeowners.backend.config.StatusConfig.KEY_DISABLED; @@ -150,6 +160,83 @@ input.fallbackCodeOwners); } + if (input.globalCodeOwners != null) { + codeOwnersConfig.setStringList( + SECTION_CODE_OWNERS, + /* subsection= */ null, + KEY_GLOBAL_CODE_OWNER, + input.globalCodeOwners); + } + + if (input.mergeCommitStrategy != null) { + codeOwnersConfig.setEnum( + SECTION_CODE_OWNERS, + /* subsection= */ null, + KEY_MERGE_COMMIT_STRATEGY, + input.mergeCommitStrategy); + } + + if (input.implicitApprovals != null) { + codeOwnersConfig.setBoolean( + SECTION_CODE_OWNERS, + /* subsection= */ null, + KEY_ENABLE_IMPLICIT_APPROVALS, + input.implicitApprovals); + } + + if (input.overrideInfoUrl != null) { + codeOwnersConfig.setString( + SECTION_CODE_OWNERS, + /* subsection= */ null, + KEY_OVERRIDE_INFO_URL, + input.overrideInfoUrl); + } + + if (input.readOnly != null) { + codeOwnersConfig.setBoolean( + SECTION_CODE_OWNERS, /* subsection= */ null, KEY_READ_ONLY, input.readOnly); + } + + if (input.enableValidationOnCommitReceived != null) { + codeOwnersConfig.setEnum( + SECTION_CODE_OWNERS, + /* subsection= */ null, + KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED, + input.enableValidationOnCommitReceived); + } + + if (input.enableValidationOnSubmit != null) { + codeOwnersConfig.setEnum( + SECTION_CODE_OWNERS, + /* subsection= */ null, + KEY_ENABLE_VALIDATION_ON_SUBMIT, + input.enableValidationOnSubmit); + } + + if (input.rejectNonResolvableCodeOwners != null) { + codeOwnersConfig.setBoolean( + SECTION_CODE_OWNERS, + /* subsection= */ null, + KEY_REJECT_NON_RESOLVABLE_CODE_OWNERS, + input.rejectNonResolvableCodeOwners); + } + + if (input.rejectNonResolvableImports != null) { + codeOwnersConfig.setBoolean( + SECTION_CODE_OWNERS, + /* subsection= */ null, + KEY_REJECT_NON_RESOLVABLE_IMPORTS, + input.rejectNonResolvableImports); + } + + if (input.maxPathsInChangeMessages != null) { + codeOwnersConfig.setInt( + SECTION_CODE_OWNERS, + /* subsection= */ null, + KEY_MAX_PATHS_IN_CHANGE_MESSAGES, + input.maxPathsInChangeMessages); + } + validateConfig(projectResource.getProjectState(), codeOwnersConfig); codeOwnersProjectConfigFile.commit(metaDataUpdate);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java index 11d60b7..6ce59bc 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/PutCodeOwnerProjectConfigIT.java
@@ -14,6 +14,7 @@ package com.google.gerrit.plugins.codeowners.acceptance.api; +import static com.google.common.collect.ImmutableSet.toImmutableSet; import static com.google.common.truth.Truth.assertThat; import static com.google.gerrit.plugins.codeowners.testing.RequiredApprovalSubject.assertThat; import static com.google.gerrit.server.project.ProjectCache.illegalState; @@ -33,9 +34,13 @@ import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersIT; import com.google.gerrit.plugins.codeowners.api.CodeOwnerProjectConfigInfo; import com.google.gerrit.plugins.codeowners.api.CodeOwnerProjectConfigInput; +import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference; import com.google.gerrit.plugins.codeowners.backend.FallbackCodeOwners; import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration; +import com.google.gerrit.plugins.codeowners.backend.config.GeneralConfig; import com.google.gerrit.plugins.codeowners.backend.config.RequiredApproval; +import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy; +import com.google.gerrit.plugins.codeowners.common.MergeCommitStrategy; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.restapi.project.DeleteRef; import com.google.inject.Inject; @@ -308,6 +313,184 @@ } @Test + public void setGlobalCodeOwners() throws Exception { + assertThat(codeOwnersPluginConfiguration.getGlobalCodeOwners(project)).isEmpty(); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.globalCodeOwners = ImmutableList.of(user.email(), "foo.bar@example.com"); + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat( + codeOwnersPluginConfiguration.getGlobalCodeOwners(project).stream() + .map(CodeOwnerReference::email) + .collect(toImmutableSet())) + .containsExactly(user.email(), "foo.bar@example.com"); + + input.globalCodeOwners = ImmutableList.of(); + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.getGlobalCodeOwners(project)).isEmpty(); + } + + @Test + public void setMergeCommitStrategy() throws Exception { + assertThat(codeOwnersPluginConfiguration.getMergeCommitStrategy(project)) + .isEqualTo(MergeCommitStrategy.ALL_CHANGED_FILES); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.mergeCommitStrategy = MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION; + CodeOwnerProjectConfigInfo updatedConfig = + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(updatedConfig.general.mergeCommitStrategy) + .isEqualTo(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION); + assertThat(codeOwnersPluginConfiguration.getMergeCommitStrategy(project)) + .isEqualTo(MergeCommitStrategy.FILES_WITH_CONFLICT_RESOLUTION); + + input.mergeCommitStrategy = MergeCommitStrategy.ALL_CHANGED_FILES; + updatedConfig = projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(updatedConfig.general.mergeCommitStrategy) + .isEqualTo(MergeCommitStrategy.ALL_CHANGED_FILES); + assertThat(codeOwnersPluginConfiguration.getMergeCommitStrategy(project)) + .isEqualTo(MergeCommitStrategy.ALL_CHANGED_FILES); + } + + @Test + public void setImplicitApprovals() throws Exception { + assertThat(codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(project)).isFalse(); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.implicitApprovals = true; + CodeOwnerProjectConfigInfo updatedConfig = + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(updatedConfig.general.implicitApprovals).isTrue(); + assertThat(codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(project)).isTrue(); + + input.implicitApprovals = false; + updatedConfig = projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(updatedConfig.general.implicitApprovals).isNull(); + assertThat(codeOwnersPluginConfiguration.areImplicitApprovalsEnabled(project)).isFalse(); + } + + @Test + public void setOverrideInfoUrl() throws Exception { + assertThat(codeOwnersPluginConfiguration.getOverrideInfoUrl(project)).isEmpty(); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.overrideInfoUrl = "http://foo.bar"; + CodeOwnerProjectConfigInfo updatedConfig = + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(updatedConfig.general.overrideInfoUrl).isEqualTo("http://foo.bar"); + assertThat(codeOwnersPluginConfiguration.getOverrideInfoUrl(project)) + .value() + .isEqualTo("http://foo.bar"); + + input.overrideInfoUrl = ""; + updatedConfig = projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(updatedConfig.general.overrideInfoUrl).isNull(); + assertThat(codeOwnersPluginConfiguration.getOverrideInfoUrl(project)).isEmpty(); + } + + @Test + public void setReadOnly() throws Exception { + assertThat(codeOwnersPluginConfiguration.areCodeOwnerConfigsReadOnly(project)).isFalse(); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.readOnly = true; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.areCodeOwnerConfigsReadOnly(project)).isTrue(); + + input.readOnly = false; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.areCodeOwnerConfigsReadOnly(project)).isFalse(); + } + + @Test + public void setEnableValidationOnCommitReceived() throws Exception { + assertThat( + codeOwnersPluginConfiguration.getCodeOwnerConfigValidationPolicyForCommitReceived( + project)) + .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.enableValidationOnCommitReceived = CodeOwnerConfigValidationPolicy.FALSE; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat( + codeOwnersPluginConfiguration.getCodeOwnerConfigValidationPolicyForCommitReceived( + project)) + .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE); + + input.enableValidationOnCommitReceived = CodeOwnerConfigValidationPolicy.TRUE; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat( + codeOwnersPluginConfiguration.getCodeOwnerConfigValidationPolicyForCommitReceived( + project)) + .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE); + } + + @Test + public void setEnableValidationOnSubmit() throws Exception { + assertThat(codeOwnersPluginConfiguration.getCodeOwnerConfigValidationPolicyForSubmit(project)) + .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.enableValidationOnSubmit = CodeOwnerConfigValidationPolicy.FALSE; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.getCodeOwnerConfigValidationPolicyForSubmit(project)) + .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE); + + input.enableValidationOnSubmit = CodeOwnerConfigValidationPolicy.TRUE; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.getCodeOwnerConfigValidationPolicyForSubmit(project)) + .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE); + } + + @Test + public void setRejectNonResolvableCodeOwners() throws Exception { + assertThat(codeOwnersPluginConfiguration.rejectNonResolvableCodeOwners(project)).isTrue(); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.rejectNonResolvableCodeOwners = false; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.rejectNonResolvableCodeOwners(project)).isFalse(); + + input.rejectNonResolvableCodeOwners = true; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.rejectNonResolvableCodeOwners(project)).isTrue(); + } + + @Test + public void setRejectNonResolvableImports() throws Exception { + assertThat(codeOwnersPluginConfiguration.rejectNonResolvableImports(project)).isTrue(); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.rejectNonResolvableImports = false; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.rejectNonResolvableImports(project)).isFalse(); + + input.rejectNonResolvableImports = true; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.rejectNonResolvableImports(project)).isTrue(); + } + + @Test + public void setMaxPathsInChangeMessages() throws Exception { + assertThat(codeOwnersPluginConfiguration.getMaxPathsInChangeMessages(project)) + .isEqualTo(GeneralConfig.DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES); + + CodeOwnerProjectConfigInput input = new CodeOwnerProjectConfigInput(); + input.maxPathsInChangeMessages = 10; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.getMaxPathsInChangeMessages(project)).isEqualTo(10); + + input.maxPathsInChangeMessages = 0; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.getMaxPathsInChangeMessages(project)).isEqualTo(0); + + input.maxPathsInChangeMessages = GeneralConfig.DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES; + projectCodeOwnersApiFactory.project(project).updateConfig(input); + assertThat(codeOwnersPluginConfiguration.getMaxPathsInChangeMessages(project)) + .isEqualTo(GeneralConfig.DEFAULT_MAX_PATHS_IN_CHANGE_MESSAGES); + } + + @Test @UseClockStep public void checkCommitData() throws Exception { RevCommit head1 = projectOperations.project(project).getHead(RefNames.REFS_CONFIG);
diff --git a/resources/Documentation/about.md b/resources/Documentation/about.md index 6126097..d558de6 100644 --- a/resources/Documentation/about.md +++ b/resources/Documentation/about.md
@@ -16,3 +16,55 @@ for code owners is pretty generic and [configurable](config.html) so that it should be suitable for other teams as well. +## <a id="functionality">Functionality + +* Support for defining code owners: + * Code owners can be specified in `OWNERS` files that can appear in any + directory in the source branch. + * Default code owners can be specified on repository level by an `OWNERS` + file in the `refs/meta/config` branch. + * Global code owners across repositories can be configured. + * A fallback code owners policy controls who owns files that are not covered + by `OWNERS` files. + * Code owners can be specified by email (groups are not supported). + * Inheritance from parent directories is supported and can be disabled. + * Including an `OWNERS` file from other directories / branches / projects is + possible (only on the same host). + * File globs can be used. + * see [code owners documentation](config-guide.html#codeOwners) and + [OWNERS syntax](backend-find-owners.html#syntax) +<br><br> +* Prevents submitting changes without code owner approvals: + * Which votes count as code owner approvals is + [configurable](setup-guide.html#configureCodeOwnerApproval). + * Implemented as Java submit rule (no Prolog). +<br><br> +* Support for overrides: + * Privileged users can be allowed to override the code owner submit check. + * Overriding is done by voting on a [configured override + label](setup-guide.html#configureCodeOwnerOverrides). + * see [override setup](config-faqs.html#setupOverrides) +<br><br> +* UI extensions on change screen: + * Code owner suggestion + * Display of the code owners submit requirement + * Display of code owner statuses in the file list + * Change messages that list the owned paths. + * see [UI walkthrough](how-to-use.html) and [user guide](user-guide.html) +<br><br> +* Extensible: + * Supports multiple [backends](backends.html) which can implement different + syntaxes for `OWNERS` files. +<br><br> +* Validation: + * updates to `OWNERS` files are [validated](validation.html) on commit + received and submit + * `OWNERS` files can be validated on demand to detect consistency issues +<br><br> +* Rich REST API: + * see [REST API documentation](rest-api.html) +<br><br> +* Highly configurable: + * see [setup guide](setup-guide.html), [config-guide](config-guide.html), + [config FAQs](config-faqs.html) and [config documentation](config.html) +
diff --git a/resources/Documentation/alternative-plugins.md b/resources/Documentation/alternative-plugins.md index b335414..d3723a1 100644 --- a/resources/Documentation/alternative-plugins.md +++ b/resources/Documentation/alternative-plugins.md
@@ -1,10 +1,112 @@ # Alternative Plugins -Similar functionality is provided by the -[find-owners](https://gerrit-review.googlesource.com/admin/repos/plugins/find-owners) -plugin and the -[owners](https://gerrit-review.googlesource.com/admin/repos/plugins/owners) -plugin. +Similar functionality is provided by the following plugins: + +* [find-owners](#findOwners) plugin +* [owners](#owners) plugin + +## <a id="findOwners">find-owners plugin (deprecated) + +**Status:** deprecated, from Gerrit 3.4 on the `code-owners` plugin should be used instead\ +**Repository:** [plugins/find-owners](https://gerrit-review.googlesource.com/admin/repos/plugins/find-owners)\ +**Documentation:** [about](https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/resources/Documentation/about.md), [syntax](https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/resources/Documentation/syntax.md), [REST API](https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/resources/Documentation/rest-api.md), [config](https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/resources/Documentation/config.md) + +### <a id="findOwnersCompatibility">Compatibility with the code-owners plugin + +The [find-owners](backend-find-owners.html) backend in the`code-owners` plugin +supports the same syntax for `OWNERS` files as the `find-owners` plugin. This +means that existing `OWNERS` files continue to work with the `code-owners` +plugin and no migration for the `OWNERS` files is required. + +**IMPORTANT:** When migrating to the `code-owners` plugin, make sure that it is +correctly configured (see [setup guide](setup-guide.html)). + +**NOTE:** The REST API of the `code-owners` plugin is completely different than +the REST API of the `find-owners` plugin. This means callers of the REST API +must be adapted to the new API. + +**NOTE:** The `OWNERS` syntax in the `code-owners` plugin supports some +additional features. This means that `OWNERS` files that work with the +`code-owners` plugin may not work with the `find-owners` plugin. + +### <a id="findOwnersFunctionality">Functionality + +* Basic support for defining code owners: + * Code owners can be specified in `OWNERS` files that can appear in any + directory in the source branch. + * Code owners can be specified by email. + * Inheritance from parent directories is supported and can be disabled. + * Including an `OWNERS` file from another directory in the same project or + from another project on the same host is possible (same branch is assumed). + * File globs can be used. + * See [documentation](https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/resources/Documentation/syntax.md) for the supported syntax. +<br><br> +* Prolog rule to prevent submitting changes without owner approvals. + * A change can be exempted from owners approval by setting a footer in the + commit message. +<br><br> +* Basic UI: + * Supports to discover users that can grant owner approval on a change + (weighed suggestion) and add them as reviewer to the change. + * Missing owner approvals are visualized on a change. + * Owner approval is granted by voting on the `Code-Review` label. +<br><br> +* REST endpoints: + * [Action](https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/java/com/googlesource/gerrit/plugins/findowners/Action.java) REST endpoint: + * `GET /changes/<change-id>/revisions/<revision-id>/find-owners` + * returns a [RestResult](https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/java/com/googlesource/gerrit/plugins/findowners/RestResult.java) which contains: + * a file to list of owners map + * a list of owner infos with weight infos + * fields for debugging + * fields for change, patch set, current reviewers and changed files + * [GetOwners](https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/java/com/googlesource/gerrit/plugins/findowners/GetOwners.java) REST endpoint: + * `GET /changes/<change-id>/owners` + * Delegates to Action REST endpoint (see above) + * Also see [REST endpoint documentation](https://gerrit.googlesource.com/plugins/find-owners/+/master/src/main/resources/Documentation/rest-api.md) + +## <a id="owners">owners plugin + owners-autoassign plugin + +**Status:** maintained by the Gerrit open source community (no Google involvement)\ +**Repository:** [plugins/owners](https://gerrit-review.googlesource.com/admin/repos/plugins/owners)\ +**Documentation:** [readme](https://gerrit.googlesource.com/plugins/owners/+/master/README.md), [config & syntax](https://gerrit.googlesource.com/plugins/owners/+/master/owners/src/main/resources/Documentation/config.md) + +### <a id="ownersCompatibility">Compatibility with the code-owners plugin + +The `OWNERS` sytax that is used by the `owners` plugin is **not** compatible +with the `code-owners` plugin. This means any migration from the `owners` plugin +to the `code-owners` plugin (and vice versa) requires migrating all existing +`OWNERS` files. + +**NOTE:** It would be feasible to implement a new [backend](backends.html) in +the `code-owners` plugin that supports the syntax of the `owners` plugin +(contributions are welcome). + +**NOTE:** The `owners` plugin supports groups as code owners, which are not +supported by the `code-owners` plugin. + +### <a id="ownersFunctionality">Functionality + +* Basic support for defining code owners in the source branch and globally for + a repository: + * Code owners can be specified in `OWNERS` files that can appear in any + directory in the source branch. + * Code owners can be specified on repository level by an `OWNERS` file in + the `refs/meta/config` branch. + * Code owners can be specified by email or full name. + * Groups can be specified as code owners (by group name and group UUID). + * Inheritance from parent directories is supported and can be disabled. + * Regular expressions can be used. + * Syntax is based on YAML. + * See [documentation](https://gerrit.googlesource.com/plugins/owners/+/master/owners/src/main/resources/Documentation/config.md) for the supported syntax. +<br><br> +* Prolog rule to prevent submitting changes without code owner approvals. + * The label on which code owners must vote is configurable. +<br><br> +* The UI visualizes whose approval is needed for submit. + * Implemented by the `need` description of the Prolog rule, which will be + shown in the UI (the plugin doesn't contain any UI code) +<br><br> +* Auto-adding of code owners as reviewers. ---
diff --git a/resources/Documentation/config-faqs.md b/resources/Documentation/config-faqs.md index 4a86c2e..e6971d2 100644 --- a/resources/Documentation/config-faqs.md +++ b/resources/Documentation/config-faqs.md
@@ -5,6 +5,8 @@ * [How to avoid issues with code owner config files](#avoidIssuesWithCodeOwnerConfigs) * [How to investigate issues with code owner config files](#investigateIssuesWithCodeOwnerConfigs) * [How to setup code owner overrides](#setupOverrides) +* [What's the best place to keep the global plugin + configuration](#globalPluginConfiguration) ## <a id="updateCodeOwnersConfig">How to update the code-owners.config file for a project @@ -132,6 +134,25 @@ Alternatively the permissions can also be assigned via the [Set Access REST endpoint](../../../Documentation/rest-api-projects.html#set-access). +## <a id="globalPluginConfiguration">What's the best place to keep the global plugin configuration + +The global plugin configuration can either be maintained in the +[gerrit.config](config.html) file or in the +[code-owners.config](config.html#projectLevelConfigFile) file in the +`refs/meta/config` branch of the `All-Projects` project. From the perspective of +the code-owners plugin both places are equally good. However which place is +preferred can depend on the system setup, e.g. changes to `gerrit.config` may be +harder to do and require a multi-day rollout, whereas changes of the +`All-Projects` configuration can be done through the [REST +API](rest-api.html#update-code-owner-project-config) and are always instant. + +**NOTE:** Any configuration that is done in `All-Projects` overrides the +corresponding configuration that is inherited from `gerrit.config`. + +**NOTE:** There are a few configuration parameters (e.g. for [allowed email +domains](config.html#pluginCodeOwnersAllowedEmailDomain)) that cannot be set on +project level and hence must be set in `gerrit.config`. + --- Back to [@PLUGIN@ documentation index](index.html)
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md index 7455898..9c99988 100644 --- a/resources/Documentation/config.md +++ b/resources/Documentation/config.md
@@ -146,8 +146,8 @@ <a id="pluginCodeOwnersEnableValidationOnSubmit">plugin.@PLUGIN@.enableValidationOnSubmit</a> : Policy for validating code owner config files when a change is submitted. Allowed values are `true` (the code owner config file - validation is enabled and the submit of invalid code owner config files - is rejected), `false` (the code owner config file validation is + validation is enabled and the submission of invalid code owner config + files is rejected), `false` (the code owner config file validation is disabled, invalid code owner config files are not rejected) and `dry_run` (code owner config files are validated, but invalid code owner config files are not rejected).\
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md new file mode 100644 index 0000000..2e63c23 --- /dev/null +++ b/resources/Documentation/metrics.md
@@ -0,0 +1,27 @@ +# Metrics + +The @PLUGIN@ plugin exports several metrics which can give insights into the +usage and performance of the code owners functionality. + +## <a id="latencyMetrics"> Latency Metrics + +* `compute_changed_files`: + Latency for computing changed files. +* `compute_file_statuses`: + Latency for computing file statuses. +* `compute_file_statuses_for_account`: + Latency for computing file statuses for an account. +* `resolve_code_owner_config`: + Latency for resolving a code owner config file. +* `resolve_code_owner_config_import`: + Latency for resolving an import of a code owner config file. +* `resolve_code_owner_config_imports`: + Latency for resolving all imports of a code owner config file. +* `run_code_owner_submit_rule`: + Latency for running the code owner submit rule. + +--- + +Back to [@PLUGIN@ documentation index](index.html) + +Part of [Gerrit Code Review](../../../Documentation/index.html)
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md index 899060d..3fc947a 100644 --- a/resources/Documentation/rest-api.md +++ b/resources/Documentation/rest-api.md
@@ -840,6 +840,16 @@ | `required_approval` | optional | The approval that is required from code owners. Must be specified in the format "\<label-name\>+\<label-value\>". If an empty string is provided the required approval configuration is unset. Unsetting the required approval means that the inherited required approval configuration or the default required approval (`Code-Review+1`) will apply. In contrast to providing an empty string, providing `null` (or not setting the value) means that the required approval configuration is not updated. | `override_approvals` | optional | The approvals that count as override for the code owners submit check. Must be specified in the format "\<label-name\>+\<label-value\>". | `fallback_code_owners` | optional | Policy that controls who should own paths that have no code owners defined. +| `global_code_owners` | optional | List of emails of users that should be code owners globally across all branches. +| `merge_commit_strategy` | optional | Strategy that defines for merge commits which files require code owner approvals. Can be `ALL_CHANGED_FILES` or `FILES_WITH_CONFLICT_RESOLUTION` (see [mergeCommitStrategy](config.html#pluginCodeOwnersMergeCommitStrategy) for an explanation of these values). +| `implicit_approvals` | optional | Whether an implicit code owner approval from the last uploader is assumed. +| `override_info_url` | optional | URL for a page that provides project/host-specific information about how to request a code owner override. +| `read_only` | optional | Whether code owner config files are read-only. +| `enable_validation_on_commit_received` | optional | Policy for validating code owner config files when a commit is received. Allowed values are `true` (the code owner config file validation is enabled and the upload of invalid code owner config files is rejected), `false` (the code owner config file validation is disabled, invalid code owner config files are not rejected) and `dry_run` (code owner config files are validated, but invalid code owner config files are not rejected). +| `enable_validation_on_submit` | optional | Policy for validating code owner config files when a change is submitted. Allowed values are `true` (the code owner config file validation is enabled and the submission of invalid code owner config files is rejected), `false` (the code owner config file validation is disabled, invalid code owner config files are not rejected) and `dry_run` (code owner config files are validated, but invalid code owner config files are not rejected). +| `reject_non_resolvable_code_owners` | optional | Whether modifications of code owner config files that newly add non-resolvable code owners should be rejected on commit received and submit. +| `reject_non_resolvable_imports` | optional | Whether modifications of code owner config files that newly add non-resolvable imports should be rejected on commit received an submit. +| `max_paths_in_change_messages` | optional | The maximum number of paths that are included in change messages. Setting the value to `0` disables including owned paths into change messages. ---
diff --git a/resources/Documentation/user-guide.md b/resources/Documentation/user-guide.md index 70dfaeb..d4308a1 100644 --- a/resources/Documentation/user-guide.md +++ b/resources/Documentation/user-guide.md
@@ -102,6 +102,14 @@ [ignoreSelfApproval](../../../Documentation/config-labels.html#label_ignoreSelfApproval) enabled, code owner approvals of the patch set uploader are ignored. +**NOTE:** Code owner approvals are always applied on the whole change / patch +set and count for all files in the change / patch set. It is not possible to +approve individual files only. This means code owners should always review all +files in the change / patch set before applying their approval. E.g. it is +discouraged to only review the owned files, since the set of owned files can +change if `OWNERS` files in the destination branch are changed after the +approval has been applied. + ## <a id="codeOwnerOverride">Code owner override Usually some privileged users, such as sheriffs, are allowed to override the
diff --git a/ui/code-owners-service.js b/ui/code-owners-service.js index cf9f1f5..13f18d7 100644 --- a/ui/code-owners-service.js +++ b/ui/code-owners-service.js
@@ -60,6 +60,25 @@ OTHER: 'OTHER', }; +// TODO: Try to remove it. The ResponseError and getErrorMessage duplicates +// code from the gr-plugin-rest-api.ts. This code is required because +// we want custom error processing in some functions. For details see +// the original gr-plugin-rest-api.ts file/ + +class ResponseError extends Error { + constructor(response) { + super(); + this.response = response; + } +} + +async function getErrorMessage(response) { + const text = await response.text(); + return text ? + `${response.status}: ${text}` : + `${response.status}`; +} + /** * Responsible for communicating with the rest-api * @@ -118,19 +137,42 @@ * @param {string} branch */ async getBranchConfig(project, branch) { - const config = await this.restApi.get( - `/projects/${encodeURIComponent(project)}/` + - `branches/${encodeURIComponent(branch)}/` + - `code_owners.branch_config` - ); - if (config.override_approval && !(config.override_approval instanceof Array)) { - // In the upcoming backend changes, the override_approval will be changed - // to array with (possible) multiple items. - // While this transition is in progress, the frontend supports both API - - // the old one and the new one. - return {...config, override_approval: [config.override_approval]}; + const errFn = (response, error) => { + if (error) throw error; + if (response) throw new ResponseError(response); + throw new Error('Generic REST API error'); } - return config; + try { + const config = await this.restApi.send( + 'GET', + `/projects/${encodeURIComponent(project)}/` + + `branches/${encodeURIComponent(branch)}/` + + `code_owners.branch_config`, + undefined, + errFn + ); + if (config.override_approval && !(config.override_approval + instanceof Array)) { + // In the upcoming backend changes, the override_approval will be changed + // to array with (possible) multiple items. + // While this transition is in progress, the frontend supports both API - + // the old one and the new one. + return {...config, override_approval: [config.override_approval]}; + } + return config; + } catch(err) { + if (err instanceof ResponseError) { + if (err.response.status === 404) { + // The 404 error means that the branch doesn't exist and + // the plugin should be disabled. + return {disabled: true}; + } + return getErrorMessage(err.response).then(msg => { + throw new Error(msg); + }); + } + throw err; + } } }