Add config option to disable validation of code owner configs on submit Together with the already existing config option to disable the validation of code owner configs on commit received, this allows admins and project owners to disable the validation completely, but doing this is not recommended. Signed-off-by: Edwin Kempin <ekempin@google.com> Change-Id: Ic191c620c418a131d1470084aa149242381a93fd
diff --git a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java index 60c14d2..1a7985d 100644 --- a/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java +++ b/java/com/google/gerrit/plugins/codeowners/config/CodeOwnersPluginConfiguration.java
@@ -130,6 +130,20 @@ } /** + * Whether code owner configs should be validated when a change is submitted. + * + * @param project the project for it should be checked whether code owner configs should be + * validated when a change is submitted + * @return whether code owner configs should be validated when a change is submitted + */ + public CodeOwnerConfigValidationPolicy getCodeOwnerConfigValidationPolicyForSubmit( + Project.NameKey project) { + requireNonNull(project, "project"); + return generalConfig.getCodeOwnerConfigValidationPolicyForSubmit( + project, getPluginConfig(project)); + } + + /** * Gets the merge commit strategy for the given project. * * @param project the project for which the merge commit strategy should be retrieved
diff --git a/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java b/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java index 8a15dcb..85fd19a 100644 --- a/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java +++ b/java/com/google/gerrit/plugins/codeowners/config/GeneralConfig.java
@@ -66,6 +66,9 @@ public static final String KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED = "enableValidationOnCommitReceived"; + @VisibleForTesting + public static final String KEY_ENABLE_VALIDATION_ON_SUBMIT = "enableValidationOnSubmit"; + @VisibleForTesting public static final String KEY_MERGE_COMMIT_STRATEGY = "mergeCommitStrategy"; @VisibleForTesting public static final String KEY_GLOBAL_CODE_OWNER = "globalCodeOwner"; @@ -240,23 +243,45 @@ * <p>The enable validation on commit received controls whether code owner config files should be * validated when a commit is received. * - * @param pluginConfig the plugin config from which the read-only configuration should be read. + * @param pluginConfig the plugin config from which the enable validation on commit received + * configuration should be read. * @return whether code owner config files should be validated when a commit is received */ CodeOwnerConfigValidationPolicy getCodeOwnerConfigValidationPolicyForCommitReceived( Project.NameKey project, Config pluginConfig) { + return getCodeOwnerConfigValidationPolicy( + KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED, project, pluginConfig); + } + + /** + * Gets the enable validation on submit configuration from the given plugin config with fallback + * to {@code gerrit.config} and default to {@code true}. + * + * <p>The enable validation on submit controls whether code owner config files should be validated + * when a change is submitted. + * + * @param pluginConfig the plugin config from which the enable validation on submit configuration + * should be read. + * @return whether code owner config files should be validated when a change is submitted + */ + CodeOwnerConfigValidationPolicy getCodeOwnerConfigValidationPolicyForSubmit( + Project.NameKey project, Config pluginConfig) { + return getCodeOwnerConfigValidationPolicy( + KEY_ENABLE_VALIDATION_ON_SUBMIT, project, pluginConfig); + } + + private CodeOwnerConfigValidationPolicy getCodeOwnerConfigValidationPolicy( + String key, Project.NameKey project, Config pluginConfig) { + requireNonNull(key, "key"); requireNonNull(project, "project"); requireNonNull(pluginConfig, "pluginConfig"); String codeOwnerConfigValidationPolicyString = - pluginConfig.getString(SECTION_CODE_OWNERS, null, KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED); + pluginConfig.getString(SECTION_CODE_OWNERS, null, key); if (codeOwnerConfigValidationPolicyString != null) { try { return pluginConfig.getEnum( - SECTION_CODE_OWNERS, - null, - KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED, - CodeOwnerConfigValidationPolicy.TRUE); + SECTION_CODE_OWNERS, null, key, CodeOwnerConfigValidationPolicy.TRUE); } catch (IllegalArgumentException e) { logger.atWarning().log( "Ignoring invalid value %s for the code owner config validation policy in '%s.config'" @@ -266,15 +291,14 @@ } try { - return pluginConfigFromGerritConfig.getEnum( - KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED, CodeOwnerConfigValidationPolicy.TRUE); + return pluginConfigFromGerritConfig.getEnum(key, CodeOwnerConfigValidationPolicy.TRUE); } catch (IllegalArgumentException e) { logger.atWarning().log( "Ignoring invalid value %s for the code owner config validation policy in gerrit.config" + " (parameter plugin.%s.%s). Falling back to default value %s.", - pluginConfigFromGerritConfig.getString(KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED), + pluginConfigFromGerritConfig.getString(key), pluginName, - KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED, + key, CodeOwnerConfigValidationPolicy.TRUE); return CodeOwnerConfigValidationPolicy.TRUE; }
diff --git a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java index 85cfca4..234ad45 100644 --- a/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java +++ b/java/com/google/gerrit/plugins/codeowners/validation/CodeOwnerConfigValidator.java
@@ -237,16 +237,44 @@ .username(caller.getLoggableName()) .patchSetId(patchSetId.get()) .build())) { - ChangeNotes changeNotes = - changeNotesFactory.create(projectState.getNameKey(), commit.change().getId()); - PatchSet patchSet = patchSetUtil.get(changeNotes, patchSetId); - IdentifiedUser patchSetUploader = userFactory.create(patchSet.uploader()); - Optional<ValidationResult> validationResult = - validateCodeOwnerConfig( - branchNameKey, repository.getConfig(), revWalk, commit, patchSetUploader); + CodeOwnerConfigValidationPolicy codeOwnerConfigValidationPolicy = + codeOwnersPluginConfiguration.getCodeOwnerConfigValidationPolicyForSubmit( + branchNameKey.project()); + logger.atFine().log("codeOwnerConfigValidationPolicy = %s", codeOwnerConfigValidationPolicy); + Optional<ValidationResult> validationResult; + if (!codeOwnerConfigValidationPolicy.runValidation()) { + validationResult = + Optional.of( + ValidationResult.create( + "skipping validation of code owner config files", + new CommitValidationMessage( + "code owners config validation is disabled", ValidationMessage.Type.HINT))); + } else { + try { + ChangeNotes changeNotes = + changeNotesFactory.create(projectState.getNameKey(), commit.change().getId()); + PatchSet patchSet = patchSetUtil.get(changeNotes, patchSetId); + IdentifiedUser patchSetUploader = userFactory.create(patchSet.uploader()); + validationResult = + validateCodeOwnerConfig( + branchNameKey, repository.getConfig(), revWalk, commit, patchSetUploader); + } catch (RuntimeException e) { + if (!codeOwnerConfigValidationPolicy.isDryRun()) { + throw e; + } + + // The validation was executed as dry-run and failures during the validation should not + // cause an error. Hence we swallow the exception here. + logger.atFine().withCause(e).log( + "ignoring failure during validation of code owner config files in revision %s" + + " (project = %s, branch = %s) because the validation was performed as dry-run", + commit.name(), branchNameKey.project(), branchNameKey.branch()); + validationResult = Optional.empty(); + } + } if (validationResult.isPresent()) { logger.atFine().log("validation result = %s", validationResult.get()); - validationResult.get().processForOnPreMerge(); + validationResult.get().processForOnPreMerge(codeOwnerConfigValidationPolicy.isDryRun()); } } } @@ -959,8 +987,8 @@ * <p>If there are no errors the validation messages are logged on fine level so that they show * up in a trace. Returning the message to the user without failing the submit is not possible. */ - void processForOnPreMerge() throws MergeValidationException { - if (hasError()) { + void processForOnPreMerge(boolean dryRun) throws MergeValidationException { + if (!dryRun && hasError()) { throw new MergeValidationException(getMessage(validationMessages())); }
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 bc42987..68b4f46 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java +++ b/javatests/com/google/gerrit/plugins/codeowners/acceptance/api/CodeOwnerConfigValidatorIT.java
@@ -1428,6 +1428,81 @@ gApi.changes().create(changeInput); } + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "false") + public void canSubmitNonParseableConfigIfValidationIsDisabled() throws Exception { + testCanSubmitNonParseableConfig(); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "dry_run") + public void canSubmitNonParseableConfigIfValidationIsDoneAsDryRun() throws Exception { + testCanSubmitNonParseableConfig(); + } + + private void testCanSubmitNonParseableConfig() throws Exception { + CodeOwnerConfig.Key codeOwnerConfigKey = createCodeOwnerConfigKey("/"); + + // disable the code owners functionality so that we can upload a non-parseable code owner config + // that we then try to submit + disableCodeOwnersForProject(project); + + PushOneCommit.Result r = + createChange( + "Add code owners", + codeOwnerConfigOperations.codeOwnerConfig(codeOwnerConfigKey).getJGitFilePath(), + "INVALID"); + r.assertOkStatus(); + + // re-enable the code owners functionality for the project + enableCodeOwnersForProject(project); + + // submit the change + approve(r.getChangeId()); + gApi.changes().id(r.getChangeId()).current().submit(); + assertThat(gApi.changes().id(r.getChangeId()).get().status).isEqualTo(ChangeStatus.MERGED); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "false") + public void canSubmitConfigWithIssuesIfValidationIsDisabled() throws Exception { + testCanSubmitConfigWithIssues(); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "dry_run") + public void canSubmitConfigWithIssuesIfValidationIsDoneAsDryRun() throws Exception { + testCanSubmitConfigWithIssues(); + } + + private void testCanSubmitConfigWithIssues() throws Exception { + CodeOwnerConfig.Key codeOwnerConfigKey = createCodeOwnerConfigKey("/"); + + // disable the code owners functionality so that we can upload a code owner config with issues + // that we then try to submit + disableCodeOwnersForProject(project); + + // upload a code owner config that has issues (non-resolvable code owners) + String unknownEmail1 = "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(unknownEmail1)) + .build())); + r.assertOkStatus(); + + // re-enable the code owners functionality for the project + enableCodeOwnersForProject(project); + + // submit the change + approve(r.getChangeId()); + gApi.changes().id(r.getChangeId()).current().submit(); + assertThat(gApi.changes().id(r.getChangeId()).get().status).isEqualTo(ChangeStatus.MERGED); + } + private CodeOwnerConfig createCodeOwnerConfigWithImport( CodeOwnerConfig.Key keyOfImportingCodeOwnerConfig, CodeOwnerConfigImportType importType,
diff --git a/javatests/com/google/gerrit/plugins/codeowners/config/GeneralConfigTest.java b/javatests/com/google/gerrit/plugins/codeowners/config/GeneralConfigTest.java index 2a92d16..6bc8ac5 100644 --- a/javatests/com/google/gerrit/plugins/codeowners/config/GeneralConfigTest.java +++ b/javatests/com/google/gerrit/plugins/codeowners/config/GeneralConfigTest.java
@@ -18,6 +18,7 @@ import static com.google.gerrit.plugins.codeowners.config.CodeOwnersPluginConfiguration.SECTION_CODE_OWNERS; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_ENABLE_IMPLICIT_APPROVALS; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_COMMIT_RECEIVED; +import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_ENABLE_VALIDATION_ON_SUBMIT; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_FALLBACK_CODE_OWNERS; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_FILE_EXTENSION; import static com.google.gerrit.plugins.codeowners.config.GeneralConfig.KEY_GLOBAL_CODE_OWNER; @@ -177,6 +178,50 @@ } @Test + public void cannotGetEnableValidationOnSubmitForNullProject() throws Exception { + NullPointerException npe = + assertThrows( + NullPointerException.class, + () -> generalConfig.getCodeOwnerConfigValidationPolicyForSubmit(null, new Config())); + assertThat(npe).hasMessageThat().isEqualTo("project"); + } + + @Test + public void cannotGetEnableValidationOnSubmitForNullPluginConfig() throws Exception { + NullPointerException npe = + assertThrows( + NullPointerException.class, + () -> generalConfig.getCodeOwnerConfigValidationPolicyForSubmit(project, null)); + assertThat(npe).hasMessageThat().isEqualTo("pluginConfig"); + } + + @Test + public void noEnableValidationOnSubmitConfiguration() throws Exception { + assertThat(generalConfig.getCodeOwnerConfigValidationPolicyForSubmit(project, new Config())) + .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "false") + public void + enableValidationOnSubmitConfigurationIsRetrievedFromGerritConfigIfNotSpecifiedOnProjectLevel() + throws Exception { + assertThat(generalConfig.getCodeOwnerConfigValidationPolicyForSubmit(project, new Config())) + .isEqualTo(CodeOwnerConfigValidationPolicy.FALSE); + } + + @Test + @GerritConfig(name = "plugin.code-owners.enableValidationOnSubmit", value = "false") + public void + enableValidationOnSubmitConfigurationInPluginConfigOverridesEnableValidationOnSubmitConfigurationInGerritConfig() + throws Exception { + Config cfg = new Config(); + cfg.setString(SECTION_CODE_OWNERS, null, KEY_ENABLE_VALIDATION_ON_SUBMIT, "true"); + assertThat(generalConfig.getCodeOwnerConfigValidationPolicyForSubmit(project, cfg)) + .isEqualTo(CodeOwnerConfigValidationPolicy.TRUE); + } + + @Test public void cannotGetMergeCommitStrategyForNullPluginConfig() throws Exception { NullPointerException npe = assertThrows(
diff --git a/resources/Documentation/config.md b/resources/Documentation/config.md index 32d3ae9..28c65b9 100644 --- a/resources/Documentation/config.md +++ b/resources/Documentation/config.md
@@ -133,6 +133,20 @@ in `@PLUGIN@.config`.\ By default `true`. +<a id="pluginCodeOwnersEnableValidationOnSubmit">plugin.@PLUGIN@.enableValidationOnSubmit</a> +: Policy for validating code owner config files when a change is + submitted. Allowed values are `true` (the code owner config file + validation is enabled and the submit of invalid code owner config files + is rejected), `false` (the code owner config file validation is + 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).\ + Disabling the submit validation is not recommended.\ + Can be overridden per project by setting + [codeOwners.enableValidationOnSubmit](#codeOwnersEnableValidationOnSubmit) + in `@PLUGIN@.config`.\ + By default `true`. + <a id="pluginCodeOwnersAllowedEmailDomain">plugin.@PLUGIN@.allowedEmailDomain</a> : Email domain that allows to assign code ownerships to emails with this domain.\ @@ -420,6 +434,22 @@ [plugin.@PLUGIN@.enableValidationOnCommitReceived](#pluginCodeOwnersEnableValidationOnCommitReceived) in `gerrit.config` is used. +<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).\ + Disabling the submit validation is not recommended.\ + Overrides the global setting + [plugin.@PLUGIN@.enableValidationOnSubmit](#pluginCodeOwnersEnableValidationOnSubmit) + in `gerrit.config`.\ + If not set, the global setting + [plugin.@PLUGIN@.enableValidationOnSubmit](#pluginCodeOwnersEnableValidationOnSubmit) + in `gerrit.config` is used. + <a id="codeOwnersRequiredApproval">codeOwners.requiredApproval</a> : Approval that is required from code owners to approve the files in a change.\
diff --git a/resources/Documentation/validation.md b/resources/Documentation/validation.md index b3b6405..4040aa0 100644 --- a/resources/Documentation/validation.md +++ b/resources/Documentation/validation.md
@@ -37,8 +37,10 @@ plugin gets installed/enabled, it is possible that invalid configuration files already exist in the repository) * updates happen behind Gerrit's back (e.g. pushes that bypass Gerrit) -* the validation is disabled in the - [plugin configuration](config.html#codeOwnersEnableValidationOnCommitReceived). +* the validation is disabled via the + [enableValidationOnCommitReceived](config.html#codeOwnersEnableValidationOnCommitReceived) + or [enableValidationOnSubmit](config.html#codeOwnersEnableValidationOnSubmit) + config options In addition for [code owner config files](user-guide.html#codeOwnerConfigFiles) no validation is done when: