Allow validating code owner configs even if code owners is switched off
When rolling out code-owners, it can make sense to enable the validation
of code owner config files before enabling the code owners
functionality. To support this we add 'FORCED' and 'FORCED_DRY_RUN' as
new options for the enableValidationOnCommitReceived and
enableValidationOnSubmit configuration parameters, since 'TRUE' and
'DRY_RUN' only trigger the validation if the code owners functionality
is enabled.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I29fdbd165e067bdab442f1d1d5e54dc537c67369
diff --git a/java/com/google/gerrit/plugins/codeowners/common/CodeOwnerConfigValidationPolicy.java b/java/com/google/gerrit/plugins/codeowners/common/CodeOwnerConfigValidationPolicy.java
index 5976c71..7b35079 100644
--- a/java/com/google/gerrit/plugins/codeowners/common/CodeOwnerConfigValidationPolicy.java
+++ b/java/com/google/gerrit/plugins/codeowners/common/CodeOwnerConfigValidationPolicy.java
@@ -19,6 +19,8 @@
/**
* The code owner config file validation is enabled and invalid code owner config files are
* rejected.
+ *
+ * <p>If the code owners functionality is disabled, no validation is performed.
*/
TRUE,
@@ -30,14 +32,39 @@
/**
* Code owner config files are validated, but invalid code owner config files are not rejected.
+ *
+ * <p>If the code owners functionality is disabled, no dry-run validation is performed.
*/
- DRY_RUN;
+ DRY_RUN,
+
+ /**
+ * Code owner config files are validated even if the code owners functionality is disabled.
+ *
+ * <p>This option is useful when the code owner config validation should be enabled as preparation
+ * to enabling the code owners functionality.
+ */
+ FORCED,
+
+ /**
+ * Code owner config files are validated even if the code owners functionality is disabled, but
+ * invalid code owner config files are not rejected.
+ *
+ * <p>This option is useful when the code owner config validation should be enabled as preparation
+ * to enabling the code owners functionality.
+ */
+ FORCED_DRY_RUN;
public boolean isDryRun() {
- return this == CodeOwnerConfigValidationPolicy.DRY_RUN;
+ return this == CodeOwnerConfigValidationPolicy.DRY_RUN
+ || this == CodeOwnerConfigValidationPolicy.FORCED_DRY_RUN;
}
public boolean runValidation() {
return this != CodeOwnerConfigValidationPolicy.FALSE;
}
+
+ public boolean isForced() {
+ return this == CodeOwnerConfigValidationPolicy.FORCED
+ || this == CodeOwnerConfigValidationPolicy.FORCED_DRY_RUN;
+ }
}
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
index ce10743..80eece9 100644
--- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
+++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -196,7 +196,8 @@
receiveEvent.repoConfig,
receiveEvent.revWalk,
receiveEvent.commit,
- receiveEvent.user);
+ receiveEvent.user,
+ codeOwnerConfigValidationPolicy.isForced());
} catch (RuntimeException e) {
if (!codeOwnerConfigValidationPolicy.isDryRun()) {
throw e;
@@ -266,7 +267,12 @@
IdentifiedUser patchSetUploader = userFactory.create(patchSet.uploader());
validationResult =
validateCodeOwnerConfig(
- branchNameKey, repository.getConfig(), revWalk, commit, patchSetUploader);
+ branchNameKey,
+ repository.getConfig(),
+ revWalk,
+ commit,
+ patchSetUploader,
+ codeOwnerConfigValidationPolicy.isForced());
} catch (RuntimeException e) {
if (!codeOwnerConfigValidationPolicy.isDryRun()) {
throw e;
@@ -297,6 +303,8 @@
* @param revCommit the commit for which newly added and modified code owner configs should be
* validated
* @param user user for which the code owner visibility checks should be performed
+ * @param force whether the validation should be done even if the code owners functionality is
+ * disabled for the branch
* @return the validation result, {@link Optional#empty()} if no validation is performed because
* the given commit doesn't contain newly added or modified code owner configs
*/
@@ -305,10 +313,12 @@
Config repoConfig,
RevWalk revWalk,
RevCommit revCommit,
- IdentifiedUser user) {
+ IdentifiedUser user,
+ boolean force) {
CodeOwnersPluginConfigSnapshot codeOwnersConfig =
codeOwnersPluginConfiguration.getProjectConfig(branchNameKey.project());
- if (codeOwnersConfig.isDisabled(branchNameKey.branch())) {
+ logger.atFine().log("force = %s", force);
+ if (!force && codeOwnersConfig.isDisabled(branchNameKey.branch())) {
return Optional.of(
ValidationResult.create(
pluginName,
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 c35280c..d2ce328 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -425,6 +425,35 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnCommitReceived", value = "forced")
+ public void
+ cannotUploadNonParseableConfigIfCodeOwnersFunctionalityIsDisabledButValidationIsEnforced()
+ throws Exception {
+ disableCodeOwnersForProject(project);
+
+ CodeOwnerConfig.Key codeOwnerConfigKey = createCodeOwnerConfigKey("/");
+
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ "INVALID");
+ assertFatalWithMessages(
+ r,
+ "invalid code owner config files",
+ String.format(
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
+ project,
+ getParsingErrorMessage(
+ ImmutableMap.of(
+ FindOwnersBackend.class,
+ "invalid line: INVALID",
+ ProtoBackend.class,
+ "1:8: expected \"{\""))));
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.enableValidationOnCommitReceived", value = "dry_run")
public void canUploadNonParseableConfigIfValidationIsDoneAsDryRun() throws Exception {
CodeOwnerConfig.Key codeOwnerConfigKey = createCodeOwnerConfigKey("/");
@@ -450,6 +479,37 @@
}
@Test
+ @GerritConfig(
+ name = "plugin.code-owners.enableValidationOnCommitReceived",
+ value = "forced_dry_run")
+ public void
+ canUploadNonParseableConfigIfCodeOwnersFunctionalityIsDisabledButDryRunValidationIsEnforced()
+ throws Exception {
+ disableCodeOwnersForProject(project);
+
+ CodeOwnerConfig.Key codeOwnerConfigKey = createCodeOwnerConfigKey("/");
+
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ "INVALID");
+ assertOkWithFatals(
+ r,
+ "invalid code owner config files",
+ String.format(
+ "invalid code owner config file '%s' (project = %s, branch = master):\n %s",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
+ project,
+ getParsingErrorMessage(
+ ImmutableMap.of(
+ FindOwnersBackend.class,
+ "invalid line: INVALID",
+ ProtoBackend.class,
+ "1:8: expected \"{\""))));
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.readOnly", value = "true")
public void cannotUploadConfigIfConfigsAreConfiguredToBeReadOnly() throws Exception {
PushOneCommit.Result r =
@@ -1713,6 +1773,13 @@
testCanSubmitNonParseableConfig();
}
+ @Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "forced_dry_run")
+ public void canSubmitNonParseableConfigIfValidationIsDoneAsForcedDryRun() throws Exception {
+ disableCodeOwnersForProject(project);
+ testCanSubmitNonParseableConfig();
+ }
+
private void testCanSubmitNonParseableConfig() throws Exception {
setAsDefaultCodeOwners(admin);
@@ -1739,6 +1806,48 @@
}
@Test
+ @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "forced")
+ public void
+ cannotSubmitConfigWithIssuesIfCodeOwnersFunctionalityIsDisabledButValidationIsEnforced()
+ throws Exception {
+ disableCodeOwnersForProject(project);
+
+ CodeOwnerConfig.Key codeOwnerConfigKey = createCodeOwnerConfigKey("/");
+
+ // upload a change with a code owner config that has issues (non-resolvable code owners)
+ String unknownEmail = "non-existing-email@example.com";
+ PushOneCommit.Result r =
+ createChange(
+ "Add code owners",
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(),
+ format(
+ CodeOwnerConfig.builder(codeOwnerConfigKey, TEST_REVISION)
+ .addCodeOwnerSet(CodeOwnerSet.createWithoutPathExpressions(unknownEmail))
+ .build()));
+ r.assertOkStatus();
+
+ // approve the change
+ approve(r.getChangeId());
+
+ // try to submit the change
+ ResourceConflictException exception =
+ assertThrows(
+ ResourceConflictException.class,
+ () -> gApi.changes().id(r.getChangeId()).current().submit());
+ assertThat(exception)
+ .hasMessageThat()
+ .isEqualTo(
+ String.format(
+ "Failed to submit 1 change due to the following problems:\n"
+ + "Change %d: [code-owners] invalid code owner config files:\n"
+ + " ERROR: code owner email '%s' in '%s' cannot be resolved for %s",
+ r.getChange().getId().get(),
+ unknownEmail,
+ codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getFilePath(),
+ identifiedUserFactory.create(admin.id()).getLoggableName()));
+ }
+
+ @Test
@GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "false")
public void canSubmitConfigWithIssuesIfValidationIsDisabled() throws Exception {
testCanSubmitConfigWithIssues();
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md
index 9dbb6a4..570af86 100644
--- a/resources/Documentation/config.md
+++ b/resources/Documentation/config.md
@@ -185,12 +185,38 @@
<a id="pluginCodeOwnersEnableValidationOnCommitReceived">plugin.@PLUGIN@.enableValidationOnCommitReceived</a>
: 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).\
+ \
+ Can be `TRUE`, `FALSE`, `DRY_RUN`, `FORCED` or `FORCED_DRY_RUN`.\
+ \
+ `TRUE`:\
+ The code owner config file validation is enabled and the upload of
+ invalid code owner config files is rejected.\
+ If the code owners functionality is disabled, no validation is
+ performed.\
+ \
+ `FALSE`:\
+ The code owner config file validation is disabled, invalid code owner
+ config files are not rejected.\
+ \
+ `DRY_RUN`:\
+ Code owner config files are validated, but invalid code owner config
+ files are not rejected.\
+ If the code owners functionality is disabled, no dry-run validation is
+ performed.\
+ \
+ `FORCED`:\
+ Code owner config files are validated even if the code owners
+ functionality is disabled.\
+ This option is useful when the code owner config validation should be
+ enabled as preparation to enabling the code owners functionality.\
+ \
+ `FORCED_DRY_RUN`:\
+ Code owner config files are validated even if the code owners
+ functionality is disabled, but invalid code owner config files are not
+ rejected.\
+ This option is useful when the code owner config validation should be
+ enabled as preparation to enabling the code owners functionality.\
+ \
Should only be disabled if there is bot that validates the code owner
config files in open changes as part of a pre-submit validation.\
Can be overridden per project by setting
@@ -200,12 +226,39 @@
<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 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).\
+ submitted.
+ \
+ Can be `TRUE`, `FALSE`, `DRY_RUN`, `FORCED` or `FORCED_DRY_RUN`.\
+ \
+ `TRUE`:\
+ The code owner config file validation is enabled and the submission of
+ invalid code owner config files is rejected.\
+ If the code owners functionality is disabled, no validation is
+ performed.\
+ \
+ `FALSE`:\
+ The code owner config file validation is disabled, invalid code owner
+ config files are not rejected.\
+ \
+ `DRY_RUN`:\
+ Code owner config files are validated, but invalid code owner config
+ files are not rejected.\
+ If the code owners functionality is disabled, no dry-run validation is
+ performed.\
+ \
+ `FORCED`:\
+ Code owner config files are validated even if the code owners
+ functionality is disabled.\
+ This option is useful when the code owner config validation should be
+ enabled as preparation to enabling the code owners functionality.\
+ \
+ `FORCED_DRY_RUN`:\
+ Code owner config files are validated even if the code owners
+ functionality is disabled, but invalid code owner config files are not
+ rejected.\
+ This option is useful when the code owner config validation should be
+ enabled as preparation to enabling the code owners functionality.\
+ \
Disabling the submit validation is not recommended.\
Can be overridden per project by setting
[codeOwners.enableValidationOnSubmit](#codeOwnersEnableValidationOnSubmit)
@@ -592,13 +645,11 @@
in `gerrit.config` is used.
<a id="codeOwnersEnableValidationOnCommitReceived">codeOwners.enableValidationOnCommitReceived</a>
-: 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).\
+: Policy for validating code owner config files when a commit is
+ received.\
+ Can be `TRUE`, `FALSE`, `DRY_RUN`, `FORCED` or `FORCED_DRY_RUN`. For a
+ description of the values see
+ [plugin.@PLUGIN@.enableValidationOnCommitReceived](#pluginCodeOwnersEnableValidationOnCommitReceived).\
Should only be disabled if there is bot that validates the code owner
config files in open changes as part of a pre-submit validation.\
Overrides the global setting
@@ -627,12 +678,10 @@
<a id="codeOwnersEnableValidationOnSubmit">codeOwners.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
- 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).\
+ submitted.\
+ Can be `TRUE`, `FALSE`, `DRY_RUN`, `FORCED` or `FORCED_DRY_RUN`. For a
+ description of the values see
+ [plugin.@PLUGIN@.enableValidationOnSubmit](#pluginCodeOwnersEnableValidationOnSubmit).\
Disabling the submit validation is not recommended.\
Overrides the global setting
[plugin.@PLUGIN@.enableValidationOnSubmit](#pluginCodeOwnersEnableValidationOnSubmit)
