Merge "CodeOwnerApprovalCheck: Allow to use diff cache to get changed files"
diff --git a/BUILD b/BUILD
index 7c7f9e0..5333141 100644
--- a/BUILD
+++ b/BUILD
@@ -9,9 +9,6 @@
"//tools/bzl:plugin.bzl",
"gerrit_plugin",
)
-load("//tools/bzl:genrule2.bzl", "genrule2")
-load("//tools/bzl:js.bzl", "polygerrit_plugin")
-load("@npm//@bazel/rollup:index.bzl", "rollup_bundle")
gerrit_plugin(
name = "code-owners",
@@ -22,7 +19,7 @@
"Gerrit-HttpModule: com.google.gerrit.plugins.codeowners.module.HttpModule",
"Gerrit-BatchModule: com.google.gerrit.plugins.codeowners.module.BatchModule",
],
- resource_jars = [":code-owners-fe-static"],
+ resource_jars = ["//plugins/code-owners/ui:code-owners"],
resource_strip_prefix = "plugins/code-owners/resources",
resources = glob(["resources/**/*"]),
deps = [
@@ -33,35 +30,3 @@
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/validation",
],
)
-
-polygerrit_plugin(
- name = "code-owners-fe",
- app = "plugin-bundle.js",
- plugin_name = "code-owners",
-)
-
-rollup_bundle(
- name = "plugin-bundle",
- srcs = glob([
- "ui/**/*.js",
- ]),
- entry_point = "ui/plugin.js",
- format = "iife",
- rollup_bin = "//tools/node_tools:rollup-bin",
- sourcemap = "hidden",
- deps = [
- "@tools_npm//rollup-plugin-node-resolve",
- ],
-)
-
-genrule2(
- name = "code-owners-fe-static",
- srcs = [":code-owners-fe"],
- outs = ["code-owners-fe-static.jar"],
- cmd = " && ".join([
- "mkdir $$TMP/static",
- "cp -r $(locations :code-owners-fe) $$TMP/static",
- "cd $$TMP",
- "zip -Drq $$ROOT/$@ -g .",
- ]),
-)
diff --git a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
index 13c3b3e..1309568 100644
--- a/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
+++ b/java/com/google/gerrit/plugins/codeowners/acceptance/AbstractCodeOwnersTest.java
@@ -200,6 +200,7 @@
protected void createOwnersOverrideLabel(String labelName) throws RestApiException {
LabelDefinitionInput input = new LabelDefinitionInput();
+ input.function = "NoOp";
input.values = ImmutableMap.of("+1", "Override", " 0", "No Override");
gApi.projects().name(project.get()).label(labelName).create(input).get();
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java
index 655ef66..a2079b8 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwners.java
@@ -49,6 +49,7 @@
private Long seed;
private Boolean resolveAllUsers;
private Boolean highestScoreOnly;
+ private Boolean debug;
/**
* Lists the code owners for the given path.
@@ -128,6 +129,18 @@
}
/**
+ * Sets whether debug logs should be included into the response.
+ *
+ * <p>Requires the 'Check Code Owner' global capability.
+ *
+ * @param debug whether debug logs should be included into the response
+ */
+ public QueryRequest withDebug(boolean debug) {
+ this.debug = debug;
+ return this;
+ }
+
+ /**
* Sets the branch revision from which the code owner configs should be read.
*
* <p>Not supported for querying code owners for a path in a change.
@@ -166,6 +179,11 @@
return Optional.ofNullable(highestScoreOnly);
}
+ /** Whether debug logs should be included into the response. */
+ public Optional<Boolean> getDebug() {
+ return Optional.ofNullable(debug);
+ }
+
/** Returns the branch revision from which the code owner configs should be read. */
public Optional<String> getRevision() {
return Optional.ofNullable(revision);
diff --git a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnersInfo.java b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnersInfo.java
index 2d3a84c..d699029 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/CodeOwnersInfo.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/CodeOwnersInfo.java
@@ -33,4 +33,11 @@
* <p>Not set if {@code false}.
*/
public Boolean ownedByAllUsers;
+
+ /**
+ * Debug logs that may help to understand why a user is or isn't suggested as a code owner.
+ *
+ * <p>Only set if requested via {@code --debug}.
+ */
+ public List<String> debugLogs;
}
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java
index 107fa61..fff0b9c 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInBranchImpl.java
@@ -60,6 +60,7 @@
getSeed().ifPresent(getCodeOwners::setSeed);
getResolveAllUsers().ifPresent(getCodeOwners::setResolveAllUsers);
getHighestScoreOnly().ifPresent(getCodeOwners::setHighestScoreOnly);
+ getDebug().ifPresent(getCodeOwners::setDebug);
getRevision().ifPresent(getCodeOwners::setRevision);
CodeOwnersInBranchCollection.PathResource pathInBranchResource =
codeOwnersInBranchCollection.parse(
diff --git a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java
index b185172..97a5715 100644
--- a/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java
+++ b/java/com/google/gerrit/plugins/codeowners/api/impl/CodeOwnersInChangeImpl.java
@@ -65,6 +65,7 @@
getSeed().ifPresent(getCodeOwners::setSeed);
getResolveAllUsers().ifPresent(getCodeOwners::setResolveAllUsers);
getHighestScoreOnly().ifPresent(getCodeOwners::setHighestScoreOnly);
+ getDebug().ifPresent(getCodeOwners::setDebug);
CodeOwnersInChangeCollection.PathResource pathInChangeResource =
codeOwnersInChangeCollection.parse(
revisionResource, IdString.fromDecoded(path.toString()));
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
index 7f07943..e69616d 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRule.java
@@ -146,10 +146,10 @@
}
}
errorMessage += ".";
- logger.atSevere().withCause(t).log(errorMessage);
codeOwnerMetrics.countCodeOwnerSubmitRuleErrors.increment(cause);
if (isRuleError) {
+ logger.atWarning().log(errorMessage);
return Optional.of(ruleError(errorMessage));
}
throw new CodeOwnersInternalServerErrorException(errorMessage, t);
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
index 11b9b6b..8272bb6 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/PathCodeOwners.java
@@ -222,6 +222,9 @@
"resolve code owners for %s from code owner config %s", path, codeOwnerConfig.key());
List<String> messages = new ArrayList<>();
+ messages.add(
+ String.format(
+ "resolve code owners for %s from code owner config %s", path, codeOwnerConfig.key()));
// Create a code owner config builder to create the resolved code owner config (= code owner
// config that is scoped to the path and which has imports resolved)
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
index 0d0b44d..234e4a0 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginProjectConfigSnapshot.java
@@ -47,7 +47,7 @@
import java.util.Optional;
import org.eclipse.jgit.lib.Config;
-/** Snapshot of the code-owners plugin configuration for one project. */
+/** Snapshot of the project-specific code-owners plugin configuration. */
public class CodeOwnersPluginProjectConfigSnapshot {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
@@ -120,7 +120,7 @@
return fileExtension;
}
- /** Checks whether code owner configs are read-only. */
+ /** Whether code owner configs are read-only. */
public boolean areCodeOwnerConfigsReadOnly() {
if (codeOwnerConfigsReadOnly == null) {
codeOwnerConfigsReadOnly = generalConfig.getReadOnly(projectName, pluginConfig);
@@ -128,9 +128,7 @@
return codeOwnerConfigsReadOnly;
}
- /**
- * Checks whether pure revert changes are exempted from needing code owner approvals for submit.
- */
+ /** Whether pure revert changes are exempted from needing code owner approvals for submit. */
public boolean arePureRevertsExempted() {
if (exemptPureReverts == null) {
exemptPureReverts = generalConfig.getExemptPureReverts(projectName, pluginConfig);
@@ -139,7 +137,7 @@
}
/**
- * Checks whether newly added non-resolvable code owners should be rejected on commit received and
+ * Whether newly added non-resolvable code owners should be rejected on commit received and
* submit.
*
* @param branchName the branch for which it should be checked whether non-resolvable code owners
@@ -166,8 +164,7 @@
}
/**
- * Checks whether newly added non-resolvable imports should be rejected on commit received and
- * submit.
+ * Whether newly added non-resolvable imports should be rejected on commit received and submit.
*
* @param branchName the branch for which it should be checked whether non-resolvable imports
* should be rejected
@@ -335,9 +332,6 @@
/**
* Whether the code owners functionality is disabled for the given branch.
*
- * <p>Callers must ensure that the project of the specified branch exists. If the project doesn't
- * exist the call fails with {@link IllegalStateException}.
- *
* <p>The configuration is evaluated in the following order:
*
* <ul>
@@ -540,14 +534,15 @@
* <p>If multiple approvals are returned, any of them is sufficient to override the code owners
* submit check.
*
- * <p>The override approval configuration is evaluated in the following order:
+ * <p>The override approval configuration is read from:
*
* <ul>
- * <li>override approval configuration for project (with inheritance)
- * <li>globally configured override approval
+ * <li>the override approval configuration for project (with inheritance)
+ * <li>the globally configured override approval
* </ul>
*
- * <p>The first override approval configuration that exists counts and the evaluation is stopped.
+ * <p>Override approvals that are configured on project-level extend the inherited override
+ * approval configuration.
*
* <p>The returned override approvals are sorted alphabetically by their string representation
* (e.g. {@code Owners-Override+1}).
@@ -608,7 +603,7 @@
* Gets the required approvals that are configured.
*
* @param requiredApprovalConfig the config from which the required approvals should be read
- * @return the required approvals that is configured, an empty list if no required approvals are
+ * @return the required approvals that are configured, an empty list if no required approvals are
* configured
*/
private ImmutableList<RequiredApproval> getConfiguredRequiredApproval(
diff --git a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
index cdb8174..e5588bf 100644
--- a/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
+++ b/java/com/google/gerrit/plugins/codeowners/metrics/CodeOwnerMetrics.java
@@ -62,6 +62,7 @@
public final Counter0 countCodeOwnerConfigCacheReads;
public final Counter1<String> countCodeOwnerSubmitRuleErrors;
public final Counter0 countCodeOwnerSubmitRuleRuns;
+ public final Counter1<Boolean> countCodeOwnerSuggestions;
public final Counter3<String, String, String> countInvalidCodeOwnerConfigFiles;
private final MetricMaker metricMaker;
@@ -172,6 +173,15 @@
this.countCodeOwnerSubmitRuleRuns =
createCounter(
"count_code_owner_submit_rule_runs", "Total number of code owner submit rule runs");
+ this.countCodeOwnerSuggestions =
+ createCounter1(
+ "count_code_owner_suggestions",
+ "Total number of code owner suggestions",
+ Field.ofBoolean("resolve_all_users", (metadataBuilder, resolveAllUsers) -> {})
+ .description(
+ "Whether code ownerships that are assigned to all users are resolved to random"
+ + " users.")
+ .build());
this.countInvalidCodeOwnerConfigFiles =
createCounter3(
"count_invalid_code_owner_config_files",
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
index d1e6cd2..85db8e3 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/AbstractGetCodeOwnersForPath.java
@@ -42,6 +42,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScorings;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.AccountDirectory.FillOptions;
import com.google.gerrit.server.account.AccountLoader;
@@ -79,6 +80,8 @@
private final Accounts accounts;
private final AccountControl.Factory accountControlFactory;
private final PermissionBackend permissionBackend;
+ private final CheckCodeOwnerCapability checkCodeOwnerCapability;
+ private final CodeOwnerMetrics codeOwnerMetrics;
private final CodeOwnersPluginConfiguration codeOwnersPluginConfiguration;
private final CodeOwnerConfigHierarchy codeOwnerConfigHierarchy;
private final Provider<CodeOwnerResolver> codeOwnerResolver;
@@ -90,6 +93,7 @@
private Optional<Long> seed = Optional.empty();
private boolean resolveAllUsers;
private boolean highestScoreOnly;
+ private boolean debug;
@Option(
name = "-o",
@@ -138,11 +142,22 @@
this.highestScoreOnly = highestScoreOnly;
}
+ @Option(
+ name = "--debug",
+ usage =
+ "whether debug logs should be included into the response"
+ + " (requires the 'Check Code Owner' global capability)")
+ public void setDebug(boolean debug) {
+ this.debug = debug;
+ }
+
protected AbstractGetCodeOwnersForPath(
AccountVisibility accountVisibility,
Accounts accounts,
AccountControl.Factory accountControlFactory,
PermissionBackend permissionBackend,
+ CheckCodeOwnerCapability checkCodeOwnerCapability,
+ CodeOwnerMetrics codeOwnerMetrics,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
Provider<CodeOwnerResolver> codeOwnerResolver,
@@ -151,6 +166,8 @@
this.accounts = accounts;
this.accountControlFactory = accountControlFactory;
this.permissionBackend = permissionBackend;
+ this.checkCodeOwnerCapability = checkCodeOwnerCapability;
+ this.codeOwnerMetrics = codeOwnerMetrics;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.codeOwnerConfigHierarchy = codeOwnerConfigHierarchy;
this.codeOwnerResolver = codeOwnerResolver;
@@ -164,10 +181,16 @@
parseHexOptions();
validateLimit();
+ if (debug) {
+ permissionBackend.currentUser().check(checkCodeOwnerCapability.getPermission());
+ }
+
if (!seed.isPresent()) {
seed = getDefaultSeed(rsrc);
}
+ codeOwnerMetrics.countCodeOwnerSuggestions.increment(resolveAllUsers);
+
// The distance that applies to code owners that are defined in the root code owner
// configuration.
int rootDistance = rsrc.getPath().getNameCount();
@@ -180,6 +203,7 @@
Set<CodeOwner> codeOwners = new HashSet<>();
AtomicBoolean ownedByAllUsers = new AtomicBoolean(false);
+ List<String> debugLogs = new ArrayList<>();
codeOwnerConfigHierarchy.visit(
rsrc.getBranch(),
rsrc.getRevision(),
@@ -187,6 +211,11 @@
codeOwnerConfig -> {
CodeOwnerResolverResult pathCodeOwners =
codeOwnerResolver.get().resolvePathCodeOwners(codeOwnerConfig, rsrc.getPath());
+
+ if (debug) {
+ debugLogs.addAll(pathCodeOwners.messages());
+ }
+
codeOwners.addAll(filterCodeOwners(rsrc, pathCodeOwners.codeOwners()));
int distance =
@@ -224,6 +253,12 @@
if (codeOwners.size() < limit || !ownedByAllUsers.get()) {
CodeOwnerResolverResult globalCodeOwners = getGlobalCodeOwners(rsrc.getBranch().project());
+
+ if (debug) {
+ debugLogs.add("resolve global code owners");
+ debugLogs.addAll(globalCodeOwners.messages());
+ }
+
globalCodeOwners
.codeOwners()
.forEach(
@@ -261,6 +296,7 @@
codeOwnersInfo.codeOwners =
codeOwnerJsonFactory.create(getFillOptions()).format(sortedAndLimitedCodeOwners);
codeOwnersInfo.ownedByAllUsers = ownedByAllUsers.get() ? true : null;
+ codeOwnersInfo.debugLogs = debug ? debugLogs : null;
return Response.ok(codeOwnersInfo);
}
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/BUILD b/java/com/google/gerrit/plugins/codeowners/restapi/BUILD
index e938a4c..298b46b 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/BUILD
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/BUILD
@@ -9,6 +9,7 @@
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/api",
"//plugins/code-owners/java/com/google/gerrit/plugins/codeowners/backend",
"//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/java/com/google/gerrit/plugins/codeowners/validation",
"//plugins/code-owners/proto:owners_metadata_java_proto",
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
index e4099c6..446b91e 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInBranch.java
@@ -26,6 +26,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigHierarchy;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.change.IncludedInResolver;
@@ -74,6 +75,8 @@
Accounts accounts,
AccountControl.Factory accountControlFactory,
PermissionBackend permissionBackend,
+ CheckCodeOwnerCapability checkCodeOwnerCapability,
+ CodeOwnerMetrics codeOwnerMetrics,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
Provider<CodeOwnerResolver> codeOwnerResolver,
@@ -84,6 +87,8 @@
accounts,
accountControlFactory,
permissionBackend,
+ checkCodeOwnerCapability,
+ codeOwnerMetrics,
codeOwnersPluginConfiguration,
codeOwnerConfigHierarchy,
codeOwnerResolver,
diff --git a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
index e6a2f0d..e2aac82 100644
--- a/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
+++ b/java/com/google/gerrit/plugins/codeowners/restapi/GetCodeOwnersForPathInChange.java
@@ -30,6 +30,7 @@
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScore;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerScoring;
import com.google.gerrit.plugins.codeowners.backend.config.CodeOwnersPluginConfiguration;
+import com.google.gerrit.plugins.codeowners.metrics.CodeOwnerMetrics;
import com.google.gerrit.server.account.AccountControl;
import com.google.gerrit.server.account.Accounts;
import com.google.gerrit.server.account.ServiceUserClassifier;
@@ -62,6 +63,8 @@
Accounts accounts,
AccountControl.Factory accountControlFactory,
PermissionBackend permissionBackend,
+ CheckCodeOwnerCapability checkCodeOwnerCapability,
+ CodeOwnerMetrics codeOwnerMetrics,
CodeOwnersPluginConfiguration codeOwnersPluginConfiguration,
CodeOwnerConfigHierarchy codeOwnerConfigHierarchy,
Provider<CodeOwnerResolver> codeOwnerResolver,
@@ -72,6 +75,8 @@
accounts,
accountControlFactory,
permissionBackend,
+ checkCodeOwnerCapability,
+ codeOwnerMetrics,
codeOwnersPluginConfiguration,
codeOwnerConfigHierarchy,
codeOwnerResolver,
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
index 791705c..31fa4e0 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnerCheckInfoSubject.java
@@ -145,10 +145,6 @@
}
}
- public IterableSubject hasDebugLogsThat() {
- return check("debugLogs").that(codeOwnerCheckInfo().debugLogs);
- }
-
private CodeOwnerCheckInfo codeOwnerCheckInfo() {
isNotNull();
return codeOwnerCheckInfo;
diff --git a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnersInfoSubject.java b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnersInfoSubject.java
index 28ea59e..11433e2 100644
--- a/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnersInfoSubject.java
+++ b/java/com/google/gerrit/plugins/codeowners/testing/CodeOwnersInfoSubject.java
@@ -20,6 +20,7 @@
import com.google.common.truth.BooleanSubject;
import com.google.common.truth.FailureMetadata;
+import com.google.common.truth.IterableSubject;
import com.google.common.truth.Subject;
import com.google.gerrit.plugins.codeowners.api.CodeOwnerInfo;
import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo;
@@ -58,6 +59,16 @@
return check("ownedByAllUsers").that(codeOwnersInfo().ownedByAllUsers);
}
+ public void hasDebugLogsThatContainAllOf(String... expectedMessages) {
+ for (String expectedMessage : expectedMessages) {
+ check("debugLogs").that(codeOwnersInfo().debugLogs).contains(expectedMessage);
+ }
+ }
+
+ public IterableSubject hasDebugLogsThat() {
+ return check("debugLogs").that(codeOwnersInfo().debugLogs);
+ }
+
private CodeOwnersInfo codeOwnersInfo() {
isNotNull();
return codeOwnersInfo;
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index 7ec5b1f..24be1ce 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -22,6 +22,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Streams;
import com.google.common.flogger.FluentLogger;
@@ -29,6 +30,7 @@
import com.google.gerrit.entities.PatchSet;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.plugins.codeowners.backend.ChangedFiles;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerBackend;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
@@ -137,6 +139,7 @@
private final ChangeNotes.Factory changeNotesFactory;
private final PatchSetUtil patchSetUtil;
private final IdentifiedUser.GenericFactory userFactory;
+ private final SkipCodeOwnerConfigValidationPushOption skipCodeOwnerConfigValidationPushOption;
@Inject
CodeOwnerConfigValidator(
@@ -149,7 +152,8 @@
ProjectCache projectCache,
ChangeNotes.Factory changeNotesFactory,
PatchSetUtil patchSetUtil,
- IdentifiedUser.GenericFactory userFactory) {
+ IdentifiedUser.GenericFactory userFactory,
+ SkipCodeOwnerConfigValidationPushOption skipCodeOwnerConfigValidationPushOption) {
this.pluginName = pluginName;
this.codeOwnersPluginConfiguration = codeOwnersPluginConfiguration;
this.repoManager = repoManager;
@@ -160,6 +164,7 @@
this.changeNotesFactory = changeNotesFactory;
this.patchSetUtil = patchSetUtil;
this.userFactory = userFactory;
+ this.skipCodeOwnerConfigValidationPushOption = skipCodeOwnerConfigValidationPushOption;
}
@Override
@@ -197,7 +202,8 @@
receiveEvent.revWalk,
receiveEvent.commit,
receiveEvent.user,
- codeOwnerConfigValidationPolicy.isForced());
+ codeOwnerConfigValidationPolicy.isForced(),
+ receiveEvent.pushOptions);
} catch (RuntimeException e) {
if (!codeOwnerConfigValidationPolicy.isDryRun()) {
throw e;
@@ -272,7 +278,8 @@
revWalk,
commit,
patchSetUploader,
- codeOwnerConfigValidationPolicy.isForced());
+ codeOwnerConfigValidationPolicy.isForced(),
+ /* pushOptions= */ ImmutableListMultimap.of());
} catch (RuntimeException e) {
if (!codeOwnerConfigValidationPolicy.isDryRun()) {
throw e;
@@ -314,7 +321,8 @@
RevWalk revWalk,
RevCommit revCommit,
IdentifiedUser user,
- boolean force) {
+ boolean force,
+ ImmutableListMultimap<String, String> pushOptions) {
CodeOwnersPluginProjectConfigSnapshot codeOwnersConfig =
codeOwnersPluginConfiguration.getProjectConfig(branchNameKey.project());
logger.atFine().log("force = %s", force);
@@ -326,6 +334,36 @@
new CommitValidationMessage(
"code-owners functionality is disabled", ValidationMessage.Type.HINT)));
}
+
+ try {
+ if (skipCodeOwnerConfigValidationPushOption.skipValidation(pushOptions)) {
+ logger.atFine().log("skip validation requested");
+ return Optional.of(
+ ValidationResult.create(
+ pluginName,
+ "skipping validation of code owner config files",
+ new CommitValidationMessage(
+ String.format(
+ "the validation is skipped due to the --%s~%s push option",
+ pluginName, SkipCodeOwnerConfigValidationPushOption.NAME),
+ ValidationMessage.Type.HINT)));
+ }
+ } catch (AuthException e) {
+ logger.atFine().withCause(e).log("Not allowed to skip code owner config validation");
+ return Optional.of(
+ ValidationResult.create(
+ pluginName,
+ "skipping code owner config validation not allowed",
+ new CommitValidationMessage(e.getMessage(), ValidationMessage.Type.ERROR)));
+ } catch (SkipCodeOwnerConfigValidationPushOption.InvalidValueException e) {
+ logger.atFine().log(e.getMessage());
+ return Optional.of(
+ ValidationResult.create(
+ pluginName,
+ "invalid push option",
+ new CommitValidationMessage(e.getMessage(), ValidationMessage.Type.ERROR)));
+ }
+
if (codeOwnersConfig.areCodeOwnerConfigsReadOnly()) {
return Optional.of(
ValidationResult.create(
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationCapability.java b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationCapability.java
new file mode 100644
index 0000000..1fcb717
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationCapability.java
@@ -0,0 +1,46 @@
+// 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.validation;
+
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.extensions.api.access.PluginPermission;
+import com.google.gerrit.extensions.config.CapabilityDefinition;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+/**
+ * Global capability that allows a user to skip the code owner config validation on push via the
+ * {@code code-owners~skip-validation} push option.
+ */
+@Singleton
+public class SkipCodeOwnerConfigValidationCapability extends CapabilityDefinition {
+ public static final String ID = "canSkipCodeOwnerConfigValidation";
+
+ private final String pluginName;
+
+ @Inject
+ SkipCodeOwnerConfigValidationCapability(@PluginName String pluginName) {
+ this.pluginName = pluginName;
+ }
+
+ @Override
+ public String getDescription() {
+ return "Can Skip Code Owner Config Validation";
+ }
+
+ public PluginPermission getPermission() {
+ return new PluginPermission(pluginName, ID, /* fallBackToAdmin= */ true);
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
new file mode 100644
index 0000000..bc7ac6b
--- /dev/null
+++ b/java/com/google/gerrit/plugins/codeowners/validation/SkipCodeOwnerConfigValidationPushOption.java
@@ -0,0 +1,127 @@
+// 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.validation;
+
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableListMultimap;
+import com.google.gerrit.extensions.annotations.PluginName;
+import com.google.gerrit.extensions.restapi.AuthException;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnersInternalServerErrorException;
+import com.google.gerrit.server.git.receive.PluginPushOption;
+import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
+
+/** Push option that allows to skip the code owner config validation. */
+@Singleton
+public class SkipCodeOwnerConfigValidationPushOption implements PluginPushOption {
+ public static final String NAME = "skip-validation";
+
+ private static final String DESCRIPTION = "skips the code owner config validation";
+
+ private final String pluginName;
+ private final PermissionBackend permissionBackend;
+ private final SkipCodeOwnerConfigValidationCapability skipCodeOwnerConfigValidationCapability;
+
+ @Inject
+ SkipCodeOwnerConfigValidationPushOption(
+ @PluginName String pluginName,
+ PermissionBackend permissionBackend,
+ SkipCodeOwnerConfigValidationCapability skipCodeOwnerConfigValidationCapability) {
+ this.pluginName = pluginName;
+ this.permissionBackend = permissionBackend;
+ this.skipCodeOwnerConfigValidationCapability = skipCodeOwnerConfigValidationCapability;
+ }
+
+ @Override
+ public String getName() {
+ return NAME;
+ }
+
+ @Override
+ public String getDescription() {
+ return DESCRIPTION;
+ }
+
+ /**
+ * Whether the code owner config validation should be skipped.
+ *
+ * <p>Only returns {@code true} if the {@code --code-owners~skip-validation} push option was
+ * specified and the calling user is allowed to skip the code owner config validation (requires
+ * the {@link SkipCodeOwnerConfigValidationCapability}).
+ *
+ * @param pushOptions the push options that have been specified on the push
+ * @return {@code true} if the {@code --code-owners~skip-validation} push option was specified and
+ * the calling user is allowed to skip the code owner config validation
+ * @throws InvalidValueException if the {@code --code-owners~skip-validation} push option was
+ * specified with an invalid value or if the {@code --code-owners~skip-validation} push option
+ * was specified multiple times
+ * @throws AuthException thrown if the {@code --code-owners~skip-validation} push option was
+ * specified, but the calling user is not allowed to skip the code owner config validation
+ */
+ public boolean skipValidation(ImmutableListMultimap<String, String> pushOptions)
+ throws InvalidValueException, AuthException {
+ String qualifiedName = pluginName + "~" + NAME;
+ if (!pushOptions.containsKey(qualifiedName)) {
+ return false;
+ }
+ ImmutableList<String> values = pushOptions.get(qualifiedName);
+ if (values.size() != 1) {
+ throw new InvalidValueException(values);
+ }
+
+ String value = values.get(0);
+ if (Boolean.parseBoolean(value) || value.isEmpty()) {
+ canSkipCodeOwnerConfigValidation();
+ return true;
+ }
+
+ if (value.equalsIgnoreCase(Boolean.FALSE.toString())) {
+ return false;
+ }
+
+ // value was neither 'true', 'false' nor empty
+ throw new InvalidValueException(values);
+ }
+
+ private void canSkipCodeOwnerConfigValidation() throws AuthException {
+ try {
+ permissionBackend
+ .currentUser()
+ .check(skipCodeOwnerConfigValidationCapability.getPermission());
+ } catch (PermissionBackendException e) {
+ throw new CodeOwnersInternalServerErrorException(
+ String.format(
+ "Failed to check %s capability", SkipCodeOwnerConfigValidationCapability.ID),
+ e);
+ }
+ }
+
+ public class InvalidValueException extends Exception {
+ private static final long serialVersionUID = 1L;
+
+ InvalidValueException(ImmutableList<String> invalidValues) {
+ super(
+ invalidValues.size() == 1
+ ? String.format(
+ "Invalid value for --%s~%s push option: %s",
+ pluginName, NAME, invalidValues.get(0))
+ : String.format(
+ "--%s~%s push option can be specified only once, received multiple values: %s",
+ pluginName, NAME, invalidValues));
+ }
+ }
+}
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/ValidationModule.java b/java/com/google/gerrit/plugins/codeowners/validation/ValidationModule.java
index a5d06ee..d28925c 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/ValidationModule.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/ValidationModule.java
@@ -14,7 +14,10 @@
package com.google.gerrit.plugins.codeowners.validation;
+import com.google.gerrit.extensions.annotations.Exports;
+import com.google.gerrit.extensions.config.CapabilityDefinition;
import com.google.gerrit.extensions.registration.DynamicSet;
+import com.google.gerrit.server.git.receive.PluginPushOption;
import com.google.gerrit.server.git.validators.CommitValidationListener;
import com.google.gerrit.server.git.validators.MergeValidationListener;
import com.google.inject.AbstractModule;
@@ -25,5 +28,11 @@
protected void configure() {
DynamicSet.bind(binder(), CommitValidationListener.class).to(CodeOwnerConfigValidator.class);
DynamicSet.bind(binder(), MergeValidationListener.class).to(CodeOwnerConfigValidator.class);
+
+ bind(CapabilityDefinition.class)
+ .annotatedWith(Exports.named(SkipCodeOwnerConfigValidationCapability.ID))
+ .to(SkipCodeOwnerConfigValidationCapability.class);
+ DynamicSet.bind(binder(), PluginPushOption.class)
+ .to(SkipCodeOwnerConfigValidationPushOption.class);
}
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
index b2d421d..2688b07 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/AbstractGetCodeOwnersForPathIT.java
@@ -35,6 +35,7 @@
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.Permission;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.client.ListAccountsOption;
import com.google.gerrit.extensions.restapi.AuthException;
@@ -45,8 +46,13 @@
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.TestPathExpressions;
import com.google.gerrit.plugins.codeowners.api.CodeOwners;
import com.google.gerrit.plugins.codeowners.api.CodeOwnersInfo;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfig;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigImportMode;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerConfigReference;
+import com.google.gerrit.plugins.codeowners.backend.CodeOwnerReference;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerResolver;
import com.google.gerrit.plugins.codeowners.backend.CodeOwnerSet;
+import com.google.gerrit.plugins.codeowners.restapi.CheckCodeOwnerCapability;
import com.google.gerrit.plugins.codeowners.restapi.GetCodeOwnersForPathInBranch;
import com.google.inject.Inject;
import java.util.List;
@@ -167,6 +173,7 @@
.comparingElementsUsing(hasAccountName())
.containsExactly(null, null, null);
assertThat(codeOwnersInfo).hasOwnedByAllUsersThat().isNull();
+ assertThat(codeOwnersInfo).hasDebugLogsThat().isNull();
}
@Test
@@ -1464,4 +1471,133 @@
"/foo/bar/baz.md");
assertThat(codeOwnersInfo).hasCodeOwnersThat().isEmpty();
}
+
+ @Test
+ public void debugRequireCallerToBeAdminOrHaveTheCheckCodeOwnerCapability() throws Exception {
+ requestScopeOperations.setApiUser(user.id());
+ AuthException authException =
+ assertThrows(
+ AuthException.class,
+ () ->
+ queryCodeOwners(
+ getCodeOwnersApi().query().withDebug(/* debug= */ true), "/foo/bar/baz.md"));
+ assertThat(authException)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format("%s for plugin code-owners not permitted", CheckCodeOwnerCapability.ID));
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "global.owner@example.com")
+ public void getCodeOwnersWithDebug_byAdmin() throws Exception {
+ testGetCodeOwnersWithDebug();
+ }
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.globalCodeOwner", value = "global.owner@example.com")
+ public void getCodeOwnersWithDebug_byUserThatHasTheCheckCodeOwnerCapability() throws Exception {
+ projectOperations
+ .allProjectsForUpdate()
+ .add(allowCapability("code-owners-" + CheckCodeOwnerCapability.ID).group(REGISTERED_USERS))
+ .update();
+
+ requestScopeOperations.setApiUser(user.id());
+ testGetCodeOwnersWithDebug();
+ }
+
+ private void testGetCodeOwnersWithDebug() throws Exception {
+ TestAccount globalOwner =
+ accountCreator.create("global_owner", "global.owner@example.com", "Global Owner", null);
+
+ skipTestIfImportsNotSupportedByCodeOwnersBackend();
+
+ TestAccount user2 = accountCreator.user2();
+
+ CodeOwnerConfig.Key rootKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/")
+ .addCodeOwnerEmail(admin.email())
+ .create();
+
+ String nonExistingEmail = "non-existing@example.com";
+ CodeOwnerConfig.Key fooKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/")
+ .addCodeOwnerEmail(nonExistingEmail)
+ .create();
+
+ Project.NameKey nonExistingProject = Project.nameKey("non-existing");
+ CodeOwnerConfigReference nonResolvableCodeOwnerConfigReference =
+ CodeOwnerConfigReference.builder(
+ CodeOwnerConfigImportMode.GLOBAL_CODE_OWNER_SETS_ONLY,
+ codeOwnerConfigOperations
+ .codeOwnerConfig(CodeOwnerConfig.Key.create(nonExistingProject, "master", "/"))
+ .getFilePath())
+ .setProject(nonExistingProject)
+ .build();
+
+ CodeOwnerConfig.Key fooBarKey =
+ codeOwnerConfigOperations
+ .newCodeOwnerConfig()
+ .project(project)
+ .branch("master")
+ .folderPath("/foo/bar/")
+ .addImport(nonResolvableCodeOwnerConfigReference)
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("md"))
+ .addCodeOwnerEmail(user.email())
+ .build())
+ .addCodeOwnerSet(
+ CodeOwnerSet.builder()
+ .addPathExpression(testPathExpressions.matchFileType("txt"))
+ .addCodeOwnerEmail(user2.email())
+ .build())
+ .create();
+
+ String path = "/foo/bar/baz.md";
+ CodeOwnersInfo codeOwnersInfo =
+ queryCodeOwners(getCodeOwnersApi().query().withDebug(/* debug= */ true), path);
+ assertThat(codeOwnersInfo)
+ .hasCodeOwnersThat()
+ .comparingElementsUsing(hasAccountId())
+ .containsExactly(user.id(), admin.id(), globalOwner.id())
+ .inOrder();
+ assertThat(codeOwnersInfo)
+ .hasDebugLogsThatContainAllOf(
+ String.format("resolve code owners for %s from code owner config %s", path, fooBarKey),
+ "per-file code owner set with path expressions [*.md] matches",
+ String.format(
+ "The import of %s:master:/%s in %s:master:/foo/bar/%s cannot be resolved:"
+ + " project %s not found",
+ nonExistingProject.get(),
+ getCodeOwnerConfigFileName(),
+ project.get(),
+ getCodeOwnerConfigFileName(),
+ nonExistingProject.get()),
+ String.format(
+ "resolving code owner reference %s", CodeOwnerReference.create(user.email())),
+ String.format("resolved to account %d", user.id().get()),
+ String.format("resolve code owners for %s from code owner config %s", path, fooKey),
+ String.format(
+ "resolving code owner reference %s", CodeOwnerReference.create(nonExistingEmail)),
+ String.format(
+ "cannot resolve code owner email %s: no account with this email exists",
+ nonExistingEmail),
+ String.format("resolve code owners for %s from code owner config %s", path, rootKey),
+ String.format(
+ "resolving code owner reference %s", CodeOwnerReference.create(admin.email())),
+ String.format("resolved to account %d", admin.id().get()),
+ "resolve global code owners",
+ String.format(
+ "resolving code owner reference %s",
+ CodeOwnerReference.create(globalOwner.email())),
+ String.format("resolved to account %d", globalOwner.id().get()));
+ }
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
index c364abc..fda0093 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -16,6 +16,7 @@
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allow;
+import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
@@ -62,12 +63,16 @@
import com.google.gerrit.plugins.codeowners.backend.proto.ProtoBackend;
import com.google.gerrit.plugins.codeowners.backend.proto.ProtoCodeOwnerConfigParser;
import com.google.gerrit.plugins.codeowners.common.CodeOwnerConfigValidationPolicy;
+import com.google.gerrit.plugins.codeowners.validation.SkipCodeOwnerConfigValidationCapability;
+import com.google.gerrit.plugins.codeowners.validation.SkipCodeOwnerConfigValidationPushOption;
import com.google.gerrit.server.IdentifiedUser;
import com.google.inject.Inject;
import com.google.inject.Key;
import com.google.inject.util.Providers;
import java.nio.file.Path;
import java.util.Optional;
+import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
+import org.eclipse.jgit.junit.TestRepository;
import org.eclipse.jgit.lib.AnyObjectId;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
@@ -430,6 +435,146 @@
}
@Test
+ public void userCannotSkipCodeOwnerConfigValidationWithoutCapability() throws Exception {
+ PushOneCommit.Result r =
+ uploadNonParseableConfigWithPushOption(
+ user, String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME));
+ assertErrorWithMessages(
+ r,
+ "skipping code owner config validation not allowed",
+ String.format(
+ "%s for plugin code-owners not permitted", SkipCodeOwnerConfigValidationCapability.ID));
+ }
+
+ @Test
+ public void adminCanSkipCodeOwnerConfigValidation() throws Exception {
+ PushOneCommit.Result r =
+ uploadNonParseableConfigWithPushOption(
+ admin, String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME));
+ assertOkWithHints(
+ r,
+ "skipping validation of code owner config files",
+ String.format(
+ "the validation is skipped due to the --code-owners~%s push option",
+ SkipCodeOwnerConfigValidationPushOption.NAME));
+ }
+
+ @Test
+ public void canUploadNonParseableConfigWithSkipOption() throws Exception {
+ allowRegisteredUsersToSkipValidation();
+
+ // with --code-owners~skip-validation
+ PushOneCommit.Result r =
+ uploadNonParseableConfigWithPushOption(
+ user, String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME));
+ assertOkWithHints(
+ r,
+ "skipping validation of code owner config files",
+ String.format(
+ "the validation is skipped due to the --code-owners~%s push option",
+ SkipCodeOwnerConfigValidationPushOption.NAME));
+
+ // with --code-owners~skip-validation=true
+ r =
+ uploadNonParseableConfigWithPushOption(
+ user,
+ String.format("code-owners~%s=true", SkipCodeOwnerConfigValidationPushOption.NAME));
+ assertOkWithHints(
+ r,
+ "skipping validation of code owner config files",
+ String.format(
+ "the validation is skipped due to the --code-owners~%s push option",
+ SkipCodeOwnerConfigValidationPushOption.NAME));
+
+ // with --code-owners~skip-validation=TRUE
+ r =
+ uploadNonParseableConfigWithPushOption(
+ user,
+ String.format("code-owners~%s=TRUE", SkipCodeOwnerConfigValidationPushOption.NAME));
+ assertOkWithHints(
+ r,
+ "skipping validation of code owner config files",
+ String.format(
+ "the validation is skipped due to the --code-owners~%s push option",
+ SkipCodeOwnerConfigValidationPushOption.NAME));
+ }
+
+ @Test
+ public void cannotUploadNonParseableConfigIfSkipOptionIsFalse() throws Exception {
+ allowRegisteredUsersToSkipValidation();
+
+ // with --code-owners~skip-validation=false
+ PushOneCommit.Result r =
+ uploadNonParseableConfigWithPushOption(
+ user,
+ String.format("code-owners~%s=false", SkipCodeOwnerConfigValidationPushOption.NAME));
+ String abbreviatedCommit = abbreviateName(r.getCommit());
+ r.assertErrorStatus(
+ String.format(
+ "commit %s: [code-owners] %s", abbreviatedCommit, "invalid code owner config files"));
+ }
+
+ @Test
+ public void cannotUploadNonParseableConfigWithInvalidSkipOption() throws Exception {
+ allowRegisteredUsersToSkipValidation();
+
+ PushOneCommit.Result r =
+ uploadNonParseableConfigWithPushOption(
+ user,
+ String.format("code-owners~%s=INVALID", SkipCodeOwnerConfigValidationPushOption.NAME));
+ assertErrorWithMessages(
+ r,
+ "invalid push option",
+ String.format(
+ "Invalid value for --code-owners~%s push option: INVALID",
+ SkipCodeOwnerConfigValidationPushOption.NAME));
+ }
+
+ @Test
+ public void cannotUploadNonParseableConfigIfSkipOptionIsSetMultipleTimes() throws Exception {
+ allowRegisteredUsersToSkipValidation();
+
+ PushOneCommit.Result r =
+ uploadNonParseableConfigWithPushOption(
+ user,
+ String.format("code-owners~%s", SkipCodeOwnerConfigValidationPushOption.NAME),
+ String.format("code-owners~%s=false", SkipCodeOwnerConfigValidationPushOption.NAME));
+ assertErrorWithMessages(
+ r,
+ "invalid push option",
+ String.format(
+ "--code-owners~%s push option can be specified only once, received multiple values: [, false]",
+ SkipCodeOwnerConfigValidationPushOption.NAME));
+ }
+
+ private PushOneCommit.Result uploadNonParseableConfigWithPushOption(
+ TestAccount testAccount, String... pushOptions) throws Exception {
+ TestRepository<InMemoryRepository> userRepo = cloneProject(project, testAccount);
+ PushOneCommit push =
+ pushFactory.create(
+ testAccount.newIdent(),
+ userRepo,
+ "Add code owners",
+ codeOwnerConfigOperations
+ .codeOwnerConfig(createCodeOwnerConfigKey("/"))
+ .getJGitFilePath(),
+ "INVALID");
+ push.setPushOptions(ImmutableList.copyOf(pushOptions));
+ return push.to("refs/for/master");
+ }
+
+ private void allowRegisteredUsersToSkipValidation() {
+ // grant the global capability that is required to use the
+ // --code-owners~skip-validation push option to registered users
+ projectOperations
+ .allProjectsForUpdate()
+ .add(
+ allowCapability("code-owners-" + SkipCodeOwnerConfigValidationCapability.ID)
+ .group(REGISTERED_USERS))
+ .update();
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.enableValidationOnCommitReceived", value = "forced")
public void
cannotUploadNonParseableConfigIfCodeOwnersFunctionalityIsDisabledButValidationIsEnforced()
diff --git a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
index eb7e53f..5d63b73 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerSubmitRuleIT.java
@@ -415,6 +415,79 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void changeIsSubmittableWithOverrideIfOwnersFileIsNonParsable() throws Exception {
+ createOwnersOverrideLabel();
+
+ // Add a non-parsable code owner config.
+ String nameOfInvalidCodeOwnerConfigFile = getCodeOwnerConfigFileName();
+ createNonParseableCodeOwnerConfig(nameOfInvalidCodeOwnerConfigFile);
+
+ PushOneCommit.Result r = createChange("Some Change", "foo.txt", "some content");
+ String changeId = r.getChangeId();
+
+ // Apply Code-Review+2 to satisfy the MaxWithBlock function of the Code-Review label.
+ approve(changeId);
+
+ ChangeInfo changeInfo =
+ gApi.changes()
+ .id(changeId)
+ .get(
+ ListChangesOption.SUBMITTABLE,
+ ListChangesOption.ALL_REVISIONS,
+ ListChangesOption.CURRENT_ACTIONS);
+ assertThat(changeInfo.submittable).isFalse();
+
+ // Check that the submit button is not visible.
+ assertThat(changeInfo.revisions.get(r.getCommit().getName()).actions.get("submit")).isNull();
+
+ // Check the submit requirement.
+ assertThatCollection(changeInfo.requirements).isEmpty();
+
+ // Try to submit the change.
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class, () -> gApi.changes().id(changeId).current().submit());
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Failed to submit 1 change due to the following problems:\n"
+ + "Change %s: submit rule error: Failed to evaluate code owner statuses for"
+ + " patch set 1 of change %s (cause: invalid code owner config file '%s'"
+ + " (project = %s, branch = master):\n %s).",
+ changeInfo._number,
+ changeInfo._number,
+ JgitPath.of(nameOfInvalidCodeOwnerConfigFile).getAsAbsolutePath(),
+ project,
+ getParsingErrorMessageForNonParseableCodeOwnerConfig()));
+
+ // Apply an override.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
+
+ // Check the submittable flag.
+ changeInfo =
+ gApi.changes()
+ .id(changeId)
+ .get(
+ ListChangesOption.SUBMITTABLE,
+ ListChangesOption.ALL_REVISIONS,
+ ListChangesOption.CURRENT_ACTIONS);
+ assertThat(changeInfo.submittable).isTrue();
+
+ // Check the submit requirement.
+ LegacySubmitRequirementInfoSubject submitRequirementInfoSubject =
+ assertThatCollection(changeInfo.requirements).onlyElement();
+ submitRequirementInfoSubject.hasStatusThat().isEqualTo("OK");
+ submitRequirementInfoSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ submitRequirementInfoSubject.hasTypeThat().isEqualTo("code-owners");
+
+ // Submit the change.
+ gApi.changes().id(changeId).current().submit();
+ assertThat(gApi.changes().id(changeId).get().status).isEqualTo(ChangeStatus.MERGED);
+ }
+
+ @Test
@GerritConfig(
name = "plugin.code-owners.mergeCommitStrategy",
value = "FILES_WITH_CONFLICT_RESOLUTION")
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
index 8c18b66..a06db05 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/CodeOwnerSubmitRuleTest.java
@@ -25,6 +25,7 @@
import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Change;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
import com.google.gerrit.plugins.codeowners.acceptance.testsuite.CodeOwnerConfigOperations;
import com.google.gerrit.plugins.codeowners.testing.LegacySubmitRequirementSubject;
@@ -154,17 +155,17 @@
@Test
public void ruleError_nonParsableCodeOwnerConfig() throws Exception {
- testRuleErrorForNonParsableCodeOwnerConfigl(/* invalidCodeOwnerConfigInfoUrl= */ null);
+ testRuleErrorForNonParsableCodeOwnerConfig(/* invalidCodeOwnerConfigInfoUrl= */ null);
}
@Test
@GerritConfig(name = "plugin.code-owners.invalidCodeOwnerConfigInfoUrl", value = "http://foo.bar")
public void ruleError_nonParsableCodeOwnerConfig_withInvalidCodeOwnerConfigInfoUrl()
throws Exception {
- testRuleErrorForNonParsableCodeOwnerConfigl("http://foo.bar");
+ testRuleErrorForNonParsableCodeOwnerConfig("http://foo.bar");
}
- public void testRuleErrorForNonParsableCodeOwnerConfigl(
+ private void testRuleErrorForNonParsableCodeOwnerConfig(
@Nullable String invalidCodeOwnerConfigInfoUrl) throws Exception {
String nameOfInvalidCodeOwnerConfigFile = getCodeOwnerConfigFileName();
createNonParseableCodeOwnerConfig(nameOfInvalidCodeOwnerConfigFile);
@@ -190,4 +191,43 @@
? String.format("\nFor help check %s.", invalidCodeOwnerConfigInfoUrl)
: ""));
}
+
+ @Test
+ @GerritConfig(name = "plugin.code-owners.overrideApproval", value = "Owners-Override+1")
+ public void overrideWhenCodeOwnerConfigIsNonParsable() throws Exception {
+ createOwnersOverrideLabel();
+
+ String nameOfInvalidCodeOwnerConfigFile = getCodeOwnerConfigFileName();
+ createNonParseableCodeOwnerConfig(nameOfInvalidCodeOwnerConfigFile);
+
+ ChangeData changeData = createChange().getChange();
+ String changeId = changeData.change().getKey().get();
+
+ SubmitRecordSubject submitRecordSubject =
+ assertThatOptional(codeOwnerSubmitRule.evaluate(changeData)).value();
+ submitRecordSubject.hasStatusThat().isRuleError();
+ submitRecordSubject
+ .hasErrorMessageThat()
+ .isEqualTo(
+ String.format(
+ "Failed to evaluate code owner statuses for patch set %d of change %d"
+ + " (cause: invalid code owner config file '%s' (project = %s, branch = master):\n"
+ + " %s).",
+ changeData.change().currentPatchSetId().get(),
+ changeData.change().getId().get(),
+ JgitPath.of(nameOfInvalidCodeOwnerConfigFile).getAsAbsolutePath(),
+ project,
+ getParsingErrorMessageForNonParseableCodeOwnerConfig()));
+
+ // Apply an override.
+ gApi.changes().id(changeId).current().review(new ReviewInput().label("Owners-Override", 1));
+ changeData.reloadChange();
+
+ submitRecordSubject = assertThatOptional(codeOwnerSubmitRule.evaluate(changeData)).value();
+ submitRecordSubject.hasStatusThat().isOk();
+ LegacySubmitRequirementSubject submitRequirementSubject =
+ submitRecordSubject.hasSubmitRequirementsThat().onlyElement();
+ submitRequirementSubject.hasTypeThat().isEqualTo("code-owners");
+ submitRequirementSubject.hasFallbackTextThat().isEqualTo("Code Owners");
+ }
}
diff --git a/resources/Documentation/backend-find-owners-cookbook.md b/resources/Documentation/backend-find-owners-cookbook.md
index a3e1adb..98db7c8 100644
--- a/resources/Documentation/backend-find-owners-cookbook.md
+++ b/resources/Documentation/backend-find-owners-cookbook.md
@@ -40,7 +40,7 @@
### <a id="ignoreParentCodeOwners">Ignore parent code owners
-To ignore code owners that are defined in the `OWNERS` file of the parent
+To ignore code owners that are defined in the `OWNERS` files of the parent
directories the [set noparent](backend-find-owners.html#setNoparent) file-level
rule can be used:
@@ -51,6 +51,19 @@
richard.roe@example.com
```
\
+For example, if code owners for the file '/foo/bar/baz.txt' are computed the
+`OWNERS` files are evaluated in this order:
+
+1. `/foo/bar/OWNERS`
+2. `/foo/OWNERS`
+3. `/OWNERS`
+4. `/OWNERS` in `refs/meta/config`
+ (contains [default code owners](backend-find-owners.html#defaultCodeOwnerConfiguration))
+
+If any `set noparent` file-level rule is seen the evaluation is stopped and
+further `OWNERS` files are ignored. E.g. if `/foo/OWNERS` contains
+`set noparent` the `OWNERS` files mentioned at 3. and 4. are ignored.
+
**NOTE:** When the [set noparent](backend-find-owners.html#setNoparent)
file-level rule is used you should always define code owners which should be
used instead of the code owners from the parent directories. Otherwise the files
@@ -59,6 +72,9 @@
approvals](#exemptFiles), assign the code ownership to [all
users](backend-find-owners.html#allUsers) instead ([example](#exemptFiles)).
+**NOTE:** The usage of `set noparent` has no effect on `OWNERS` files in
+subfolders.
+
### <a id="defineCodeOwnersForAFile">Define code owners for a certain file
By using the [per-file](backend-find-owners.html#perFile) restriction prefix it
@@ -87,8 +103,8 @@
users that are mentioned in these [per-file](backend-find-owners.html#perFile)
lines.
-If non-restricted code owners are present, the file is also owned by any
-non-restricted code owners:
+If folder code owners are present, the file is also owned by any folder code
+owners:
```
jane.roe@example.com
@@ -101,9 +117,9 @@
parent directories.
#### <a id="perFileWithSetNoparent">
-Ignoring non-restricted code owners and inherited parent code owners for a file
-is possible by using a matching [per-file](backend-find-owners.html#perFile)
-line with [set noparent](backend-find-owners.html#setNoparent).
+Ignoring folder code owners and inherited parent code owners for a file is
+possible by using a matching [per-file](backend-find-owners.html#perFile) line
+with [set noparent](backend-find-owners.html#setNoparent).
```
jane.roe@example.com
@@ -113,13 +129,33 @@
per-file BUILD=tina.toe@example.com,martha.moe@example.com
```
\
+For example, if code owners for the file '/foo/bar/baz.txt' are computed the
+code owners in the `OWNERS` files are evaluated in this order:
+
+1. matching per-file code owners in `/foo/bar/OWNERS`
+2. folder code owners in `/foo/bar/OWNERS`
+3. matching per-file code owners in `/foo/OWNERS`
+4. folder code owners in `/foo/OWNERS`
+5. matching per-file code owners in `/OWNERS`
+6. folder code owners in `/OWNERS`
+7. matching per-file code owners in `/OWNERS` in `refs/meta/config`
+ (contains [default code owners](backend-find-owners.html#defaultCodeOwnerConfiguration))
+8. folder code owners in `/OWNERS` in `refs/meta/config`
+ (contains [default code owners](backend-find-owners.html#defaultCodeOwnerConfiguration))
+
+If any `set noparent` file-level rule is seen the evaluation is stopped and
+code owners on further levels are ignored. E.g. if `/foo/OWNERS` contains a
+matching per-file rule with `set noparent` the code owners mentioned at 4. to 8.
+are ignored.
+
**NOTE:** When the [set noparent](backend-find-owners.html#setNoparent) rule is
-used you should always define code owners which should be used instead of the
-non-restricted code owners and the code owners from the parent directories.
-Otherwise the matched files stay [without code owners](#noCodeOwners) and nobody
-can grant code owner approval on them. To [exempt matched files from requiring
-code owner approvals](#exemptFiles), assign the code ownership to [all
-users](backend-find-owners.html#allUsers) instead ([example](#exemptFiles)).
+used on a per-file rule you should always define code owners which should be
+used instead of the folder code owners and the code owners from the parent
+directories. Otherwise the matched files stay [without code
+owners](#noCodeOwners) and nobody can grant code owner approval on them. To
+[exempt matched files from requiring code owner approvals](#exemptFiles), assign
+the code ownership to [all users](backend-find-owners.html#allUsers) instead
+([example](#exemptFiles)).
**NOTE:** The syntax for path expressions / globs is explained
[here](path-expressions.html#globs).
@@ -189,10 +225,10 @@
```
\
**NOTE:** The `per-file` line from `/OWNERS_BUILD` is not imported, since the
-[file](backend-find-owners.html#fileKeyword) keyword only imports non-restricted
-code owners. Using the [include](backend-find-owners.html#includeKeyword)
-keyword, that would also consider per-file code owners, is not supported for
-`per-file` lines.
+[file](backend-find-owners.html#fileKeyword) keyword only imports folder code
+owners. Using the [include](backend-find-owners.html#includeKeyword) keyword,
+that would also consider per-file code owners, is not supported for `per-file`
+lines.
### <a id="importOtherOwnersFile">Import code owners from other OWNERS file
@@ -223,9 +259,8 @@
\
**NOTE:** The `per-file` line from `java/com/example/foo/OWNERS` is not
imported, since the [file](backend-find-owners.html#fileKeyword) keyword only
-imports non-restricted code owners. If also `per-line` files should be imported
-the [include](backend-find-owners.html#includeKeyword) keyword can be used
-instead:
+imports folder code owners. If also `per-line` lines should be imported the
+[include](backend-find-owners.html#includeKeyword) keyword can be used instead:
`javatests/com/example/foo/OWNERS`:
```
@@ -330,8 +365,8 @@
* an `OWNERS` file with only `set noparent` (ignores code owners from parent
directories, but doesn't define code owners that should be used instead)
-* a `per-file` line with only `set noparent` (ignores non-restricted code owners
- and code owners from parent directories, but doesn't define code owners that
+* a `per-file` line with only `set noparent` (ignores folder code owners and
+ code owners from parent directories, but doesn't define code owners that
should be used instead)
* no `OWNERS` file at root level
diff --git a/resources/Documentation/config-faqs.md b/resources/Documentation/config-faqs.md
index 7396934..ab88bb7 100644
--- a/resources/Documentation/config-faqs.md
+++ b/resources/Documentation/config-faqs.md
@@ -4,6 +4,7 @@
* [How to check if the code owners functionality is enabled for a project or branch](#checkIfEnabled)
* [How to avoid issues with code owner config files](#avoidIssuesWithCodeOwnerConfigs)
* [How to investigate issues with code owner config files](#investigateIssuesWithCodeOwnerConfigs)
+* [How to investigate issues with the code owner suggestion](#investigateIssuesWithCodeOwnerSuggestion)
* [How to define default code owners](#defineDefaultCodeOwners)
* [How to setup code owner overrides](#setupOverrides)
* [What's the best place to keep the global plugin
@@ -86,6 +87,7 @@
Since code owner config files are part of the source code, any issues with them
should be investigated and fixed by the project owners and host administrators.
+
To do this they can:
* Check the code owner config files for issues by calling the [Check Code Owner
@@ -101,6 +103,39 @@
Also see [above](#avoidIssuesWithCodeOwnerConfigs) how to avoid issues with code
owner config files in the first place.
+## <a id="investigateIssuesWithCodeOwnerSuggestion">How to investigate issues with the code owner suggestion
+
+If the code owners config suggestion is not working as expected, this is either
+caused by:
+
+* issues in the code owner config files
+* user permissions
+* account visibility
+* account states
+* a bug in the @PLUGIN@ plugin
+
+Issues with code owner config files, user permissions, account visibility and
+account states should be investigated and fixed by the project owners and host
+administrators.
+
+To do this they can:
+
+* Use the `--debug` option of the [List Code
+ Owners](rest-api.html#list-code-owners-for-path-in-branch) REST endpoints to
+ get debug logs included into the response.
+* Check the code owner config files for issues by calling the [Check Code Owner
+ Config File REST endpoint](rest-api.html#check-code-owner-config-files)
+* Check the code ownership of a user for a certain path by calling the [Check
+ Code Owner REST endpoint](rest-api.html#check-code-owner) (requires the caller
+ to be host administrator or have the [Check Code Owner
+ capability](rest-api.html#checkCodeOwner))
+
+Bugs with the @PLUGIN@ plugin should be filed as issues for the Gerrit team, but
+only after other causes have been excluded.
+
+Also see [above](#avoidIssuesWithCodeOwnerConfigs) how to avoid issues with code
+owner config files in the first place.
+
## <a id="defineDefaultCodeOwners">How to define default code owners
[Default code owners](backend-find-owners.html#defaultCodeOwnerConfiguration)
diff --git a/resources/Documentation/config-guide.md b/resources/Documentation/config-guide.md
index 3202450..473de83 100644
--- a/resources/Documentation/config-guide.md
+++ b/resources/Documentation/config-guide.md
@@ -121,7 +121,8 @@
[Global code owners](config.html#pluginCodeOwnersGlobalCodeOwner) are defined
in the plugin configuration and apply to all projects or all child projects.\
They are intended to configure bots as code owners that need to operate on
- all or multiple projects.\
+ all or multiple projects. Alternatively bots may be configured as exempted
+ users (see further below).\
Global code owners still apply if parent code owners are ignored.
5. Fallback code owners:
[Fallback code owners](config.html#pluginCodeOwnersFallbackCodeOwners) is a
@@ -130,6 +131,13 @@
Fallback code owners are not included in the code owner suggestion.\
Configuring all users as fallback code owners may allow bypassing the code
owners check (see [security pitfalls](#securityFallbackCodeOwners) below).
+6. Exempted users:
+ [Exempted users](config.html#pluginCodeOwnersExemptedUser) are exempted from
+ requiring code owner approvals.\
+ If a user is exempted from requiring code owner approvals changes that are
+ uploaded by this user are automatically code-owner approved.\
+ Exempted users are intended to be used for bots that need to create changes
+ on all or multiple projects that should not require code owner approvals.
In addition users can be allowed to [override the code owner submit
check](user-guide.html#codeOwnerOverride). This permission is normally granted
diff --git a/resources/Documentation/metrics.md b/resources/Documentation/metrics.md
index fd782a7..de0ec87 100644
--- a/resources/Documentation/metrics.md
+++ b/resources/Documentation/metrics.md
@@ -71,6 +71,11 @@
The cause of the submit rule error.
* `count_code_owner_submit_rule_runs`:
Total number of code owner submit rule runs.
+* `count_code_owner_suggestions`:
+ Total number of code owner suggestions.
+ * `resolve_all_users`:
+ Whether code ownerships that are assigned to all users are resolved to
+ random users.
* `count_invalid_code_owner_config_files`:
Total number of failed requests caused by an invalid / non-parsable code owner
config file.
diff --git a/resources/Documentation/rest-api.md b/resources/Documentation/rest-api.md
index df39f6f..be963ff 100644
--- a/resources/Documentation/rest-api.md
+++ b/resources/Documentation/rest-api.md
@@ -79,7 +79,7 @@
Content-Type: application/json; charset=UTF-8
{
- "disabled": "true"
+ "disabled": true
}
```
@@ -96,7 +96,7 @@
)]}'
{
"status": {
- "disabled": "true"
+ "disabled": true
}
}
```
@@ -299,15 +299,15 @@
)]}'
{
- "is_code_owner": "false",
- "is_resolvable": "false",
- "can_read_ref": "true",
+ "is_code_owner": false,
+ "is_resolvable": false,
+ "can_read_ref": true,
"code_owner_config_file_paths": [
"/OWNERS",
],
- "is_fallback_code_owner": "false",
- "is_default_code_owner": "false",
- "is_global_code_owner": "false",
+ "is_fallback_code_owner": false,
+ "is_default_code_owner": false,
+ "is_global_code_owner": false,
"debug_logs": [
"checking code owner config file foo/bar:master:/OWNERS",
"found email xyz@example.com as code owner in /OWNERS",
@@ -462,6 +462,7 @@
| `seed` | optional | Seed, as a long value, that should be used to shuffle code owners that have the same score. Can be used to make the sort order stable across several requests, e.g. to get the same set of random code owners for different file paths that have the same code owners. Important: the sort order is only stable if the requests use the same seed **and** the same limit. In addition, the sort order is not guaranteed to be stable if new accounts are created in between the requests, or if the account visibility is changed.
| `resolve-all-users` | optional | Whether code ownerships that are assigned to all users should be resolved to random users. If not set, `false` by default. Also see the [sorting example](#sortingExample) below to see how this parameter affects the returned code owners.
| `highest-score-only` | optional | Whether only code owners with the highest score should be returned. If not set, `false` by default.
+| `debug` | optional | Whether debug logs should be included into the response. Requires the [Check Code Owner](#checkCodeOwner) global capability.
| `revision` | optional | Revision from which the code owner configs should be read as commit SHA1. Can be used to read historic code owners from this branch, but imports from other branches or repositories as well as default and global code owners from `refs/meta/config` are still read from the current revisions. If not specified the code owner configs are read from the HEAD revision of the branch. Not supported for getting code owners for a path in a change.
As a response a [CodeOwnersInfo](#code-owners-info) entity is returned that
@@ -959,6 +960,7 @@
| ------------- | -------- | ----------- |
| `code_owners` | | List of code owners as [CodeOwnerInfo](#code-owner-info) entities. The code owners are sorted by a score that is computed from mutliple [scoring factors](#scoringFactors).
| `owned_by_all_users` | optional | Whether the path is owned by all users. Not set if `false`.
+| `debug_logs` | optional | Debug logs that may help to understand why a user is or isn't suggested as a code owner. Only set if requested via `--debug`.
### <a id="file-code-owner-status-info"> FileCodeOwnerStatusInfo
The `FileCodeOwnerStatusInfo` entity describes the code owner statuses for a
@@ -1039,7 +1041,8 @@
### <a id="checkCodeOwner">Check Code Owner
Global capability that allows a user to call the [Check Code
-Owner](#check-code-owner) REST endpoint.
+Owner](#check-code-owner) REST endpoint and use the `--debug` option of the
+[List Code Owners](#list-code-owners-for-path-in-branch) REST endpoints.
Assigning this capability allows users to inspect code ownerships. This may
reveal accounts and secondary emails to the user that the user cannot see
diff --git a/resources/Documentation/setup-guide.md b/resources/Documentation/setup-guide.md
index 451daed..bd67ed2 100644
--- a/resources/Documentation/setup-guide.md
+++ b/resources/Documentation/setup-guide.md
@@ -16,7 +16,7 @@
3. [Opt-out branches that should not use code owners](#optOutBranches)
4. [Configure the label vote that should count as code owner approval](#configureCodeOwnerApproval)
5. [Grant code owners permission to vote on the label that counts as code owner approval](#grantCodeOwnerPermissions)
-6. [Configure code owner overrides](#configureCodeOwnerOverrides)
+6. [Configure code owner overrides & fallback code owners](#configureCodeOwnerOverridesAndFallbackCodeOwners)
7. [Configure allowed email domains](#configureAllowedEmailDomains)
8. [Optional Configuration](#optionalConfiguration)
9. [Stop using the find-owners Prolog submit rule](#stopUsingFindOwners)
@@ -237,13 +237,22 @@
needs to be granted via the access screen of the project or a parent project (at
`https://<host>/admin/repos/<project-name>,access`).
-### <a id="configureCodeOwnerOverrides">6. Configure code owner overrides
+### <a id="configureCodeOwnerOverridesAndFallbackCodeOwners">6. Configure code owner overrides & fallback code owners
+
+It's possible that some files have no code owners defined (e.g. missing root
+code owner config file). In this case changes for these files cannot be code
+owner approved and hence cannot be submitted.
+
+To avoid that this leads to unsubmittable changes it is recommended to configure
+code owner overrides and/or fallback code owners.
+
+#### <a id="configureCodeOwnerOverrides">Configure code owner overrides
It's possible to configure code owner overrides that allow privileged users to
override code owner approvals. This means they can approve changes without being
a code owner.
-Configuring code owner overrides is optional.
+Configuring code owner overrides is optional, but recommended.
To enable code owner overrides, you must define which label vote is required for
an override. This can be done globally by setting
@@ -287,6 +296,18 @@
add both configurations in one commit is a known issue that still needs to be
fixed).
+#### <a id="configureFallbackCodeOwners">Configure fallback code owners
+
+It is possible to configure a policy for [fallback code
+owners](config.html#pluginCodeOwnersFallbackCodeOwners) that controls who should
+own files for which no code owners have been defined, e.g. project owners, all
+users or no one (default).
+
+Configuring fallback code owners is optional. For the initial rollout of the
+code-owners plugin it is highly recommended to allow fallback code owners so
+that projects that do not have any code owner config files yet are not
+disrupted.
+
### <a id="configureAllowedEmailDomains">7. Configure allowed email domains
By default, the emails in code owner config files that make users code owners
@@ -315,10 +336,6 @@
Examples (not an exhaustive list):
* [Global code owners](config.html#pluginCodeOwnersGlobalCodeOwner)
-* Configure a policy for [fallback code
- owners](config.html#pluginCodeOwnersFallbackCodeOwners) (who should own files
- for which no code owners have been defined, e.g. project owners, all users or
- no one)
* Whether [an implicit code owner approval from the last uploader is
assumed](config.html#codeOwnersEnableImplicitApprovals)
* [Merge commit strategy](config.html#codeOwnersMergeCommitStrategy) that
@@ -347,7 +364,7 @@
* with a code owner override (if override labels have been configured, see
[above](#configureCodeOwnerOverrides))
* with an approval from a fallback code owner (if fallback code owners have been
- configured, see [above](#optionalConfiguration)).
+ configured, see [above](#configureFallbackCodeOwners)).
Right after the code owners functionality got enabled for a project/branch, it
is recommended to add an initial code owner configuration at the root level that
diff --git a/resources/Documentation/validation.md b/resources/Documentation/validation.md
index 1e4687b..1c660a5 100644
--- a/resources/Documentation/validation.md
+++ b/resources/Documentation/validation.md
@@ -41,6 +41,8 @@
[enableValidationOnCommitReceived](config.html#codeOwnersEnableValidationOnCommitReceived)
or [enableValidationOnSubmit](config.html#codeOwnersEnableValidationOnSubmit)
config options
+* the [--code-owners~skip-validation](#skipCodeOwnerConfigValidationOnDemand)
+ push option was specified on push
In addition for [code owner config files](user-guide.html#codeOwnerConfigFiles)
no validation is done when:
@@ -53,6 +55,22 @@
blocking all uploads, to reduce the risk of breaking the plugin configuration
`code-owner.config` files are validated too)
+## <a id="skipCodeOwnerConfigValidationOnDemand">Skip code owner config validation on demand
+
+By setting the `--code-owners~skip-validation` push option it is possible to
+skip the code owner config validation on push.
+
+Using this push option requires the calling user to have to
+`Can Skip Code Owner Config Validation` global capability. Host administrators
+have this capability implicitly assigned via the `Administrate Server` global
+capability.
+
+**NOTE:** Using this option only makes sense if the [code owner config validation
+on submit](config.html#pluginCodeOwnersEnableValidationOnSubmit) is disabled, as
+otherwise it's not possible to submit the created change (using the push option
+only skips the validation for the push, but not for the submission of the
+change).
+
## <a id="howCodeOwnerConfigsCanGetIssuesAfterSubmit">
In addition it is possible that [code owner config
files](user-guide.hmtl#codeOwnerConfigFiles) get issues after they have been
diff --git a/ui/BUILD b/ui/BUILD
new file mode 100644
index 0000000..274676e
--- /dev/null
+++ b/ui/BUILD
@@ -0,0 +1,14 @@
+load("//tools/bzl:js.bzl", "gerrit_js_bundle")
+
+package_group(
+ name = "visibility",
+ packages = ["//plugins/code-owners/..."],
+)
+
+package(default_visibility = [":visibility"])
+
+gerrit_js_bundle(
+ name = "code-owners",
+ srcs = glob(["*.js"]),
+ entry_point = "plugin.js",
+)
diff --git a/ui/suggest-owners.js b/ui/suggest-owners.js
index 2304523..f07003f 100644
--- a/ui/suggest-owners.js
+++ b/ui/suggest-owners.js
@@ -105,9 +105,6 @@
padding: 0 var(--spacing-m);
margin: var(--spacing-m) 0;
}
- p.loading {
- text-align: center;
- }
.loadingSpin {
display: inline-block;
}
@@ -130,12 +127,17 @@
}
.suggestion-row {
flex-wrap: wrap;
- border-bottom: 1px solid var(--border-color);
+ border-top: 1px solid var(--border-color);
padding: var(--spacing-s) 0;
}
.show-all-owners-row {
- padding: var(--spacing-m) var(--spacing-xl) var(--spacing-s);
- justify-content: flex-end;
+ padding: var(--spacing-m) var(--spacing-xl) var(--spacing-s) 0;
+ }
+ .show-all-owners-row .loading {
+ padding: 0;
+ }
+ .show-all-owners-row .show-all-label {
+ margin-left: auto; /* align label to the right */
}
.suggestion-row-indicator {
margin-right: var(--spacing-s);
@@ -247,10 +249,22 @@
margin-right: var(--spacing-m);
}
</style>
- <p class="loading" hidden="[[!isLoading]]">
- <span class="loadingSpin"></span>
- [[progressText]]
- </p>
+ <ul class="suggestion-container">
+ <li class="show-all-owners-row">
+ <p class="loading" hidden="[[!isLoading]]">
+ <span class="loadingSpin"></span>
+ [[progressText]]
+ </p>
+ <label class="show-all-label">
+ <input
+ id="showAllOwnersCheckbox"
+ type="checkbox"
+ checked="{{_showAllOwners::change}}"
+ />
+ Show all owners
+ </label>
+ </li>
+ </ul>
<ul class="suggestion-container">
<template
is="dom-repeat"
@@ -332,16 +346,6 @@
</template>
</li>
</template>
- <li class="show-all-owners-row">
- <label>
- <input
- id="showAllOwnersCheckbox"
- type="checkbox"
- checked="{{_showAllOwners::change}}"
- />
- Show all owners
- </label>
- </li>
</ul>
`;
}
